shadow variables - pg15 edition

On Thu, Aug 18, 2022 at 12:54 AM Justin Pryzby <pryzby@telsasoft.com> wrote:

There's been no progress on this in the past discussions.

/messages/by-id/877k1psmpf.fsf@mailbox.samurai.com
/messages/by-id/CAApHDvpqBR7u9yzW4yggjG=QfN=FZsc8Wo2ckokpQtif-+iQ2A@mail.gmail.com
/messages/by-id/MN2PR18MB2927F7B5F690065E1194B258E35D0@MN2PR18MB2927.namprd18.prod.outlook.com

But an unfortunate consequence of not fixing the historic issues is that it
precludes the possibility that anyone could be expected to notice if they
introduce more instances of the same problem (as in the first half of these
patches). Then the hole which has already been dug becomes deeper, further
increasing the burden of fixing the historic issues before being able to use
-Wshadow.

The first half of the patches fix shadow variables newly-introduced in v15
(including one of my own patches), the rest are fixing the lowest hanging fruit
of the "short list" from COPT=-Wshadow=compatible-local

I can't see that any of these are bugs, but it seems like a good goal to move
towards allowing use of the -Wshadow* options to help avoid future errors, as
well as cleanliness and readability (rather than allowing it to get harder to
use -Wshadow).

Hey, thanks for picking this up!

I'd started looking at these [1]/messages/by-id/CAHut+Puv4LaQKVQSErtV_=3MezUdpipVOMt7tJ3fXHxt_YK-Zw@mail.gmail.com last year and spent a day trying to
categorise them all in a spreadsheet (shadows a global, shadows a
parameter, shadows a local var etc) but I became swamped by the
volume, and then other work/life got in the way.

+1 from me.

------
[1]: /messages/by-id/CAHut+Puv4LaQKVQSErtV_=3MezUdpipVOMt7tJ3fXHxt_YK-Zw@mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

On Thu, Aug 18, 2022 at 08:49:14AM +1000, Peter Smith wrote:

I'd started looking at these [1] last year and spent a day trying to
categorise them all in a spreadsheet (shadows a global, shadows a
parameter, shadows a local var etc) but I became swamped by the
volume, and then other work/life got in the way.

+1 from me.

A lot of the changes proposed here update the code so as the same
variable gets used across more code paths by removing declarations,
but we have two variables defined because both are aimed to be used in
a different context (see AttachPartitionEnsureIndexes() in tablecmds.c
for example).

Wouldn't it be a saner approach in a lot of cases to rename the
shadowed variables (aka the ones getting removed in your patches) and
keep them local to the code paths where we use them?
--
Michael

On Thu, Aug 18, 2022 at 09:39:02AM +0900, Michael Paquier wrote:

On Thu, Aug 18, 2022 at 08:49:14AM +1000, Peter Smith wrote:

I'd started looking at these [1] last year and spent a day trying to
categorise them all in a spreadsheet (shadows a global, shadows a
parameter, shadows a local var etc) but I became swamped by the
volume, and then other work/life got in the way.

+1 from me.

A lot of the changes proposed here update the code so as the same
variable gets used across more code paths by removing declarations,
but we have two variables defined because both are aimed to be used in
a different context (see AttachPartitionEnsureIndexes() in tablecmds.c
for example).

Wouldn't it be a saner approach in a lot of cases to rename the
shadowed variables (aka the ones getting removed in your patches) and
keep them local to the code paths where we use them?

The cases where I removed a declaration are ones where the variable either
hasn't yet been assigned in the outer scope (so it's safe to use first in the
inner scope, since its value is later overwriten in the outer scope). Or it's
no longer used in the outer scope, so it's safe to re-use it in the inner scope
(as in AttachPartitionEnsureIndexes). Since you think it's saner, I changed to
rename them.

In the case of "first", the var is used in two independent loops, the same way,
and re-initialized. In the case of found_whole_row, the var is ignored, as the
comments say, so it would be silly to declare more vars to be additionally
ignored.

