Horton

Thread

Commits

  1. Fix shadow variable in postgres.c

  2. Add -Wshadow=compatible-local to the standard compilation flags

  3. Fix final warnings produced by -Wshadow=compatible-local

  4. Fix final compiler warning produced by -Wshadow=compatible-local

  5. Add optional parameter to PG_TRY() macros

  6. Small refactor to get rid of -Wshadow=compatible-local warning

  7. More -Wshadow=compatible-local warning fixes

  8. Further -Wshadow=compatible-local warning fixes

  9. Further reduce warnings with -Wshadow=compatible-local

  10. Reduce warnings with -Wshadow=compatible-local builds

  11. Remove shadowed local variables that are new in v15

  1. shadow variables - pg15 edition

    Justin Pryzby <pryzby@telsasoft.com> — 2022-08-17T14:54:35Z

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

https://www.postgresql.org/message-id/flat/877k1psmpf.fsf%40mailbox.samurai.com
https://www.postgresql.org/message-id/flat/CAApHDvpqBR7u9yzW4yggjG%3DQfN%3DFZsc8Wo2ckokpQtif-%2BiQ2A%40mail.gmail.com#2d900bfe18fce17f97ec1f00800c8e27
https://www.postgresql.org/message-id/flat/MN2PR18MB2927F7B5F690065E1194B258E35D0%40MN2PR18MB2927.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).

-- 
Justin

  2. Re: shadow variables - pg15 edition

    Peter Smith <smithpb2250@gmail.com> — 2022-08-17T22:49:14Z

    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.
>
> https://www.postgresql.org/message-id/flat/877k1psmpf.fsf%40mailbox.samurai.com
> https://www.postgresql.org/message-id/flat/CAApHDvpqBR7u9yzW4yggjG%3DQfN%3DFZsc8Wo2ckokpQtif-%2BiQ2A%40mail.gmail.com#2d900bfe18fce17f97ec1f00800c8e27
> https://www.postgresql.org/message-id/flat/MN2PR18MB2927F7B5F690065E1194B258E35D0%40MN2PR18MB2927.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] 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] https://www.postgresql.org/message-id/flat/CAHut%2BPuv4LaQKVQSErtV_%3D3MezUdpipVOMt7tJ3fXHxt_YK-Zw%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

  3. Re: shadow variables - pg15 edition

    Michael Paquier <michael@paquier.xyz> — 2022-08-18T00:39:02Z

    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

  4. Re: shadow variables - pg15 edition

    Justin Pryzby <pryzby@telsasoft.com> — 2022-08-18T02:36:26Z

    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.

  5. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-08-18T03:17:33Z

    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

  6. Re: shadow variables - pg15 edition

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-18T03:42:27Z

    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

  7. Re: shadow variables - pg15 edition

    Justin Pryzby <pryzby@telsasoft.com> — 2022-08-18T05:16:37Z

    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

  8. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-08-18T07:27:09Z

    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

  9. Re: shadow variables - pg15 edition

    Peter Smith <smithpb2250@gmail.com> — 2022-08-18T08:26:33Z

    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

  10. Re: shadow variables - pg15 edition

    Justin Pryzby <pryzby@telsasoft.com> — 2022-08-18T23:21:41Z

    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

  11. Re: shadow variables - pg15 edition

    Peter Smith <smithpb2250@gmail.com> — 2022-08-19T00:49:25Z

    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

  12. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-08-19T03:37:52Z

    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

  13. Re: shadow variables - pg15 edition

    Justin Pryzby <pryzby@telsasoft.com> — 2022-08-19T04:28:16Z

    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

  14. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-08-20T09:17:41Z

    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

  15. Re: shadow variables - pg15 edition

    Justin Pryzby <pryzby@telsasoft.com> — 2022-08-23T01:16:59Z

    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

  16. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-08-23T01:38:40Z

    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

  17. Re: shadow variables - pg15 edition

    Justin Pryzby <pryzby@telsasoft.com> — 2022-08-23T02:14:12Z

    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

  18. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-08-24T00:37:29Z

    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

  19. Re: shadow variables - pg15 edition

    Justin Pryzby <pryzby@telsasoft.com> — 2022-08-24T02:39:44Z

    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

  20. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-08-24T10:47:31Z

    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

  21. Re: shadow variables - pg15 edition

    Justin Pryzby <pryzby@telsasoft.com> — 2022-08-24T14:00:27Z

    On Wed, Aug 24, 2022 at 10:47:31PM +1200, David Rowley wrote:
> 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.

This fixed pg_get_statisticsobj_worker() but not pg_get_indexdef_worker() nor
pg_get_partkeydef_worker().

(Also, I'd mentioned that my fixes for those deliberately re-used the
outer-scope vars, which isn't what you did, and it's why I didn't include them
with the patch for inner-scope).

> 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.

Yes, the hope is to limit the change to variables that are only used a couple
times within a few lines.  It's also possible that these will break patches in
development, but that's normal for any change at all.

> 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.

I reported this last week.
https://www.postgresql.org/message-id/20220819211824.GX26426@telsasoft.com

-- 
Justin

  22. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-08-24T22:51:41Z

    On Thu, 25 Aug 2022 at 02:00, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Wed, Aug 24, 2022 at 10:47:31PM +1200, David Rowley wrote:
> > 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.
>
> This fixed pg_get_statisticsobj_worker() but not pg_get_indexdef_worker() nor
> pg_get_partkeydef_worker().

The latter two can't be fixed in the same way as
pg_get_statisticsobj_worker(), which is why I left them alone.  We can
deal with those when getting onto the next category of warnings, which
I believe should be the "Repurpose"  category.  If you look at the
shadow_analysis spreadsheet then you can see how I've categorised
each.  I'm not pretending those are all 100% accurate. Various cases
the choice of category was subjective.  My aim here is to fix as many
of the warnings as possible in the safest way possible for the
particular warning. This is why pg_get_statisticsobj_worker() wasn't
fixed in the same pass as pg_get_indexdef_worker() and
pg_get_partkeydef_worker().

David

  23. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-08-25T01:46:11Z

    On Wed, 24 Aug 2022 at 22:47, David Rowley <dgrowleyml@gmail.com> wrote:
> 5. "Refactor" (fix the code to make it better)

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

