Re: shadow variables - pg15 edition

On Tue, 23 Aug 2022 at 13:17, Justin Pryzby <pryzby@telsasoft.com> wrote:
>  Attached is a squished version.

I see there's some renaming ones snuck in there. e.g:

- Relation rel;
- HeapTuple tuple;
+ Relation pg_foreign_table;
+ HeapTuple foreigntuple;

This one does not seem to be in the category I mentioned:

@@ -3036,8 +3036,6 @@ XLogFileInitInternal(XLogSegNo logsegno,
TimeLineID logtli,
  pgstat_report_wait_start(WAIT_EVENT_WAL_INIT_SYNC);
  if (pg_fsync(fd) != 0)
  {
- int save_errno = errno;
-

More renaming:

+++ b/src/backend/catalog/heap.c
@@ -1818,19 +1818,19 @@ heap_drop_with_catalog(Oid relid)
  */
  if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
  {
- Relation rel;
- HeapTuple tuple;
+ Relation pg_foreign_table;
+ HeapTuple foreigntuple;

More renaming:

+++ b/src/backend/commands/publicationcmds.c
@@ -106,7 +106,7 @@ parse_publication_options(ParseState *pstate,
  {
  char    *publish;
  List    *publish_list;
- ListCell   *lc;
+ ListCell   *lc2;

and again:

+++ b/src/backend/commands/tablecmds.c
@@ -10223,7 +10223,7 @@ CloneFkReferencing(List **wqueue, Relation
parentRel, Relation partRel)
  Oid constrOid;
  ObjectAddress address,
  referenced;
- ListCell   *cell;
+ ListCell   *lc;

I've not checked the context one this, but this does not appear to
meet the category of moving to an inner scope:

+++ b/src/backend/executor/execPartition.c
@@ -768,7 +768,6 @@ ExecInitPartitionInfo(ModifyTableState *mtstate,
EState *estate,
  {
  List    *onconflset;
  List    *onconflcols;
- bool found_whole_row;

Looks like you're just using the one from the wider scope. That's not
the category we're after for now.

You've also got some renaming going on in ExecInitAgg()

- phasedata->gset_lengths[i] = perhash->numCols = aggnode->numCols;
+ phasedata->gset_lengths[setno] = perhash->numCols = aggnode->numCols;

I wondered about this one too:

- i = -1;
- while ((i = bms_next_member(all_grouped_cols, i)) >= 0)
- aggstate->all_grouped_cols = lcons_int(i, aggstate->all_grouped_cols);
+ {
+ int i = -1;
+ while ((i = bms_next_member(all_grouped_cols, i)) >= 0)
+ aggstate->all_grouped_cols = lcons_int(i, aggstate->all_grouped_cols);
+ }

I had in mind that maybe we should switch those to be something more like:

for (int i = -1; (i = bms_next_member(all_grouped_cols, i)) >= 0;)

But I had 2nd thoughts as the "while" version has become the standard method.

(Really that code should be using bms_prev_member() and lappend_int()
so we don't have to memmove() the entire list each lcons_int() call.
(not for this patch though))

More renaming being done here:

- int i; /* Index into *ident_user */
+ int j; /* Index into *ident_user */

... in fact, there's lots of renaming, so I'll just stop looking.

Can you just send a patch that only changes the cases where you can
remove a variable declaration from an outer scope into a single inner
scope, or multiple inner scope when the variable can be declared
inside a for() loop? The mcv_get_match_bitmap() change is an example
of this.  There's still a net reduction in lines of code, so I think
the mcv_get_match_bitmap(), and any like it are ok for this next step.
A counter example is ExecInitPartitionInfo() where the way to do this
would be to move the found_whole_row declaration into multiple inner
scopes.  That's a net increase in code lines, for which I think
requires more careful thought if we want that or not.

David