--
Justin

PS. I hadn't sent the other patches which rename the variables, having assumed
that the discussion would be bikeshedded to death and derail without having
fixed the lowest hanging fruits. I'm attaching them those now to see what
happens.

On Thu, 18 Aug 2022 at 02:54, Justin Pryzby <pryzby@telsasoft.com> wrote:

The first half of the patches fix shadow variables newly-introduced in v15
(including one of my own patches), the rest are fixing the lowest hanging fruit
of the "short list" from COPT=-Wshadow=compatible-local

I wonder if it's better to fix the "big hitters" first. The idea
there would be to try to reduce the number of these warnings as
quickly and easily as possible. If we can get the numbers down fairly
significantly without too much effort, then that should provide us
with a bit more motivation to get rid of the remaining ones.

Here are the warnings grouped by the name of the variable:

$ make -s 2>&1 | grep "warning: declaration of" | grep -oP
"‘([_a-zA-Z]{1}[_a-zA-Z0-9]*)’" | sort | uniq -c
2 ‘aclresult’
3 ‘attnum’
1 ‘cell’
1 ‘cell__state’
2 ‘cmp’
2 ‘command’
1 ‘constraintOid’
1 ‘copyTuple’
1 ‘data’
1 ‘db’
1 ‘_do_rethrow’
1 ‘dpns’
1 ‘econtext’
1 ‘entry’
36 ‘expected’
1 ‘first’
1 ‘found_whole_row’
1 ‘host’
20 ‘i’
1 ‘iclause’
1 ‘idxs’
1 ‘i_oid’
4 ‘isnull’
1 ‘it’
2 ‘item’
1 ‘itemno’
1 ‘j’
1 ‘jtc’
1 ‘k’
1 ‘keyno’
7 ‘l’
13 ‘lc’
4 ‘lc__state’
1 ‘len’
1 ‘_local_sigjmp_buf’
1 ‘name’
2 ‘now’
1 ‘owning_tab’
1 ‘page’
1 ‘partitionId’
2 ‘path’
3 ‘proc’
1 ‘proclock’
1 ‘querytree_list’
1 ‘range’
1 ‘rel’
1 ‘relation’
1 ‘relid’
1 ‘rightop’
2 ‘rinfo’
1 ‘_save_context_stack’
1 ‘save_errno’
1 ‘_save_exception_stack’
1 ‘slot’
1 ‘sqlca’
9 ‘startelem’
1 ‘stmt_list’
2 ‘str’
1 ‘subpath’
1 ‘tbinfo’
1 ‘ti’
1 ‘transno’
1 ‘ttype’
1 ‘tuple’
5 ‘val’
1 ‘value2’
1 ‘wco’
1 ‘xid’
1 ‘xlogfname’

The top 5 by count here account for about half of the warnings, so
maybe is best to start with those? Likely the ones ending in __state
will fix themselves when you fix the variable with the same name
without that suffix.

The attached patch targets fixing the "expected" variable.

$ ./configure --prefix=/home/drowley/pg
CFLAGS="-Wshadow=compatible-local" > /dev/null
$ make clean -s
$ make -j -s 2>&1 | grep "warning: declaration of" | wc -l
153
$ make clean -s
$ patch -p1 < reduce_local_variable_shadow_warnings_in_regress.c.patch
$ make -j -s 2>&1 | grep "warning: declaration of" | wc -l
117

So 36 fewer warnings with the attached.

I'm probably not the only committer to want to run a mile when they
see someone posting 17 or 26 patches in an email. So maybe "bang for
buck" is a better method for getting the ball rolling here. As you
know, I was recently bitten by local shadows in af7d270dd, so I do
believe in the cause.

What do you think?

David

Michael Paquier <michael@paquier.xyz> writes:

A lot of the changes proposed here update the code so as the same
variable gets used across more code paths by removing declarations,
but we have two variables defined because both are aimed to be used in
a different context (see AttachPartitionEnsureIndexes() in tablecmds.c
for example).