I've attached a patch which I think improves the code in
gistRelocateBuildBuffersOnSplit() so that there's no longer a shadowed
variable. I also benchmarked this method in a tight loop and can
measure no performance change from getting the loop index this way vs
the old way.

This only fixes one of the #5s I mentioned. I ended up scraping my
idea to fix the shadowed 'i' in get_qual_for_range() as it became too
complex.  The idea was to use list_cell_number() to find out how far
we looped in the forboth() loop.  It turned out that 'i' was used in
the subsequent loop in "j = i;". The fix just became too complex and I
didn't think it was worth the risk of breaking something just to get
rid of the showed 'i'.

David

  24. Re: shadow variables - pg15 edition

    Justin Pryzby <pryzby@telsasoft.com> — 2022-08-25T02:08:39Z

    On Wed, Aug 24, 2022 at 10:47:31PM +1200, David Rowley wrote:
> 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.

> 4. "Repurpose" (variables have the same purpose and may as well use
> the same variable)

> Would you be able to write a patch for #4.

The first of the patches that I sent yesterday was all about "repurposed" vars
from outer scope (lc, l, isnull, save_errno), and was 70% of your list of vars
to repurpose.

Here, I've included the rest of your list.

Plus another patch for vars which I'd already written patches to repurpose, but
which aren't classified as "repurpose" on your list.