Wouldn't it be a saner approach in a lot of cases to rename the
shadowed variables (aka the ones getting removed in your patches) and
keep them local to the code paths where we use them?

Yeah. I do not think a patch of this sort has any business changing
the scopes of variables. That moves it out of "cosmetic cleanup"
and into "hm, I wonder if this introduces any bugs". Most hackers
are going to decide that they have better ways to spend their time
than doing that level of analysis for a very noncritical patch.

regards, tom lane

On Thu, Aug 18, 2022 at 03:17:33PM +1200, David Rowley wrote:

I'm probably not the only committer to want to run a mile when they
see someone posting 17 or 26 patches in an email. So maybe "bang for
buck" is a better method for getting the ball rolling here. As you
know, I was recently bitten by local shadows in af7d270dd, so I do
believe in the cause.

What do you think?

You already fixed the shadow var introduced in master/pg16, and I sent patches
for the shadow vars added in pg15 (marked as such and presented as 001-008), so
perhaps it's okay to start with that ?

BTW, one of the remaining warnings seems to be another buglet, which I'll write
about at a later date.

--
Justin

On Thu, 18 Aug 2022 at 17:16, Justin Pryzby <pryzby@telsasoft.com> wrote:

On Thu, Aug 18, 2022 at 03:17:33PM +1200, David Rowley wrote:

I'm probably not the only committer to want to run a mile when they
see someone posting 17 or 26 patches in an email. So maybe "bang for
buck" is a better method for getting the ball rolling here. As you
know, I was recently bitten by local shadows in af7d270dd, so I do
believe in the cause.

What do you think?

You already fixed the shadow var introduced in master/pg16, and I sent patches
for the shadow vars added in pg15 (marked as such and presented as 001-008), so
perhaps it's okay to start with that ?

Alright, I made a pass over the 0001-0008 patches.

0001. I'd also rather see these 4 renamed:

+++ b/src/bin/pg_dump/pg_dump.c
@@ -3144,7 +3144,6 @@ dumpDatabase(Archive *fout)
  PQExpBuffer loHorizonQry = createPQExpBuffer();
  int i_relfrozenxid,
  i_relfilenode,
- i_oid,
  i_relminmxid;

Adding an extra 'i' (for inner) on the front seems fine to me.

0002. I don't really like the "my" name. I also see you've added the
word "this" to many other variables that are shadowing. It feels kinda
like you're missing a "self" and a "me" in there somewhere! :)