For subselect.c, you could remove some more "lc" vars and re-use the "l" var
for consistency (but I suppose you won't want that).

-- 
Justin

  25. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-08-25T14:55:42Z

    On Thu, 25 Aug 2022 at 14:08, Justin Pryzby <pryzby@telsasoft.com> wrote:
> Here, I've included the rest of your list.

OK, I've gone through v3-remove-var-declarations.txt, v4-reuse.txt
v4-reuse-more.txt and committed most of what you had and removed a few
that I thought should be renames instead.

I also added some additional ones after reprocessing the RenameOrScope
category from the spreadsheet.

With some minor adjustments to a small number of your ones, I pushed
what I came up with.

David

  26. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-08-25T14:56:15Z

    On Thu, 25 Aug 2022 at 13:46, David Rowley <dgrowleyml@gmail.com> wrote:
> I've attached a patch which I think improves the code in
> gistRelocateBuildBuffersOnSplit() so that there's no longer a shadowed
> variable. I also benchmarked this method in a tight loop and can
> measure no performance change from getting the loop index this way vs
> the old way.

I've now pushed this patch too.

David

  27. Re: shadow variables - pg15 edition

    Justin Pryzby <pryzby@telsasoft.com> — 2022-08-30T05:44:41Z

    On Wed, Aug 24, 2022 at 10:47:31PM +1200, David Rowley wrote:
> 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.

The most *likely* outcome of renaming the *outer* variable is that
*every* cherry-pick involving that variable would fails to compile,
which is an *obvious* failure (good) but also kind of annoying if it
could've worked fine if it weren't renamed.  I think most of the renames
should be applied to the inner var, because it's of narrower scope, and
more likely to cause a conflict (good) rather than appearing to apply
cleanly but then misbehave.  But it seems reasonable to consider
renaming both if the inner scope is longer than a handful of lines.

> 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.

I'm afraid the discussion about default flags would distract from fixing
the individual warnings, which itself preclude usability of the flag by
individual developers, or buildfarm, even as a local setting.

It can't be enabled until *all* the shadows are gone, due to -Werror on
the buildfarm and cirrusci.  Unless perhaps we used -Wno-error=shadow.
I suppose we're only talking about enabling it for gcc?

The biggest benefit is if we fix *all* the local shadow vars, since that
allows someone to make use of the option, and thereby avoiding future
such issues.  Enabling the option could conceivably avoid issues
cherry-picking into back branch - if an inner var is re-introduced
during conflict resolution, then a new warning would be issued, and
hopefully the developer would look more closely.

Would you check if any of these changes are good enough ?

-- 
Justin

  28. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-10-04T01:27:09Z

    On Tue, 30 Aug 2022 at 17:44, Justin Pryzby <pryzby@telsasoft.com> wrote:
> Would you check if any of these changes are good enough ?

I looked through v5.txt and modified it so that the fix for the shadow
warnings are more aligned to the spreadsheet I created.

I also fixed some additional warnings which leaves just 5 warnings. Namely:

../../../src/include/utils/elog.h:317:29: warning: declaration of
‘_save_exception_stack’ shadows a previous local
../../../src/include/utils/elog.h:318:39: warning: declaration of
‘_save_context_stack’ shadows a previous local
../../../src/include/utils/elog.h:319:28: warning: declaration of
‘_local_sigjmp_buf’ shadows a previous local
../../../src/include/utils/elog.h:320:22: warning: declaration of
‘_do_rethrow’ shadows a previous local
pgbench.c:7509:40: warning: declaration of ‘now’ shadows a previous local

The first 4 of those are due to a nested PG_TRY().  The final one I
just ran out of inspiration on what to rename the variable to.

If there are no objections then I'll push this in the next day or 2.

David

  29. Re: shadow variables - pg15 edition

    Justin Pryzby <pryzby@telsasoft.com> — 2022-10-04T02:30:12Z

    On Tue, Oct 04, 2022 at 02:27:09PM +1300, David Rowley wrote:
> On Tue, 30 Aug 2022 at 17:44, Justin Pryzby <pryzby@telsasoft.com> wrote:
> > Would you check if any of these changes are good enough ?
> 
> I looked through v5.txt and modified it so that the fix for the shadow
> warnings are more aligned to the spreadsheet I created.

Thanks

> diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
> index 350039cc86..7848deeea9 100644
> --- a/src/backend/utils/adt/datetime.c
> +++ b/src/backend/utils/adt/datetime.c
> @@ -1019,17 +1019,17 @@ DecodeDateTime(char **field, int *ftype, int nf,
>  				if (ptype == DTK_JULIAN)
>  				{
>  					char	   *cp;
> -					int			val;
> +					int			jday;
>  
>  					if (tzp == NULL)
>  						return DTERR_BAD_FORMAT;
>  
>  					errno = 0;
> -					val = strtoint(field[i], &cp, 10);
> +					jday = strtoint(field[i], &cp, 10);
>  					if (errno == ERANGE || val < 0)
>  						return DTERR_FIELD_OVERFLOW;

Here, you forgot to change "val < 0".

I tried to see how to make that fail (differently) but can't see yet how
pass a negative julian date..

-- 
Justin

  30. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-10-05T08:05:07Z

    On Tue, 4 Oct 2022 at 15:30, Justin Pryzby <pryzby@telsasoft.com> wrote:
> Here, you forgot to change "val < 0".

Thanks. I made another review pass of each change to ensure I didn't
miss any others.  There were no other issues, so I pushed the adjusted
patch.

5 warnings remain.  4 of these are for PG_TRY() and co.

David

  31. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-10-05T10:22:33Z

    On Wed, 5 Oct 2022 at 21:05, David Rowley <dgrowleyml@gmail.com> wrote:
> 5 warnings remain.  4 of these are for PG_TRY() and co.

I've attached a draft patch for a method I was considering to fix the
warnings we're getting from the nested PG_TRY() statement in
utility.c.

The C preprocessor does not allow name overloading in macros, but of
course, it does allow variable argument marcos with ...  so I just
used that and added ##__VA_ARGS__ to each variable.  I think that
should work ok providing callers only supply 0 or 1 arguments to the
macro, and of course, make that parameter value the same for each set
of macros used in the PG_TRY() statement.

The good thing about the optional argument is that we don't need to
touch any existing users of PG_TRY().  The attached just modifies the
inner-most PG_TRY() in the only nested PG_TRY() we have in the tree in
utility.c.

The only warning remaining after applying the attached is the "now"
warning in pgbench.c:7509.  I'd considered changing this to "thenow"
which translates to "right now" in the part of Scotland that I'm from.
I also considered "nownow", which is used in South Africa [1].
Anyway, I'm not really being serious, but I didn't come up with
anything better than "now2". It's just I didn't like that as it sort
of implies there are multiple definitions of "now" and I struggle with
that...  maybe I'm just thinking too much in terms of Newtonian
Relativity...

David

[1] https://www.goodthingsguy.com/fun/now-now-just-now/

  32. Re: shadow variables - pg15 edition

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-10-05T13:34:16Z

    On 2022-Oct-05, David Rowley wrote:

> The only warning remaining after applying the attached is the "now"
> warning in pgbench.c:7509.  I'd considered changing this to "thenow"
> which translates to "right now" in the part of Scotland that I'm from.
> I also considered "nownow", which is used in South Africa [1].
> Anyway, I'm not really being serious, but I didn't come up with
> anything better than "now2". It's just I didn't like that as it sort
> of implies there are multiple definitions of "now" and I struggle with
> that...  maybe I'm just thinking too much in terms of Newtonian
> Relativity...

:-D

A simpler idea might be to just remove the inner declaration, and have
that block set the outer var.  There's no damage, since the block is
going to end and not access the previous value anymore.

diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index aa1a3541fe..91a067859b 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -7506,7 +7506,7 @@ threadRun(void *arg)
 		/* progress report is made by thread 0 for all threads */
 		if (progress && thread->tid == 0)
 		{
-			pg_time_usec_t now = pg_time_now();
+			now = pg_time_now();	/* not lazy; clobbers outer value */
 
 			if (now >= next_report)
 			{


The "now now" reference reminded me of "ahorita"
https://doorwaytomexico.com/paulina/ahorita-meaning-examples/
which is source of misunderstandings across borders in South America ...

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"The important things in the world are problems with society that we don't
understand at all. The machines will become more complicated but they won't
be more complicated than the societies that run them."    (Freeman Dyson)

  33. Re: shadow variables - pg15 edition

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-10-05T14:19:21Z

    David Rowley <dgrowleyml@gmail.com> writes:
> I've attached a draft patch for a method I was considering to fix the
> warnings we're getting from the nested PG_TRY() statement in
> utility.c.

+1

> The only warning remaining after applying the attached is the "now"
> warning in pgbench.c:7509.  I'd considered changing this to "thenow"
> which translates to "right now" in the part of Scotland that I'm from.
> I also considered "nownow", which is used in South Africa [1].
> Anyway, I'm not really being serious, but I didn't come up with
> anything better than "now2".

Yeah, "now2" seems as reasonable as anything.

			regards, tom lane

  34. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-10-05T21:21:41Z

    On Thu, 6 Oct 2022 at 03:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > I've attached a draft patch for a method I was considering to fix the
> > warnings we're getting from the nested PG_TRY() statement in
> > utility.c.
>
> +1

Pushed.

> > The only warning remaining after applying the attached is the "now"
> > warning in pgbench.c:7509.  I'd considered changing this to "thenow"
> > which translates to "right now" in the part of Scotland that I'm from.
> > I also considered "nownow", which is used in South Africa [1].
> > Anyway, I'm not really being serious, but I didn't come up with
> > anything better than "now2".
>
> Yeah, "now2" seems as reasonable as anything.

Also pushed.  (Thanks for saving me on that one.)

David

  35. Re: shadow variables - pg15 edition

    Andres Freund <andres@anarazel.de> — 2022-10-05T21:40:52Z

    Hi,

On 2022-10-06 10:21:41 +1300, David Rowley wrote:
> Also pushed.  (Thanks for saving me on that one.)

Your commit message said the last shadowed variable. But building with
-Wshadow=compatible-local triggers a bunch of warnings for me (see trimmed at
the end).  Looks like it "only" fixed it for src/, without optional
dependencies like gssapi and python.

I think we should add -Wshadow=compatible-local to our sets of warning flags
after fixing those.


[237/1827 42  12%] Compiling C object src/interfaces/libpq/libpq.a.p/fe-secure-gssapi.c.o
../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-secure-gssapi.c: In function ‘pg_GSS_write’:
../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-secure-gssapi.c:138:41: warning: declaration of ‘ret’ shadows a previous local [-Wshadow=compatible-local]
  138 |                         ssize_t         ret;
      |                                         ^~~
../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-secure-gssapi.c:92:25: note: shadowed declaration is here
   92 |         ssize_t         ret = -1;
      |                         ^~~


[1283/1827 42  70%] Compiling C object src/pl/plpython/plpython3.so.p/plpy_cursorobject.c.o
In file included from ../../../../home/andres/src/postgresql/src/include/postgres.h:48,
                 from ../../../../home/andres/src/postgresql/src/pl/plpython/plpy_cursorobject.c:7:
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_cursorobject.c: In function ‘PLy_cursor_plan’:
../../../../home/andres/src/postgresql/src/include/utils/elog.h:325:29: warning: declaration of ‘_save_exception_stack’ shadows a previous local [-Wshadow=compatible-local]
  325 |                 sigjmp_buf *_save_exception_stack##__VA_ARGS__ = PG_exception_stack; \
      |                             ^~~~~~~~~~~~~~~~~~~~~
...

[1289/1827 42  70%] Compiling C object src/pl/plpython/plpython3.so.p/plpy_exec.c.o
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_exec.c: In function ‘PLy_exec_trigger’:
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_exec.c:378:46: warning: declaration of ‘tdata’ shadows a previous local [-Wshadow=compatible-local]
  378 |                                 TriggerData *tdata = (TriggerData *) fcinfo->context;
      |                                              ^~~~~
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_exec.c:310:22: note: shadowed declaration is here
  310 |         TriggerData *tdata;
      |                      ^~~~~

[1291/1827 42  70%] Compiling C object src/pl/plpython/plpython3.so.p/plpy_spi.c.o
In file included from ../../../../home/andres/src/postgresql/src/include/postgres.h:48,
                 from ../../../../home/andres/src/postgresql/src/pl/plpython/plpy_spi.c:7:
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_spi.c: In function ‘PLy_spi_execute_plan’:
../../../../home/andres/src/postgresql/src/include/utils/elog.h:325:29: warning: declaration of ‘_save_exception_stack’ shadows a previous local [-Wshadow=compatible-local]
  325 |                 sigjmp_buf *_save_exception_stack##__VA_ARGS__ = PG_exception_stack; \
      |                             ^~~~~~~~~~~~~~~~~~~~~


[1344/1827 42  73%] Compiling C object contrib/bloom/bloom.so.p/blinsert.c.o
../../../../home/andres/src/postgresql/contrib/bloom/blinsert.c: In function ‘blinsert’:
../../../../home/andres/src/postgresql/contrib/bloom/blinsert.c:235:33: warning: declaration of ‘page’ shadows a previous local [-Wshadow=compatible-local]
  235 |                 Page            page;
      |                                 ^~~~
../../../../home/andres/src/postgresql/contrib/bloom/blinsert.c:210:25: note: shadowed declaration is here
  210 |         Page            page,
      |                         ^~~~

[1415/1827 42  77%] Compiling C object contrib/file_fdw/file_fdw.so.p/file_fdw.c.o
../../../../home/andres/src/postgresql/contrib/file_fdw/file_fdw.c: In function ‘get_file_fdw_attribute_options’:
../../../../home/andres/src/postgresql/contrib/file_fdw/file_fdw.c:453:29: warning: declaration of ‘options’ shadows a previous local [-Wshadow=compatible-local]
  453 |                 List       *options;
      |                             ^~~~~~~
../../../../home/andres/src/postgresql/contrib/file_fdw/file_fdw.c:443:21: note: shadowed declaration is here
  443 |         List       *options = NIL;
      |                     ^~~~~~~

[1441/1827 42  78%] Compiling C object contrib/hstore/hstore.so.p/hstore_io.c.o
In file included from ../../../../home/andres/src/postgresql/contrib/hstore/hstore_io.c:12:
../../../../home/andres/src/postgresql/contrib/hstore/hstore_io.c: In function ‘hstorePairs’:
../../../../home/andres/src/postgresql/contrib/hstore/hstore.h:131:21: warning: declaration of ‘buflen’ shadows a parameter [-Wshadow=compatible-local]
  131 |                 int buflen = (ptr_) - (buf_);                                                           \
      |                     ^~~~~~
../../../../home/andres/src/postgresql/contrib/hstore/hstore_io.c:411:9: note: in expansion of macro ‘HS_FINALIZE’
  411 |         HS_FINALIZE(out, pcount, buf, ptr);
      |         ^~~~~~~~~~~
../../../../home/andres/src/postgresql/contrib/hstore/hstore_io.c:388:47: note: shadowed declaration is here
  388 | hstorePairs(Pairs *pairs, int32 pcount, int32 buflen)
      |                                         ~~~~~~^~~~~~

[1564/1827 42  85%] Compiling C object contrib/postgres_fdw/postgres_fdw.so.p/deparse.c.o
../../../../home/andres/src/postgresql/contrib/postgres_fdw/deparse.c: In function ‘foreign_expr_walker’:
../../../../home/andres/src/postgresql/contrib/postgres_fdw/deparse.c:946:53: warning: declaration of ‘lc’ shadows a previous local [-Wshadow=compatible-local]
  946 |                                         ListCell   *lc;
      |                                                     ^~
../../../../home/andres/src/postgresql/contrib/postgres_fdw/deparse.c:904:45: note: shadowed declaration is here
  904 |                                 ListCell   *lc;
      |                                             ^~

[1575/1827 38  86%] Compiling C object src/test/modules/test_integerset/test_integerset.so.p/test_integerset.c.o
../../../../home/andres/src/postgresql/src/test/modules/test_integerset/test_integerset.c: In function ‘test_huge_distances’:
../../../../home/andres/src/postgresql/src/test/modules/test_integerset/test_integerset.c:588:33: warning: declaration of ‘x’ shadows a previous local [-Wshadow=compatible-local]
  588 |                 uint64          x = values[i];
      |                                 ^
../../../../home/andres/src/postgresql/src/test/modules/test_integerset/test_integerset.c:526:25: note: shadowed declaration is here
  526 |         uint64          x;
      |                         ^

[1633/1827 3  89%] Compiling C object contrib/postgres_fdw/postgres_fdw.so.p/postgres_fdw.c.o
../../../../home/andres/src/postgresql/contrib/postgres_fdw/postgres_fdw.c: In function ‘postgresGetForeignPlan’:
../../../../home/andres/src/postgresql/contrib/postgres_fdw/postgres_fdw.c:1344:37: warning: declaration of ‘lc’ shadows a previous local [-Wshadow=compatible-local]
 1344 |                         ListCell   *lc;
      |                                     ^~
../../../../home/andres/src/postgresql/contrib/postgres_fdw/postgres_fdw.c:1238:21: note: shadowed declaration is here
 1238 |         ListCell   *lc;
      |                     ^~


Greetings,

Andres Freund

  36. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-10-05T22:46:28Z

    On Thu, 6 Oct 2022 at 02:34, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> A simpler idea might be to just remove the inner declaration, and have
> that block set the outer var.  There's no damage, since the block is
> going to end and not access the previous value anymore.
>
> diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
> index aa1a3541fe..91a067859b 100644
> --- a/src/bin/pgbench/pgbench.c
> +++ b/src/bin/pgbench/pgbench.c
> @@ -7506,7 +7506,7 @@ threadRun(void *arg)
>                 /* progress report is made by thread 0 for all threads */
>                 if (progress && thread->tid == 0)
>                 {
> -                       pg_time_usec_t now = pg_time_now();
> +                       now = pg_time_now();    /* not lazy; clobbers outer value */

I didn't want to do it that way because all this code is in a while
loop and the outer "now" will be reused after it's set by the code
above.  It's not really immediately obvious to me what repercussions
that would have, but it didn't seem worth taking any risks.

David

  37. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-10-05T22:50:27Z

    On Thu, 6 Oct 2022 at 10:40, Andres Freund <andres@anarazel.de> wrote:
> Your commit message said the last shadowed variable. But building with
> -Wshadow=compatible-local triggers a bunch of warnings for me (see trimmed at
> the end).  Looks like it "only" fixed it for src/, without optional
> dependencies like gssapi and python.

Well, that's embarrassing. You're right. I only fixed the ones I saw
from running make in the base directory of the tree.  I'll set about
fixing these nownow.

David

  38. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-10-06T00:00:41Z

    On Thu, 6 Oct 2022 at 11:50, David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Thu, 6 Oct 2022 at 10:40, Andres Freund <andres@anarazel.de> wrote:
> > Your commit message said the last shadowed variable. But building with
> > -Wshadow=compatible-local triggers a bunch of warnings for me (see trimmed at
> > the end).  Looks like it "only" fixed it for src/, without optional
> > dependencies like gssapi and python.
>
> Well, that's embarrassing. You're right. I only fixed the ones I saw
> from running make in the base directory of the tree.  I'll set about
> fixing these nownow.

Here's a patch which (I think) fixes the ones I missed.

David

  39. Re: shadow variables - pg15 edition

    Andres Freund <andres@anarazel.de> — 2022-10-06T00:39:20Z

    Hi,

On 2022-10-06 13:00:41 +1300, David Rowley wrote:
> Here's a patch which (I think) fixes the ones I missed.

Yep, does the trick for me.

I attached a patch to add -Wshadow=compatible-local to our set of warnings.


> diff --git a/contrib/hstore/hstore.h b/contrib/hstore/hstore.h
> index 4713e6ea7a..897af244a4 100644
> --- a/contrib/hstore/hstore.h
> +++ b/contrib/hstore/hstore.h
> @@ -128,15 +128,15 @@ typedef struct
>  /* finalize a newly-constructed hstore */
>  #define HS_FINALIZE(hsp_,count_,buf_,ptr_)							\
>  	do {															\
> -		int buflen = (ptr_) - (buf_);								\
> +		int _buflen = (ptr_) - (buf_);								\

Not pretty. Given that HS_FINALIZE already has multiple-eval hazards, perhaps
we could just remove the local?



> --- a/src/interfaces/libpq/fe-secure-gssapi.c
> +++ b/src/interfaces/libpq/fe-secure-gssapi.c
> @@ -135,11 +135,11 @@ pg_GSS_write(PGconn *conn, const void *ptr, size_t len)
>  		 */
>  		if (PqGSSSendLength)
>  		{
> -			ssize_t		ret;
> +			ssize_t		retval;

That looks like it could easily lead to confusion further down the
line. Wouldn't the better fix here be to remove the inner variable?


> --- a/src/pl/plpython/plpy_exec.c
> +++ b/src/pl/plpython/plpy_exec.c
> @@ -375,11 +375,11 @@ PLy_exec_trigger(FunctionCallInfo fcinfo, PLyProcedure *proc)
>  				rv = NULL;
>  			else if (pg_strcasecmp(srv, "MODIFY") == 0)
>  			{
> -				TriggerData *tdata = (TriggerData *) fcinfo->context;
> +				TriggerData *trigdata = (TriggerData *) fcinfo->context;
>  
> -				if (TRIGGER_FIRED_BY_INSERT(tdata->tg_event) ||
> -					TRIGGER_FIRED_BY_UPDATE(tdata->tg_event))
> -					rv = PLy_modify_tuple(proc, plargs, tdata, rv);
> +				if (TRIGGER_FIRED_BY_INSERT(trigdata->tg_event) ||
> +					TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event))
> +					rv = PLy_modify_tuple(proc, plargs, trigdata, rv);
>  				else
>  					ereport(WARNING,
>  							(errmsg("PL/Python trigger function returned \"MODIFY\" in a DELETE trigger -- ignored")));

This doesn't strike me as a good fix either. Isn't the inner tdata exactly
the same as the outer tdata?

	tdata = (TriggerData *) fcinfo->context;
...
				TriggerData *trigdata = (TriggerData *) fcinfo->context;



> --- a/src/test/modules/test_integerset/test_integerset.c
> +++ b/src/test/modules/test_integerset/test_integerset.c
> @@ -585,26 +585,26 @@ test_huge_distances(void)

This is one of the cases where our insistence on -Wdeclaration-after-statement
really makes this unnecessary ugly... Declaring x at the start of the function
just makes this harder to read.

Anyway, this isn't important code, and your fix seem ok.


Greetings,

Andres Freund

  40. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-10-06T03:32:25Z

    On Thu, 6 Oct 2022 at 13:39, Andres Freund <andres@anarazel.de> wrote:
> I attached a patch to add -Wshadow=compatible-local to our set of warnings.

Thanks for writing that and for looking at the patch.

FWIW, I'm +1 for having this part of our default compilation flags.  I
don't want to have to revisit this on a yearly basis. I imagine Justin
doesn't want to do that either.  I feel that since this work has
already uncovered 2 existing bugs that it's worth having this as a
default compilation flag.  Additionally, in the cases like in the
PLy_exec_trigger() trigger case below, I feel this has resulted in
slightly more simple code that's easier to follow. I feel having to be
slightly more inventive with variable names in a small number of cases
is worth the trouble.  I feel the cases where this could get annoying
are probably limited to variables declared in macros. Maybe that's
just a reason to consider static inline functions instead.  That
wouldn't work for macros such as PG_TRY(), but I think macros in that
category are rare.  I think switching it on does not mean we can never
switch it off again should we ever find something we're unable to work
around.  That just seems a little unlikely given that with the prior
commits plus the attached patch, we've managed to fix ~30 years worth
of opportunity to introduce shadowed local variables.

> > diff --git a/contrib/hstore/hstore.h b/contrib/hstore/hstore.h
> >  #define HS_FINALIZE(hsp_,count_,buf_,ptr_)                                                   \
> >       do {                                                                                                                    \
> > -             int buflen = (ptr_) - (buf_);                                                           \
> > +             int _buflen = (ptr_) - (buf_);                                                          \
>
> Not pretty. Given that HS_FINALIZE already has multiple-eval hazards, perhaps
> we could just remove the local?

You're right. It's not that pretty, but I don't feel like making the
hazards any worse is a good idea. This is old code. I'd rather change
it as little as possible to minimise the risk of introducing any bugs.
I'm open to other names for the variable, but I just don't want to
widen the scope for multiple evaluation hazards.

> > --- a/src/interfaces/libpq/fe-secure-gssapi.c
> > +++ b/src/interfaces/libpq/fe-secure-gssapi.c
> > @@ -135,11 +135,11 @@ pg_GSS_write(PGconn *conn, const void *ptr, size_t len)
> > -                     ssize_t         ret;
> > +                     ssize_t         retval;
>
> That looks like it could easily lead to confusion further down the
> line. Wouldn't the better fix here be to remove the inner variable?

hmm. You're maybe able to see something I can't there, but to me, it
looks like reusing the outer variable could change the behaviour of
the function.  Note at the end of the function we set "ret" just
before the goto label.  It looks like it might be possible for the
goto to jump to the point after "ret = bytes_sent;", in which case we
should return -1, the default value for the outer "ret". If I go and
reuse the outer "ret" for something else then it'll return whatever
value it's left set to.  I could study the code more and perhaps work
out that that cannot happen, but if it can't then it's really not
obvious to me and if it's not obvious then I just don't feel the need
to take any undue risks by reusing the outer variable.  I'm open to
better names, but I'd just rather not reuse the outer scoped variable.

> > --- a/src/pl/plpython/plpy_exec.c
> > +++ b/src/pl/plpython/plpy_exec.c
> > @@ -375,11 +375,11 @@ PLy_exec_trigger(FunctionCallInfo fcinfo, PLyProcedure *proc)
> > -                             TriggerData *tdata = (TriggerData *) fcinfo->context;
> > +                             TriggerData *trigdata = (TriggerData *) fcinfo->context;

> This doesn't strike me as a good fix either. Isn't the inner tdata exactly
> the same as the outer data?

Yeah, you're right. I've adjusted the patch to use the outer scoped
variable and get rid of the inner scoped one.

> > --- a/src/test/modules/test_integerset/test_integerset.c
> > +++ b/src/test/modules/test_integerset/test_integerset.c
> > @@ -585,26 +585,26 @@ test_huge_distances(void)
>
> This is one of the cases where our insistence on -Wdeclaration-after-statement
> really makes this unnecessary ugly... Declaring x at the start of the function
> just makes this harder to read.

Yeah, it's not pretty. Maybe one day we'll relax that rule. Until
then, I think it's not worth expending too much thought on a test
module.

David

  41. Re: shadow variables - pg15 edition

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-10-06T07:32:36Z

    On 2022-Oct-06, David Rowley wrote:

> On Thu, 6 Oct 2022 at 02:34, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > A simpler idea might be to just remove the inner declaration, and have
> > that block set the outer var.  There's no damage, since the block is
> > going to end and not access the previous value anymore.
> >
> > diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
> > index aa1a3541fe..91a067859b 100644
> > --- a/src/bin/pgbench/pgbench.c
> > +++ b/src/bin/pgbench/pgbench.c
> > @@ -7506,7 +7506,7 @@ threadRun(void *arg)
> >                 /* progress report is made by thread 0 for all threads */
> >                 if (progress && thread->tid == 0)
> >                 {
> > -                       pg_time_usec_t now = pg_time_now();
> > +                       now = pg_time_now();    /* not lazy; clobbers outer value */
> 
> I didn't want to do it that way because all this code is in a while
> loop and the outer "now" will be reused after it's set by the code
> above.  It's not really immediately obvious to me what repercussions
> that would have, but it didn't seem worth taking any risks.

No, it's re-initialized to zero every time through the loop, so setting
it to something else at the bottom doesn't have any further effect.

If it were *not* reinitialized every time through the loop, then what
would happen is that every iteration in the loop (and each operation
within) would see exactly the same value of "now", because it's only set
"lazily" (meaning, if already set, don't change it.)

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

  42. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-10-06T20:13:31Z

    On Thu, 6 Oct 2022 at 20:32, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2022-Oct-06, David Rowley wrote:
> > I didn't want to do it that way because all this code is in a while
> > loop and the outer "now" will be reused after it's set by the code
> > above.  It's not really immediately obvious to me what repercussions
> > that would have, but it didn't seem worth taking any risks.
>
> No, it's re-initialized to zero every time through the loop, so setting
> it to something else at the bottom doesn't have any further effect.

Oh yeah, you're right.

> If it were *not* reinitialized every time through the loop, then what
> would happen is that every iteration in the loop (and each operation
> within) would see exactly the same value of "now", because it's only set
> "lazily" (meaning, if already set, don't change it.)

On my misread, that's what I was afraid of changing, but now seeing
that now = 0 at the start of each loop, I understand that
pg_time_now_lazy will get an up-to-date time on each loop.

I'm happy if you want to change it to use the outer scoped variable
instead of the now2 one.

David

  43. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-10-07T00:24:03Z

    On Thu, 6 Oct 2022 at 13:39, Andres Freund <andres@anarazel.de> wrote:
> I attached a patch to add -Wshadow=compatible-local to our set of warnings.

Since I just committed the patch to fix the final warnings, I think we
should go ahead and commit the patch you wrote to add
-Wshadow=compatible-local to the standard build flags. I don't mind
doing this.

Does anyone think we shouldn't do it? Please let it be known soon.

David

  44. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-10-07T03:51:55Z

    On Fri, 7 Oct 2022 at 13:24, David Rowley <dgrowleyml@gmail.com> wrote:
> Since I just committed the patch to fix the final warnings, I think we
> should go ahead and commit the patch you wrote to add
> -Wshadow=compatible-local to the standard build flags. I don't mind
> doing this.

Pushed.

David

  45. Re: shadow variables - pg15 edition

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-10-10T16:06:22Z

    David Rowley <dgrowleyml@gmail.com> writes:
> On Fri, 7 Oct 2022 at 13:24, David Rowley <dgrowleyml@gmail.com> wrote:
>> Since I just committed the patch to fix the final warnings, I think we
>> should go ahead and commit the patch you wrote to add
>> -Wshadow=compatible-local to the standard build flags. I don't mind
>> doing this.

> Pushed.

The buildfarm's showing a few instances of this warning, which seem
to indicate that not all versions of the Perl headers are clean:

 fairywren     | 2022-10-10 09:03:50 | C:/Perl64/lib/CORE/cop.h:612:13: warning: declaration of 'av' shadows a previous local [-Wshadow=compatible-local]
 fairywren     | 2022-10-10 09:03:50 | C:/Perl64/lib/CORE/cop.h:612:13: warning: declaration of 'av' shadows a previous local [-Wshadow=compatible-local]
 fairywren     | 2022-10-10 09:03:50 | C:/Perl64/lib/CORE/cop.h:612:13: warning: declaration of 'av' shadows a previous local [-Wshadow=compatible-local]
 fairywren     | 2022-10-10 09:03:50 | C:/Perl64/lib/CORE/cop.h:612:13: warning: declaration of 'av' shadows a previous local [-Wshadow=compatible-local]
 fairywren     | 2022-10-10 09:03:50 | C:/Perl64/lib/CORE/cop.h:612:13: warning: declaration of 'av' shadows a previous local [-Wshadow=compatible-local]
 fairywren     | 2022-10-10 09:03:50 | C:/Perl64/lib/CORE/cop.h:612:13: warning: declaration of 'av' shadows a previous local [-Wshadow=compatible-local]
 snakefly      | 2022-10-10 08:21:05 | Util.c:457:14: warning: declaration of 'cv' shadows a parameter [-Wshadow=compatible-local]

Before you ask:

fairywren: perl 5.24.3
snakefly: perl 5.16.3

which are a little old, but not *that* old.

Scraping the configure logs also shows that only half of the buildfarm
(exactly 50 out of 100 reporting animals) knows -Wshadow=compatible-local,
which suggests that we might see more of these if they all did.  On the
other hand, animals with newer compilers probably also have newer Perl
installations, so assuming that the Perl crew have kept this clean
recently, maybe not.

Not sure if this is problematic enough to justify removing the switch.
A plausible alternative is to have a few animals with known-clean Perl
installations add the switch manually (and use -Werror), so that we find
out about violations without having warnings in the face of developers
who can't fix them.  I'm willing to wait to see if anyone complains of
such warnings, though.

			regards, tom lane

  46. Re: shadow variables - pg15 edition

    Andres Freund <andres@anarazel.de> — 2022-10-10T16:27:32Z

    Hi,

On 2022-10-10 12:06:22 -0400, Tom Lane wrote:
> Scraping the configure logs also shows that only half of the buildfarm
> (exactly 50 out of 100 reporting animals) knows -Wshadow=compatible-local,
> which suggests that we might see more of these if they all did.

I think it's not just newness - only gcc has compatible-local, even very new
clang doesn't.


This was fixed ~6 years ago in perl:

commit f2b9631d5d19d2b71c1776e1193173d13f3620bf
Author: David Mitchell <davem@iabyn.com>
Date:   2016-05-23 14:43:56 +0100

    CX_POP_SAVEARRAY(): use more distinctive var name

    Under -Wshadow, CX_POP_SAVEARRAY's local var 'av' can generate this
    warning:

        warning: declaration shadows a local variable [-Wshadow]

    So rename it to cx_pop_savearay_av to reduce the risk of a clash.

    (See http://nntp.perl.org/group/perl.perl5.porters/236444)


> Not sure if this is problematic enough to justify removing the switch.
> A plausible alternative is to have a few animals with known-clean Perl
> installations add the switch manually (and use -Werror), so that we find
> out about violations without having warnings in the face of developers
> who can't fix them.  I'm willing to wait to see if anyone complains of
> such warnings, though.

Given the age of affected perl instances I suspect there'll not be a lot of
developers affected, and the number of warnings is reasonably small too. It'd
likely hurt more developers to not see the warnings locally, given that such
shadowing often causes bugs.

Greetings,

Andres Freund

  47. Re: shadow variables - pg15 edition

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-10-10T16:33:11Z

    On 2022-Oct-10, Andres Freund wrote:

> Given the age of affected perl instances I suspect there'll not be a lot of
> developers affected, and the number of warnings is reasonably small too. It'd
> likely hurt more developers to not see the warnings locally, given that such
> shadowing often causes bugs.

Maybe we can install a filter-out in src/pl/plperl's Makefile for the
time being.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Por suerte hoy explotó el califont porque si no me habría muerto
 de aburrido"  (Papelucho)

  48. Re: shadow variables - pg15 edition

    Andres Freund <andres@anarazel.de> — 2022-10-10T16:37:38Z

    Hi,

On 2022-10-10 18:33:11 +0200, Alvaro Herrera wrote:
> On 2022-Oct-10, Andres Freund wrote:
> 
> > Given the age of affected perl instances I suspect there'll not be a lot of
> > developers affected, and the number of warnings is reasonably small too. It'd
> > likely hurt more developers to not see the warnings locally, given that such
> > shadowing often causes bugs.
> 
> Maybe we can install a filter-out in src/pl/plperl's Makefile for the
> time being.

We could, but is it really a useful thing for something fixed 6 years ago?

Greetings,

Andres Freund

  49. Re: shadow variables - pg15 edition

    Andres Freund <andres@anarazel.de> — 2022-10-10T16:45:49Z

    On 2022-10-10 09:37:38 -0700, Andres Freund wrote:
> On 2022-10-10 18:33:11 +0200, Alvaro Herrera wrote:
> > On 2022-Oct-10, Andres Freund wrote:
> > 
> > > Given the age of affected perl instances I suspect there'll not be a lot of
> > > developers affected, and the number of warnings is reasonably small too. It'd
> > > likely hurt more developers to not see the warnings locally, given that such
> > > shadowing often causes bugs.
> > 
> > Maybe we can install a filter-out in src/pl/plperl's Makefile for the
> > time being.
> 
> We could, but is it really a useful thing for something fixed 6 years ago?

As an out, a hypothetical dev could add -Wno-shadow=compatible-local to their
CFLAGS.

  50. Re: shadow variables - pg15 edition

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-10-10T16:53:58Z

    On 2022-Oct-10, Andres Freund wrote:

> On 2022-10-10 09:37:38 -0700, Andres Freund wrote:
> > On 2022-10-10 18:33:11 +0200, Alvaro Herrera wrote:
> > > On 2022-Oct-10, Andres Freund wrote:
> > > 
> > > > Given the age of affected perl instances I suspect there'll not be a lot of
> > > > developers affected, and the number of warnings is reasonably small too. It'd
> > > > likely hurt more developers to not see the warnings locally, given that such
> > > > shadowing often causes bugs.
> > > 
> > > Maybe we can install a filter-out in src/pl/plperl's Makefile for the
> > > time being.
> > 
> > We could, but is it really a useful thing for something fixed 6 years ago?

Well, for people purposefully installing using older installs of Perl
(not me, admittedly), it does seem useful, because you get the benefit
of checking shadow vars for the rest of the tree and still get no
warnings if everything is clean.

> As an out, a hypothetical dev could add -Wno-shadow=compatible-local to their
> CFLAGS.

But that disables it for the tree as a whole, which is not better.

We can remove the filter-out when we decide to move the Perl version
requirement up, say 4 years from now.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"El hombre nunca sabe de lo que es capaz hasta que lo intenta" (C. Dickens)

  51. Re: shadow variables - pg15 edition

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-10-10T17:02:47Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2022-Oct-10, Andres Freund wrote:
>> We could, but is it really a useful thing for something fixed 6 years ago?

> Well, for people purposefully installing using older installs of Perl
> (not me, admittedly), it does seem useful, because you get the benefit
> of checking shadow vars for the rest of the tree and still get no
> warnings if everything is clean.

Meh --- people purposefully using old Perls are likely using old
compilers too.  Let's wait and see if any devs actually complain.

			regards, tom lane

  52. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-10-11T00:16:50Z

    On Tue, 11 Oct 2022 at 06:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > On 2022-Oct-10, Andres Freund wrote:
> >> We could, but is it really a useful thing for something fixed 6 years ago?
>
> > Well, for people purposefully installing using older installs of Perl
> > (not me, admittedly), it does seem useful, because you get the benefit
> > of checking shadow vars for the rest of the tree and still get no
> > warnings if everything is clean.
>
> Meh --- people purposefully using old Perls are likely using old
> compilers too.  Let's wait and see if any devs actually complain.

I can't really add much here, apart from I think it would be a shame
if some 3rd party 6 year old code was to hold us back on this.

I'm also keen to wait for complaints and only if we really have to,
remove the shadow flag from being used only in the places where we
need to.

Aside from this issue, if anything I'd be keen to go a little further
with this and upgrade to -Wshadow=local. The reason being is that I
noticed that the const qualifier is not classed as "compatible" with
the equivalently named and typed variable without the const qualifier.
ISTM that there's close to as much opportunity to mix up two variables
with the same name that are const and non-const as there are two
variables with the same const qualifier. However, I'll be waiting for
the dust to settle on the current flags before thinking any more about
that.

David

  53. Re: shadow variables - pg15 edition

    Michael Paquier <michael@paquier.xyz> — 2022-10-12T01:39:49Z

    On Tue, Oct 11, 2022 at 01:16:50PM +1300, David Rowley wrote:
> Aside from this issue, if anything I'd be keen to go a little further
> with this and upgrade to -Wshadow=local. The reason being is that I
> noticed that the const qualifier is not classed as "compatible" with
> the equivalently named and typed variable without the const qualifier.
> ISTM that there's close to as much opportunity to mix up two variables
> with the same name that are const and non-const as there are two
> variables with the same const qualifier. However, I'll be waiting for
> the dust to settle on the current flags before thinking any more about
> that.

-Wshadow=compatible-local causes one extra warning in postgres.c with
-DWRITE_READ_PARSE_PLAN_TREES:
postgres.c: In function ‘pg_rewrite_query’:
postgres.c:818:37: warning: declaration of ‘query’ shadows a parameter [-Wshadow=compatible-local]
  818 |                         Query      *query = lfirst_node(Query, lc);
      |                                     ^~~~~
postgres.c:771:25: note: shadowed declaration is here
  771 | pg_rewrite_query(Query *query)
      |                  ~~~~~~~^~~~~

Something like the patch attached would deal with this one.
--
Michael

  54. Re: shadow variables - pg15 edition

    David Rowley <dgrowleyml@gmail.com> — 2022-10-12T01:50:58Z

    On Wed, 12 Oct 2022 at 14:39, Michael Paquier <michael@paquier.xyz> wrote:
> -Wshadow=compatible-local causes one extra warning in postgres.c with
> -DWRITE_READ_PARSE_PLAN_TREES:
> postgres.c: In function ‘pg_rewrite_query’:
> postgres.c:818:37: warning: declaration of ‘query’ shadows a parameter [-Wshadow=compatible-local]
>   818 |                         Query      *query = lfirst_node(Query, lc);
>       |                                     ^~~~~
> postgres.c:771:25: note: shadowed declaration is here
>   771 | pg_rewrite_query(Query *query)
>       |                  ~~~~~~~^~~~~
>
> Something like the patch attached would deal with this one.

Thanks for finding that and coming up with the patch. It looks fine to
me. Do you want to push it?

David

  55. Re: shadow variables - pg15 edition

    Michael Paquier <michael@paquier.xyz> — 2022-10-12T02:12:13Z

    On Wed, Oct 12, 2022 at 02:50:58PM +1300, David Rowley wrote:
> Thanks for finding that and coming up with the patch. It looks fine to
> me. Do you want to push it?

Thanks for double-checking.  I'll do so shortly, I just got annoyed by
that for a few days :)

Thanks for your work on this thread to be able to push the switch by
default, by the way.
--
Michael

  56. Re: shadow variables - pg15 edition

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-10-12T13:26:35Z

    On 2022-Oct-11, David Rowley wrote:

> I'm also keen to wait for complaints and only if we really have to,
> remove the shadow flag from being used only in the places where we
> need to.

+1

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"The problem with the future is that it keeps turning into the present"
(Hobbes)