@@ -7080,21 +7080,21 @@ getConstraints(Archive *fout, TableInfo
tblinfo[], int numTables)
appendPQExpBufferChar(tbloids, '{');
for (int i = 0; i < numTables; i++)
{
- TableInfo *tbinfo = &tblinfo[i];
+ TableInfo *mytbinfo = &tblinfo[i];

How about just "tinfo"?

0003. The following is used for the exact same purpose as its shadowed
counterpart. I suggest just using the variable from the outer scope.

@@ -16799,21 +16799,21 @@ dumpSequence(Archive *fout, const TableInfo *tbinfo)
  */
  if (OidIsValid(tbinfo->owning_tab) && !tbinfo->is_identity_sequence)
  {
- TableInfo  *owning_tab = findTableByOid(tbinfo->owning_tab);
+ TableInfo  *this_owning_tab = findTableByOid(tbinfo->owning_tab);

0004. I would rather people used foreach_current_index(lc) > 0 to
determine when we're not doing the first iteration of a foreach loop.
I understand there are more complex cases with filtering that this
cannot be done, but these are highly simple and using
foreach_current_index() removes multiple lines of code and makes it
look nicer.

@@ -762,8 +762,8 @@ fetch_remote_table_info(char *nspname, char *relname,
TupleTableSlot *slot;
Oid attrsRow[] = {INT2VECTOROID};
StringInfoData pub_names;
- bool first = true;

+ first = true;
initStringInfo(&pub_names);
foreach(lc, MySubscription->publications)

0005. How about just "tslot". I'm not a fan of "this".

+++ b/src/backend/replication/logical/tablesync.c
@@ -759,7 +759,7 @@ fetch_remote_table_info(char *nspname, char *relname,
  if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 150000)
  {
  WalRcvExecResult *pubres;
- TupleTableSlot *slot;
+ TupleTableSlot *thisslot;

0006. A see the outer shadowed counterpart is used to add a new backup
type. Since I'm not a fan of "this", how about the outer one gets
renamed to "newtype"?

+++ b/src/backend/backup/basebackup_target.c
@@ -73,9 +73,9 @@ BaseBackupAddTarget(char *name,
  /* Search the target type list for an existing entry with this name. */
  foreach(lc, BaseBackupTargetTypeList)
  {
- BaseBackupTargetType *ttype = lfirst(lc);
+ BaseBackupTargetType *this_ttype = lfirst(lc);

0007. Meh, more "this". How about just "col".

+++ b/src/backend/parser/parse_jsontable.c
@@ -341,13 +341,13 @@ transformJsonTableChildPlan(JsonTableContext
*cxt, JsonTablePlan *plan,
  /* transform all nested columns into cross/union join */
  foreach(lc, columns)
  {
- JsonTableColumn *jtc = castNode(JsonTableColumn, lfirst(lc));
+ JsonTableColumn *thisjtc = castNode(JsonTableColumn, lfirst(lc));

There's a discussion about reverting this entire patch. Not sure if
patching master and not backpatching to pg15 would be useful to the
people who may be doing that revert.

0008. Sorry, I had to change this one too. I just have an aversion to
variables named "temp" or "tmp".

+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -3109,10 +3109,10 @@ JsonItemFromDatum(Datum val, Oid typid, int32
typmod, JsonbValue *res)

  if (JsonContainerIsScalar(&jb->root))
  {
- bool res PG_USED_FOR_ASSERTS_ONLY;
+ bool tmp PG_USED_FOR_ASSERTS_ONLY;

- res = JsonbExtractScalar(&jb->root, jbv);
- Assert(res);
+ tmp = JsonbExtractScalar(&jb->root, jbv);
+ Assert(tmp);

I've attached a patch which does things more along the lines of how I
would have done it. I don't think we should be back patching this
stuff.

Any objections to pushing this to master only?

David

On Thu, Aug 18, 2022 at 5:27 PM David Rowley <dgrowleyml@gmail.com> wrote:

On Thu, 18 Aug 2022 at 17:16, Justin Pryzby <pryzby@telsasoft.com> wrote:

On Thu, Aug 18, 2022 at 03:17:33PM +1200, David Rowley wrote:

I'm probably not the only committer to want to run a mile when they
see someone posting 17 or 26 patches in an email. So maybe "bang for
buck" is a better method for getting the ball rolling here. As you
know, I was recently bitten by local shadows in af7d270dd, so I do
believe in the cause.

What do you think?

You already fixed the shadow var introduced in master/pg16, and I sent patches
for the shadow vars added in pg15 (marked as such and presented as 001-008), so
perhaps it's okay to start with that ?

Alright, I made a pass over the 0001-0008 patches.

...

0005. How about just "tslot". I'm not a fan of "this".

(I'm sure there are others like this; I just picked this one as an example)

AFAICT the offending 'slot' really should have never been declared at
all at the local scope in the first place - e.g. the other code in
this function seems happy enough with the pattern of just re-using the
function scoped 'slot'.

I understand that for this shadow patch changing the var-name is
considered the saner/safer way than tampering with the scope, but
perhaps it is still useful to include a comment when changing ones
like this?

e.g.
+ TupleTableSlot *tslot; /* TODO - Why declare this at all? Shouldn't
it just re-use the 'slot' at function scope? */

Otherwise, such knowledge will be lost, and nobody will ever know to
revisit them, which feels a bit more like *hiding* the mistake than
fixing it.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Thu, Aug 18, 2022 at 07:27:09PM +1200, David Rowley wrote:

0001. I'd also rather see these 4 renamed:

..

0002. I don't really like the "my" name. I also see you've added the

..

How about just "tinfo"?

..

0005. How about just "tslot". I'm not a fan of "this".

..

Since I'm not a fan of "this", how about the outer one gets renamed

..

0007. Meh, more "this". How about just "col".

..

0008. Sorry, I had to change this one too.

I agree that ii_oid and newtype are better names (although it's a bit
unfortunate to rename the outer "ttype" var of wider scope).

0003. The following is used for the exact same purpose as its shadowed
counterpart. I suggest just using the variable from the outer scope.

And that's what my original patch did, before people insisted that the patches
shouldn't change variable scope. Now it's back to where I stared.

There's a discussion about reverting this entire patch. Not sure if
patching master and not backpatching to pg15 would be useful to the
people who may be doing that revert.

I think if it were reverted, it'd be in both branches.

I've attached a patch which does things more along the lines of how I
would have done it. I don't think we should be back patching this
stuff.

Any objections to pushing this to master only?

I won't object, but some of your changes are what makes backpatching this less
reasonable (foreach_current_index and newtype). I had made these v15 patches
first to simplify backpatching, since having the same code in v15 means that
there's no backpatch hazard for this new-in-v15 code.

I am opened to presenting the patches differently, but we need to come up with
a better process than one person writing patches and someone else rewriting it.
I also don't see the value of debating which order to write the patches in.
Grouping by variable name or doing other statistical analysis doesn't change
the fact that there are 50+ issues to address to allow -Wshadow to be usable.

Maybe these would be helpful ?
- if I publish the patches on github;
- if I send the patches with more context;
- if you have an suggestion/objection/complaint with a patch, I can address it
and/or re-arrange the patchset so this is later, and all the polished
patches are presented first.

--
Justin

On Fri, Aug 19, 2022 at 9:21 AM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Thu, Aug 18, 2022 at 07:27:09PM +1200, David Rowley wrote:

0001. I'd also rather see these 4 renamed:

..

0002. I don't really like the "my" name. I also see you've added the

..

How about just "tinfo"?

..

0005. How about just "tslot". I'm not a fan of "this".

..

Since I'm not a fan of "this", how about the outer one gets renamed

..

0007. Meh, more "this". How about just "col".

..

0008. Sorry, I had to change this one too.

I agree that ii_oid and newtype are better names (although it's a bit
unfortunate to rename the outer "ttype" var of wider scope).

0003. The following is used for the exact same purpose as its shadowed
counterpart. I suggest just using the variable from the outer scope.

And that's what my original patch did, before people insisted that the patches
shouldn't change variable scope. Now it's back to where I stared.

There's a discussion about reverting this entire patch. Not sure if
patching master and not backpatching to pg15 would be useful to the
people who may be doing that revert.

I think if it were reverted, it'd be in both branches.

I've attached a patch which does things more along the lines of how I
would have done it. I don't think we should be back patching this
stuff.

Any objections to pushing this to master only?

I won't object, but some of your changes are what makes backpatching this less
reasonable (foreach_current_index and newtype). I had made these v15 patches
first to simplify backpatching, since having the same code in v15 means that
there's no backpatch hazard for this new-in-v15 code.

I am opened to presenting the patches differently, but we need to come up with
a better process than one person writing patches and someone else rewriting it.
I also don't see the value of debating which order to write the patches in.
Grouping by variable name or doing other statistical analysis doesn't change
the fact that there are 50+ issues to address to allow -Wshadow to be usable.

Maybe these would be helpful ?
- if I publish the patches on github;
- if I send the patches with more context;
- if you have an suggestion/objection/complaint with a patch, I can address it
and/or re-arrange the patchset so this is later, and all the polished
patches are presented first.

Starting off with patches might come to grief, and it won't be much
fun rearranging patches over and over.

Because there are so many changes, I think it would be better to
attack this task methodically:

STEP 1 - Capture every shadow warning and categorise exactly what kind
is it. e.g maybe do this as some XLS which can be shared. The last
time I looked there were hundreds of instances, but I expect there
will be less than a couple of dozen different *categories* of them.

e.g. shadow of a global var
e.g. shadow of a function param
e.g. shadow of a function var in a code block for the exact same usage
e.g. shadow of a function var in a code block for some 'tmp' var
e.g. shadow of a function var in a code block due to a mistake
e.g. shadow of a function var by some loop index
e.g. shadow of a function var for some loop 'first' handling
e.g. bug
etc...

STEP 2 - Define your rules for how intend to address each of these
kinds of shadows (e.g. just simple rename of the var, use
'foreach_current_index', ...). Hopefully, it will be easy to reach an
agreement now since all instances of the same kind will look pretty
much the same.

STEP 3 - Fix all of the same kinds of shadows per single patch (using
the already agreed fix approach from step 2).

REPEAT STEPS 2,3 until done.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, 19 Aug 2022 at 11:21, Justin Pryzby <pryzby@telsasoft.com> wrote:

On Thu, Aug 18, 2022 at 07:27:09PM +1200, David Rowley wrote:

Any objections to pushing this to master only?

I won't object, but some of your changes are what makes backpatching this less
reasonable (foreach_current_index and newtype). I had made these v15 patches
first to simplify backpatching, since having the same code in v15 means that
there's no backpatch hazard for this new-in-v15 code.

I spent a bit more time on this and I see that make check-world does
fail if I change either of the foreach_current_index() changes to be
incorrect. e.g change the condition from "> 0" to be "== 0", "> 1" or
"> -1".

As for the newtype change, I was inclined to give the variable name
with the most meaning to the one that's in scope for longer.

I'm starting to feel like it would be ok to backpatch these
new-to-pg-15 changes back into PG15. The reason I think this is that
they all seem low enough risk that it's probably more risky to not
backpatch and risk bugs being introduced due to mistakes being made in
conflict resolution when future patches don't apply. It was the
failing tests I mentioned above that swayed me on this.

I am opened to presenting the patches differently, but we need to come up with
a better process than one person writing patches and someone else rewriting it.

It wasn't my intention to purposefully rewrite everything. It's just
that in order to get the work into something I was willing to commit,
that's how it ended up. As for why I did that rather than ask you to
was the fact that doing it myself required fewer keystrokes, mental
effort and time than asking you to. It's not my intention to do that
for any personal credit. I'm happy for you to take that. I'd just
rather not be batting such trivial patches over the fence at each
other for days or weeks. The effort-to-reward ratio for that is
probably going to drop below my threshold after a few rounds.

David

On Fri, Aug 19, 2022 at 03:37:52PM +1200, David Rowley wrote:

I'm happy for you to take that. I'd just rather not be batting such trivial
patches over the fence at each other for days or weeks.

Yes, thanks for that.
I read through your patch, which looks fine.
Let me know what I can do when it's time for round two.

--
Justin

On Fri, 19 Aug 2022 at 16:28, Justin Pryzby <pryzby@telsasoft.com> wrote:

Let me know what I can do when it's time for round two.

I pushed the modified 0001-0008 patches earlier today and also the one
I wrote to fixup the 36 warnings about "expected" being shadowed.

I looked through a bunch of your remaining patches and was a bit
unexcited to see many more renaming such as:

- List    *querytree_list;
+ List    *this_querytree_list;

I don't think this sort of thing is an improvement.

However, one category of these changes that I do like are the ones
where we can move the variable into an inner scope. Out of your
renaming 0009-0026 patches, these are:

0013
0014
0017
0018

I feel like having the variable in scope for the minimal amount of
time makes the code cleaner and I feel like these are good next steps
because:

a) no variable needs to be renamed
b) any backpatching issues is more likely to lead to compilation
failure rather than using the wrong variable.

Likely 0016 is a subcategory of the above as if you modified that
patch to follow this rule then you'd have to declare the variable a
few times. I think that category is less interesting and we can maybe
consider those after we're done with the more simple ones.

Do you want to submit a series of patches that fixes all of the
remaining warnings that are in this category? Once these are done we
can consider the best ways to fix and if we want to fix any of the
remaining ones.

Feel free to gzip the patches up if the number is large.

David

On Sat, Aug 20, 2022 at 09:17:41PM +1200, David Rowley wrote:

On Fri, 19 Aug 2022 at 16:28, Justin Pryzby <pryzby@telsasoft.com> wrote:

Let me know what I can do when it's time for round two.

I pushed the modified 0001-0008 patches earlier today and also the one
I wrote to fixup the 36 warnings about "expected" being shadowed.

Thank you

I looked through a bunch of your remaining patches and was a bit
unexcited to see many more renaming such as:

Yes - after Michael said that was the sane procedure, I had rearranged the
patch series to present eariler those patches first which renamed variables ..

However, one category of these changes that I do like are the ones
where we can move the variable into an inner scope.

There are a lot of these, which ISTM is a good thing.
This fixes about half of the remaining warnings.

https://github.com/justinpryzby/postgres/tree/avoid-shadow-vars
You could review without applying the patches, on the webpage or (probably
better) by adding as a git remote. Attached is a squished version.

--
Justin

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

On Tue, Aug 23, 2022 at 01:38:40PM +1200, David Rowley wrote:

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:
... in fact, there's lots of renaming, so I'll just stop looking.

Actually, they didn't sneak in - what I sent are the patches which are ready to
be reviewed, excluding the set of "this" and "tmp" and other renames which you
disliked. In the branch (not the squished patch) the first ~15 patches were
mostly for C99 for loops - I presented them this way deliberately, so you could
review and comment on whatever you're able to bite off, or run with whatever
parts you think are ready. I rewrote it now to be more bite sized by
truncating off the 2nd half of the patches.

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?

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.

IMO it doesn't make sense to declare multiple integers for something like this
whether they're all ignored. Nor for "save_errno" nor the third, similar case,
for the reason in the commit message.

--
Justin

On Tue, 23 Aug 2022 at 14:14, Justin Pryzby <pryzby@telsasoft.com> wrote:

Actually, they didn't sneak in - what I sent are the patches which are ready to
be reviewed, excluding the set of "this" and "tmp" and other renames which you
disliked. In the branch (not the squished patch) the first ~15 patches were
mostly for C99 for loops - I presented them this way deliberately, so you could
review and comment on whatever you're able to bite off, or run with whatever
parts you think are ready. I rewrote it now to be more bite sized by
truncating off the 2nd half of the patches.

Thanks for the updated patch.

I've now pushed it after making some small adjustments.

It seems there was one leftover rename still there, I removed that.
The only other changes I made were to just make the patch mode
consistent with what it was doing. There were a few cases where you
were doing:

  if (typlen == -1) /* varlena */
  {
- int i;
-
- for (i = 0; i < nvalues; i++)
+ for (int i = 0; i < nvalues; i++)

That wasn't really required to remove the warning as you'd already
adjusted the scope of the shadowed variable so there was no longer a
collision. The reason I adjusted these was because sometimes you were
doing that, and sometimes you were not. I wanted to be consistent, so
I opted for not doing it as it's not required for this effort. Maybe
one day those can be changed in some other unrelated effort to C99ify
our code.

The attached patch is just the portions I didn't commit.

Thanks for working on this.

David

On Wed, Aug 24, 2022 at 12:37:29PM +1200, David Rowley wrote:

On Tue, 23 Aug 2022 at 14:14, Justin Pryzby <pryzby@telsasoft.com> wrote:

Actually, they didn't sneak in - what I sent are the patches which are ready to
be reviewed, excluding the set of "this" and "tmp" and other renames which you
disliked. In the branch (not the squished patch) the first ~15 patches were
mostly for C99 for loops - I presented them this way deliberately, so you could
review and comment on whatever you're able to bite off, or run with whatever
parts you think are ready. I rewrote it now to be more bite sized by
truncating off the 2nd half of the patches.

Thanks for the updated patch.

I've now pushed it after making some small adjustments.

Thanks for handling them.

Attached are half of the remainder of what I've written, ready for review.

I also put it here: https://github.com/justinpryzby/postgres/tree/avoid-shadow-vars

You may or may not find the associated commit messages to be useful.
Let me know if you'd like the individual patches included here, instead.

The first patch removes 2ndary, "inner" declarations, where that seems
reasonably safe and consistent with existing practice (and probably what the
original authors intended or would have written).

--
Justin

On Wed, 24 Aug 2022 at 14:39, Justin Pryzby <pryzby@telsasoft.com> wrote:

Attached are half of the remainder of what I've written, ready for review.

Thanks for the patches.

I started to do some analysis of the remaining warnings and put them
in the attached spreadsheet. I put each of the remaining warnings into
a category of how I think they should be fixed.

These categories are:

1. "Rescope" (adjust scope of outer variable to move it into a deeper scope)
2. "Rename" (a variable needs to be renamed)
3. "RenameOrScope" (a variable needs renamed or we need to something
more extreme to rescope)
4. "Repurpose" (variables have the same purpose and may as well use
the same variable)
5. "Refactor" (fix the code to make it better)
6. "Remove" (variable is not needed)

There's also:
7. "Bug?" (might be a bug)
8. "?" (I don't know)

I was hoping we'd already caught all of the #1s in 421892a19, but I
caught a few of those in some of your other patches. One you'd done
another way and some you'd done the rescope but just put it in the
wrong patch. The others had not been done yet. I just pushed
f959bf9a5 to fix those ones.

I really think #2s should be done last. I'm not as comfortable with
the renaming and we might want to discuss tactics on that. We could
either opt to rename the shadowed or shadowing variable, or both. If
we rename the shadowing variable, then pending patches or forward
patches could use the wrong variable. If we rename the shadowed
variable then it's not impossible that backpatching could go wrong
where the new code intends to reference the outer variable using the
newly named variable, but when that's backpatched it uses the variable
with the same name in the inner scope. Renaming both would make the
problem more obvious. I'm not sure which is best. The answer may
depend on how many lines the variable is in scope for. If it's just
for a few lines then the hunk context would conflict and the committer
would likely notice the issue when resolving the conflict.

For #3, I just couldn't decide the best fix. Many of these could be
moved into an inner scope, but it would require indenting a large
amount of code, e.g. in a switch() statement's "case:" to allow
variables to be declared within the case.

I think probably #4 should be next to do (maybe after #5)

I have some ideas on how to fix the two #5s, so I'm going to go and do that now.

There's only 1 #6. I'm not so sure on that yet. The variable being
assigned to the variable is the current time and I'm not sure if we
can reuse the existing variable or not as time may have moved on
sufficiently.

I'll study #7 a bit more. My eyes glazed over a bit from doing all
that analysis, so I might be mistaken about that being a bug.

For #8s. These are the PG_TRY() ones. I see you had a go at fixing
that by moving the nested PG_TRY()s to a helper function. I don't
think that's a good fix. If we were to ever consider making
-Wshadow=compatible-local in a standard build, then we'd basically be
saying that nested PG_TRYs are not allowed. I don't think that'll fly.
I'd rather find a better way to fix those. I see we can't make use of
##__LINE__ in the variable name since PG_TRY()'s friends use the
variables too and they'd be on a different line. We maybe could have
an "ident" parameter in the macro that we ##ident onto the variables
names, but that would break existing code.

The first patch removes 2ndary, "inner" declarations, where that seems
reasonably safe and consistent with existing practice (and probably what the
original authors intended or would have written).

Would you be able to write a patch for #4. I'll do #5 now. You could
do a draft patch for #2 as well, but I think it should be committed
last, if we decide it's a good move to make. It may be worth having
the discussion about if we actually want to run
-Wshadow=compatible-local as a standard build flag before we rename
anything.

David