Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix new-to-v19 -Wshadow warnings

  2. Reduce scope of for-loop-local variables to avoid shadowing

  3. Fix local-variable shadowing in pg_trgm's printSourceNFA().

  1. Cleanup shadows variable warnings, round 1

    Chao Li <li.evan.chao@gmail.com> — 2025-11-28T08:16:59Z

    Hi Hackers,

While reviewing [1], it makes me recall an experience where I had a patch
ready locally, but CommitFest CI failed with a shadows-variable warning.
Now I understand that -Wall doesn't by default enable -Wshadows with some
compilers like clang.

I did a clean build with manually enabling -Wshadow and surprisingly found
there are a lot of such warnings in the current code base, roughly ~200
occurrences.

As there are too many, I plan to fix them all in 3-4 rounds. For round 1
patch, I'd see any objection, then decide if to proceed more rounds.

[1] https://postgr.es/m/CAHut+PsF8R0Bt4J3c92+T2F0mun0rRfK=-
GH+iBv2s-O8ahJJw@mail.gmail.com

Best regards,
Chao Li (Evan)
---------------------
HighGo Software Co., Ltd.
https://www.highgo.com/

  2. Re: Cleanup shadows variable warnings, round 1

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-11-28T09:11:04Z

    On 28/11/2025 10:16, Chao Li wrote:
> Hi Hackers,
> 
> While reviewing [1], it makes me recall an experience where I had a 
> patch ready locally, but CommitFest CI failed with a shadows-variable 
> warning. Now I understand that -Wall doesn't by default enable -Wshadows 
> with some compilers like clang.
> 
> I did a clean build with manually enabling -Wshadow and 
> surprisingly found there are a lot of such warnings in the current code 
> base, roughly ~200 occurrences.
> 
> As there are too many, I plan to fix them all in 3-4 rounds. For round 1 
> patch, I'd see any objection, then decide if to proceed more rounds.

I don't know if we've agreed on a goal of getting rid of all shadowing, 
it's a lot of code churn. I agree shadowing is often confusing and 
error-prone, so maybe it's worth it.

On the patch itself:

In some of the cases, I think we should rename the global / outer-scoped 
variable instead of the local variable. For example, it's pretty insane 
that we apparently have a global variable called 'days'. :-)

Let's think a little harder about the new names. For example:

> @@ -1274,8 +1274,8 @@ execute_extension_script(Oid extensionOid, ExtensionControlFile *control,
>  			Datum		old = t_sql;
>  			char	   *reqextname = (char *) lfirst(lc);
>  			Oid			reqschema = lfirst_oid(lc2);
> -			char	   *schemaName = get_namespace_name(reqschema);
> -			const char *qSchemaName = quote_identifier(schemaName);
> +			char	   *schemaname = get_namespace_name(reqschema);
> +			const char *qSchemaName = quote_identifier(schemaname);
>  			char	   *repltoken;
>  
>  			repltoken = psprintf("@extschema:%s@", reqextname);

Having two local variables that only differ in case is also confusing. 
We're using the 'req*' prefix here for the other variables, so I think 
e.g. 'reqSchemaName' would be a good name here.

(I didn't go through the whole patch, these were just a few things that 
caught my eye at quick glance)

- Heikki

  3. Re: Cleanup shadows variable warnings, round 1

    Chao Li <li.evan.chao@gmail.com> — 2025-11-28T13:39:33Z

    
> On Nov 28, 2025, at 16:16, Chao Li <li.evan.chao@gmail.com> wrote:
> 
> Hi Hackers,
> 
> While reviewing [1], it makes me recall an experience where I had a patch ready locally, but CommitFest CI failed with a shadows-variable warning. Now I understand that -Wall doesn't by default enable -Wshadows with some compilers like clang.
> 
> I did a clean build with manually enabling -Wshadow and surprisingly found there are a lot of such warnings in the current code base, roughly ~200 occurrences.
> 
> As there are too many, I plan to fix them all in 3-4 rounds. For round 1 patch, I'd see any objection, then decide if to proceed more rounds.
> 
> [1] https://postgr.es/m/CAHut+PsF8R0Bt4J3c92+T2F0mun0rRfK=-GH+iBv2s-O8ahJJw@mail.gmail.com
> 

CF entry added: https://commitfest.postgresql.org/patch/6262/

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  4. Re: Cleanup shadows variable warnings, round 1

    Peter Eisentraut <peter@eisentraut.org> — 2025-11-29T15:29:47Z

    On 28.11.25 09:16, Chao Li wrote:
> Hi Hackers,
> 
> While reviewing [1], it makes me recall an experience where I had a 
> patch ready locally, but CommitFest CI failed with a shadows-variable 
> warning. Now I understand that -Wall doesn't by default enable -Wshadows 
> with some compilers like clang.
> 
> I did a clean build with manually enabling -Wshadow and 
> surprisingly found there are a lot of such warnings in the current code 
> base, roughly ~200 occurrences.
> 
> As there are too many, I plan to fix them all in 3-4 rounds. For round 1 
> patch, I'd see any objection, then decide if to proceed more rounds.

See 
<https://www.postgresql.org/message-id/flat/20220817145434.GC26426%40telsasoft.com> 
for a previous long thread on this, which led to the addition of the 
-Wshadow=compatible-local flag.

I think this is a slightly unsatisfactory state, because that is a 
gcc-specific option, and maybe you are using clang or something else. 
So maybe some further cleanup is useful, but please check the previous 
discussions.

  5. Re: Cleanup shadows variable warnings, round 1

    Chao Li <li.evan.chao@gmail.com> — 2025-12-03T02:28:36Z

    Hi Peter,

On Nov 29, 2025, at 23:29, Peter Eisentraut <peter@eisentraut.org> wrote:

On 28.11.25 09:16, Chao Li wrote:

Hi Hackers,
While reviewing [1], it makes me recall an experience where I had a patch
ready locally, but CommitFest CI failed with a shadows-variable warning.
Now I understand that -Wall doesn't by default enable -Wshadows with some
compilers like clang.
I did a clean build with manually enabling -Wshadow and surprisingly found
there are a lot of such warnings in the current code base, roughly ~200
occurrences.
As there are too many, I plan to fix them all in 3-4 rounds. For round 1
patch, I'd see any objection, then decide if to proceed more rounds.


See <
https://www.postgresql.org/message-id/flat/20220817145434.GC26426%40telsasoft.com>
for a previous long thread on this, which led to the addition of the
-Wshadow=compatible-local flag.


Thanks for pointing out the previous discussion. I have read through the
discussion. Looks like there was no objection on the direction of cleaning
up the shadow-variable warnings, folks were discussing how to do the
cleanup. I think my v1 patch has already taken the “saner” way as Micheal
suggested: renaming inter local variables.

I counted all warnings, there are totally 167 occurrences, seems a
manageable number. Instead of randomly splitting all fixes into several
commits as v1 did, which would really discourage reviewers and committers,
in v2, I tried to categorize all warnings to different types, and put fixes
of different types into different commits, which should make reviews a lot
easier. All commits are self-contained, each of them can be reviewed and
pushed independently.


I think this is a slightly unsatisfactory state, because that is a
gcc-specific option, and maybe you are using clang or something else. So
maybe some further cleanup is useful, but please check the previous
discussions.


I know -Wshadow=compatible-local is not supported by all compilers, some
compilers may fallback to -Wshadow and some may just ignore it. This patch
set has cleaned up all shadow-variable warnings, once the cleanup is done,
in theory, we should be able to enable -Wshadow.

0001 - simple cases of local variable shadowing local variable by changing
inner variable to for loop variable (also need to rename the for loop var)
0002 - simple cases of local variable shadowing local variable by renaming
 inner variable
0003 - simple cases of local variable shadowing local variable by renaming
outer variable. In this commit, outer local variables are used much less
than inner variables, thus renaming outer is simpler than renaming inner.
0004 -  still local shadows local, but caused by a macro definition, only
in inval.c
0005 - cases of global wal_level and wal_segment_size shadow local ones,
fixed by renaming local variables
0006 - in xlogrecovery.c, some static file-scope variables shadow local
variables, fixed by renaming local variables
0007 - cases of global progname shadows local, fixed by renaming local to
myprogname
0008 - in file_ops.c, some static file-scope variables shadow local, fixed
by renaming local variables
0009 - simple cases of local variables are shadowed by global, fixed by
renaming local variables
0010 - a few more cases of static file-scope variables shadow local
variables, fixed by renaming local variables
0011 - cases of global conn shadows local variables, fixed by renaming
local conn to myconn
0012 - fixed shadowing in ecpg.header
0013 - fixed shadowing in all time-related modules. Heikki had a concern
where there is a global named “days”, so there could be some discussions
for this commit. For now, I just renamed local variables to avoid shadowing.

With this patch set, building postgres with “-Wshadow” won’t get any
warning. Note, this patch set doesn’t cover contrib.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  6. Re: Cleanup shadows variable warnings, round 1

    Peter Smith <smithpb2250@gmail.com> — 2025-12-03T06:42:33Z

    Hi Chao.

I always build with the shadow warnings enabled, so I am happy someone
has taken up the challenge to try to clean them up. You probably found
an old thread where I tried to do the same very thing several years
ago/. I had fixed most of them in my local environment, but the thread
became stalled due to
(a) IIUC, there was some push-back about causing too much churn, and
(b) I didn't know how to break it into manageable chunks.

So I wish you better luck this time. I have just started to look at
your patches:

======

Patch v2-0001.

src/backend/backup/basebackup_incremental.c:
PrepareForIncrementalBackup:

Instead of changing the var name from 'i' to 'u', you can fix this one
by changing all the for loops to declare 'i' within the 'for ('.
That way kills two birds with one stone: it removes the shadow warning
and at the same time improves the scope of the loop variables.


-       int                     i;

-       for (i = 0; i < num_wal_ranges; ++i)
+       for (int i = 0; i < num_wal_ranges; ++i)

-       for (i = 0; i < num_wal_ranges; ++i)
+       for (int i = 0; i < num_wal_ranges; ++i)

-       unsigned        i;

-               for (i = 0; i < nblocks; ++i)
+               for (unsigned i = 0; i < nblocks; ++i)


======

I will continue to look at the rest of the patches as time permits.

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

  7. Re: Cleanup shadows variable warnings, round 1

    Chao Li <li.evan.chao@gmail.com> — 2025-12-03T06:52:14Z

    
> On Dec 3, 2025, at 14:42, Peter Smith <smithpb2250@gmail.com> wrote:
> 
> Patch v2-0001.
> 
> src/backend/backup/basebackup_incremental.c:
> PrepareForIncrementalBackup:
> 
> Instead of changing the var name from 'i' to 'u', you can fix this one
> by changing all the for loops to declare 'i' within the 'for ('.
> That way kills two birds with one stone: it removes the shadow warning
> and at the same time improves the scope of the loop variables.
> 
> 
> -       int                     i;
> 
> -       for (i = 0; i < num_wal_ranges; ++i)
> +       for (int i = 0; i < num_wal_ranges; ++i)
> 
> -       for (i = 0; i < num_wal_ranges; ++i)
> +       for (int i = 0; i < num_wal_ranges; ++i)
> 
> -       unsigned        i;
> 
> -               for (i = 0; i < nblocks; ++i)
> +               for (unsigned i = 0; i < nblocks; ++i)

Unfortunately that doesn’t work for my compiler (clang on MacOS), that’s why I renamed “I" to “u”.

By the way, Peter (S), thank you very much for reviewing.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  8. Re: Cleanup shadows variable warnings, round 1

    Chao Li <li.evan.chao@gmail.com> — 2025-12-03T10:56:33Z

    On Wed, Dec 3, 2025 at 10:28 AM Chao Li <li.evan.chao@gmail.com> wrote:

>
> 0001 - simple cases of local variable shadowing local variable by changing
> inner variable to for loop variable (also need to rename the for loop var)
> 0002 - simple cases of local variable shadowing local variable by renaming
>  inner variable
> 0003 - simple cases of local variable shadowing local variable by renaming
> outer variable. In this commit, outer local variables are used much less
> than inner variables, thus renaming outer is simpler than renaming inner.
> 0004 -  still local shadows local, but caused by a macro definition, only
> in inval.c
> 0005 - cases of global wal_level and wal_segment_size shadow local ones,
> fixed by renaming local variables
> 0006 - in xlogrecovery.c, some static file-scope variables shadow local
> variables, fixed by renaming local variables
> 0007 - cases of global progname shadows local, fixed by renaming local to
> myprogname
> 0008 - in file_ops.c, some static file-scope variables shadow local, fixed
> by renaming local variables
> 0009 - simple cases of local variables are shadowed by global, fixed by
> renaming local variables
> 0010 - a few more cases of static file-scope variables shadow local
> variables, fixed by renaming local variables
> 0011 - cases of global conn shadows local variables, fixed by renaming
> local conn to myconn
> 0012 - fixed shadowing in ecpg.header
> 0013 - fixed shadowing in all time-related modules. Heikki had a concern
> where there is a global named “days”, so there could be some discussions
> for this commit. For now, I just renamed local variables to avoid shadowing.
>
>
c252d37d8 fixed one shadow warning individually, which caused a conflict to
this patch, thus rebased to v3.

Best regards,
Chao Li (Evan)
---------------------
HighGo Software Co., Ltd.
https://www.highgo.com/

  9. Re: Cleanup shadows variable warnings, round 1

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-12-03T14:36:05Z

    On 2025-Dec-03, Chao Li wrote:

> Unfortunately that doesn’t work for my compiler (clang on MacOS),
> that’s why I renamed “I" to “u”.

I think you're missing his point.  He's suggesting to change not only
this variable, but also the other uses of "i" in this function.

I'd also change "unsigned" to "unsigned int", just because I dislike
using unadorned "unsigned" as a type name (I know it's a legal type
name.)  This could be left alone, I guess -- not important.

diff --git a/src/backend/backup/basebackup_incremental.c b/src/backend/backup/basebackup_incremental.c
index 852ab577045..322d8997400 100644
--- a/src/backend/backup/basebackup_incremental.c
+++ b/src/backend/backup/basebackup_incremental.c
@@ -270,7 +270,6 @@ PrepareForIncrementalBackup(IncrementalBackupInfo *ib,
 	ListCell   *lc;
 	TimeLineHistoryEntry **tlep;
 	int			num_wal_ranges;
-	int			i;
 	bool		found_backup_start_tli = false;
 	TimeLineID	earliest_wal_range_tli = 0;
 	XLogRecPtr	earliest_wal_range_start_lsn = InvalidXLogRecPtr;
@@ -312,7 +311,7 @@ PrepareForIncrementalBackup(IncrementalBackupInfo *ib,
 	 */
 	expectedTLEs = readTimeLineHistory(backup_state->starttli);
 	tlep = palloc0(num_wal_ranges * sizeof(TimeLineHistoryEntry *));
-	for (i = 0; i < num_wal_ranges; ++i)
+	for (int i = 0; i < num_wal_ranges; ++i)
 	{
 		backup_wal_range *range = list_nth(ib->manifest_wal_ranges, i);
 		bool		saw_earliest_wal_range_tli = false;
@@ -400,7 +399,7 @@ PrepareForIncrementalBackup(IncrementalBackupInfo *ib,
 	 * anything here. However, if there's a problem staring us right in the
 	 * face, it's best to report it, so we do.
 	 */
-	for (i = 0; i < num_wal_ranges; ++i)
+	for (int i = 0; i < num_wal_ranges; ++i)
 	{
 		backup_wal_range *range = list_nth(ib->manifest_wal_ranges, i);
 
@@ -595,15 +594,14 @@ PrepareForIncrementalBackup(IncrementalBackupInfo *ib,
 
 			while (1)
 			{
-				unsigned	nblocks;
-				unsigned	i;
+				unsigned int	nblocks;
 
 				nblocks = BlockRefTableReaderGetBlocks(reader, blocks,
 													   BLOCKS_PER_READ);
 				if (nblocks == 0)
 					break;
 
-				for (i = 0; i < nblocks; ++i)
+				for (unsigned int i = 0; i < nblocks; ++i)
 					BlockRefTableMarkBlockModified(ib->brtab, &rlocator,
 												   forknum, blocks[i]);
 			}

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

  10. Re: Cleanup shadows variable warnings, round 1

    Andres Freund <andres@anarazel.de> — 2025-12-03T15:17:28Z

    Hi,

On 2025-12-03 10:28:36 +0800, Chao Li wrote:
> I know -Wshadow=compatible-local is not supported by all compilers, some
> compilers may fallback to -Wshadow and some may just ignore it. This patch
> set has cleaned up all shadow-variable warnings, once the cleanup is done,
> in theory, we should be able to enable -Wshadow.
> 
> 0001 - simple cases of local variable shadowing local variable by changing
> inner variable to for loop variable (also need to rename the for loop var)
> 0002 - simple cases of local variable shadowing local variable by renaming
>  inner variable
> 0003 - simple cases of local variable shadowing local variable by renaming
> outer variable. In this commit, outer local variables are used much less
> than inner variables, thus renaming outer is simpler than renaming inner.
> 0004 -  still local shadows local, but caused by a macro definition, only
> in inval.c
> 0005 - cases of global wal_level and wal_segment_size shadow local ones,
> fixed by renaming local variables
> 0006 - in xlogrecovery.c, some static file-scope variables shadow local
> variables, fixed by renaming local variables
> 0007 - cases of global progname shadows local, fixed by renaming local to
> myprogname
> 0008 - in file_ops.c, some static file-scope variables shadow local, fixed
> by renaming local variables
> 0009 - simple cases of local variables are shadowed by global, fixed by
> renaming local variables
> 0010 - a few more cases of static file-scope variables shadow local
> variables, fixed by renaming local variables
> 0011 - cases of global conn shadows local variables, fixed by renaming
> local conn to myconn
> 0012 - fixed shadowing in ecpg.header
> 0013 - fixed shadowing in all time-related modules. Heikki had a concern
> where there is a global named “days”, so there could be some discussions
> for this commit. For now, I just renamed local variables to avoid shadowing.

This seems like a *lot* of noise / backpatching pain for relatively little
gain.



> From 0cddee282a08c79214fa72a21a548b73cc6256fe Mon Sep 17 00:00:00 2001
> From: "Chao Li (Evan)" <lic@highgo.com>
> Date: Tue, 2 Dec 2025 13:45:05 +0800
> Subject: [PATCH v2 05/13] cleanup: avoid local wal_level and wal_segment_size
>  being shadowed by globals
> 
> This commit fixes cases where local variables named wal_level and
> wal_segment_size were shadowed by global identifiers of the same names.
> The local variables are renamed so they are no longer shadowed by the
> globals.
> 
> Author: Chao Li <lic@highgo.com>
> Discussion: https://postgr.es/m/CAEoWx2kQ2x5gMaj8tHLJ3=jfC+p5YXHkJyHrDTiQw2nn2FJTmQ@mail.gmail.com
> ---
>  src/backend/access/rmgrdesc/xlogdesc.c  | 18 +++++++++---------
>  src/backend/access/transam/xlogreader.c |  4 ++--
>  src/bin/pg_controldata/pg_controldata.c |  4 ++--
>  3 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/src/backend/access/rmgrdesc/xlogdesc.c b/src/backend/access/rmgrdesc/xlogdesc.c
> index cd6c2a2f650..6241d87c647 100644
> --- a/src/backend/access/rmgrdesc/xlogdesc.c
> +++ b/src/backend/access/rmgrdesc/xlogdesc.c
> @@ -34,24 +34,24 @@ const struct config_enum_entry wal_level_options[] = {
>  };
>  
>  /*
> - * Find a string representation for wal_level
> + * Find a string representation for wallevel
>   */
>  static const char *
> -get_wal_level_string(int wal_level)
> +get_wal_level_string(int wallevel)
>  {
>  	const struct config_enum_entry *entry;
> -	const char *wal_level_str = "?";
> +	const char *wallevel_str = "?";

This sounds like it's talking about walls not, the WAL.

Greetings,

Andres Freund

  11. Re: Cleanup shadows variable warnings, round 1

    Peter Smith <smithpb2250@gmail.com> — 2025-12-03T21:00:51Z

    On Thu, Dec 4, 2025 at 1:36 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> On 2025-Dec-03, Chao Li wrote:
>
> > Unfortunately that doesn’t work for my compiler (clang on MacOS),
> > that’s why I renamed “I" to “u”.
>
> I think you're missing his point.  He's suggesting to change not only
> this variable, but also the other uses of "i" in this function.
>

Exactly. I should have posted the larger diff, as you did. Thanks for
clarifying.

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

  12. Re: Cleanup shadows variable warnings, round 1

    Chao Li <li.evan.chao@gmail.com> — 2025-12-03T23:04:02Z

    
> On Dec 4, 2025, at 05:00, Peter Smith <smithpb2250@gmail.com> wrote:
> 
> On Thu, Dec 4, 2025 at 1:36 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>> 
>> On 2025-Dec-03, Chao Li wrote:
>> 
>>> Unfortunately that doesn’t work for my compiler (clang on MacOS),
>>> that’s why I renamed “I" to “u”.
>> 
>> I think you're missing his point.  He's suggesting to change not only
>> this variable, but also the other uses of "i" in this function.
>> 
> 
> Exactly. I should have posted the larger diff, as you did. Thanks for
> clarifying.
> 

Sorry for misunderstanding you. I guess I read your message too quickly yesterday. Will fix it in v4.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  13. Re: Cleanup shadows variable warnings, round 1

    Chao Li <li.evan.chao@gmail.com> — 2025-12-03T23:24:36Z

    On Wed, Dec 3, 2025 at 10:36 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:

> On 2025-Dec-03, Chao Li wrote:
>
> > Unfortunately that doesn’t work for my compiler (clang on MacOS),
> > that’s why I renamed “I" to “u”.
>
> I think you're missing his point.  He's suggesting to change not only
> this variable, but also the other uses of "i" in this function.
>
> I'd also change "unsigned" to "unsigned int", just because I dislike
> using unadorned "unsigned" as a type name (I know it's a legal type
> name.)  This could be left alone, I guess -- not important.
>
> --
> Álvaro Herrera         PostgreSQL Developer  —
> https://www.EnterpriseDB.com/


I misunderstood Peter's message yesterday. I have addressed both comments
(changing all "for" and changing "unsigned" to "unsigned int") in v4.

On Wed, Dec 3, 2025 at 11:17 PM Andres Freund <andres@anarazel.de> wrote:

>
> On 2025-12-03 10:28:36 +0800, Chao Li wrote:
> > I know -Wshadow=compatible-local is not supported by all compilers, some
> > compilers may fallback to -Wshadow and some may just ignore it. This
> patch
> > set has cleaned up all shadow-variable warnings, once the cleanup is
> done,
> > in theory, we should be able to enable -Wshadow.
> >
> > 0001 - simple cases of local variable shadowing local variable by
> changing
> > inner variable to for loop variable (also need to rename the for loop
> var)
> > 0002 - simple cases of local variable shadowing local variable by
> renaming
> >  inner variable
> > 0003 - simple cases of local variable shadowing local variable by
> renaming
> > outer variable. In this commit, outer local variables are used much less
> > than inner variables, thus renaming outer is simpler than renaming inner.
> > 0004 -  still local shadows local, but caused by a macro definition, only
> > in inval.c
> > 0005 - cases of global wal_level and wal_segment_size shadow local ones,
> > fixed by renaming local variables
> > 0006 - in xlogrecovery.c, some static file-scope variables shadow local
> > variables, fixed by renaming local variables
> > 0007 - cases of global progname shadows local, fixed by renaming local to
> > myprogname
> > 0008 - in file_ops.c, some static file-scope variables shadow local,
> fixed
> > by renaming local variables
> > 0009 - simple cases of local variables are shadowed by global, fixed by
> > renaming local variables
> > 0010 - a few more cases of static file-scope variables shadow local
> > variables, fixed by renaming local variables
> > 0011 - cases of global conn shadows local variables, fixed by renaming
> > local conn to myconn
> > 0012 - fixed shadowing in ecpg.header
> > 0013 - fixed shadowing in all time-related modules. Heikki had a concern
> > where there is a global named “days”, so there could be some discussions
> > for this commit. For now, I just renamed local variables to avoid
> shadowing.
>
> This seems like a *lot* of noise / backpatching pain for relatively little
> gain.
>

 I agree the patch set is large, which is why I split it into smaller
commits, each independent and self-contained.

The motivation is that CF’s CI currently fails on shadow-variable warnings.
If you touch a file like a.c, and that file already has a legacy shadowing
issue, CI will still fail your patch even if your changes are correct. Then
you’re forced to fix unrelated shadow-variable problems just to get a clean
CI run. I’ve run into this myself, and it’s disruptive for both patch
authors and reviewers.

By cleaning up all existing shadow-variable warnings now, they won’t
interfere with future patches, and it also allows us to enable -Wshadow by
default so new shadowing issues won’t be introduced.


On Wed, Dec 3, 2025 at 11:17 PM Andres Freund <andres@anarazel.de> wrote:

>
> >  /*
> > - * Find a string representation for wal_level
> > + * Find a string representation for wallevel
> >   */
> >  static const char *
> > -get_wal_level_string(int wal_level)
> > +get_wal_level_string(int wallevel)
> >  {
> >       const struct config_enum_entry *entry;
> > -     const char *wal_level_str = "?";
> > +     const char *wallevel_str = "?";
>
> This sounds like it's talking about walls not, the WAL.
>
>
Fixed in v4.

V4 addressed all comments received so far, and fixed a mistake that caused
CI failure.

Best regards,

Chao Li (Evan)
---------------------
HighGo Software Co., Ltd.
https://www.highgo.com/

  14. Re: Cleanup shadows variable warnings, round 1

    Michael Paquier <michael@paquier.xyz> — 2025-12-04T00:56:07Z

    On Fri, Nov 28, 2025 at 11:11:04AM +0200, Heikki Linnakangas wrote:
> I don't know if we've agreed on a goal of getting rid of all shadowing, it's
> a lot of code churn. I agree shadowing is often confusing and error-prone,
> so maybe it's worth it.

(Providing my own context with more information on the matter, Peter
E. mentioning this commit upthread.)

As far as I know, the latest consensus with shadow variables was that
-Wshadow=compatible-local was OK for now, 0fe954c28584 mentioning that
we could consider a tighter -Wshadow=local later on.  I don't recall a
clear objection about doing a tighter move, just that it was a lot of
work for unclear gains especially when it comes to the extra
backpatching noise.
--
Michael

  15. Re: Cleanup shadows variable warnings, round 1

    Peter Smith <smithpb2250@gmail.com> — 2025-12-04T01:08:37Z

    FWIW... A few more review comments for v3.

//////////
Patch v3-0001.
//////////

======
src/backend/access/gist/gistbuild.c

2.1.
OK, but should you take it 1 step further?

BEFORE
foreach(lc, splitinfo)
{
  GISTPageSplitInfo *si = lfirst(lc);
AFTER
foreach_ptr(GISTPageSplitInfo, si, splitinfo)
{

======
src/backend/commands/schemacmds.c

2.2.
OK, but should you take it 1 step further?

BEFORE
foreach(parsetree_item, parsetree_list)
{
  Node    *substmt = (Node *) lfirst(parsetree_item);
AFTER
foreach_ptr(Node, substmt, parsetree_list)
{

======
src/backend/commands/statscmds.c

2.3.
OK, but I felt 'attnums_bms' might be a better replacement name than 'attnumsbm'

======
src/backend/executor/nodeValuesscan.c

2.4.
OK, but should you take it 1 step further?

BEFORE
foreach(lc, exprstatelist)
{
  ExprState  *exprstate = (ExprState *) lfirst(lc);
AFTER
foreach_ptr(ExprState, exprstate, exprstatelist)
{

======
src/backend/statistics/dependencies.c

2.5.
The other variable in other parts of this function had names like:
clause_expr
bool_expr
or_expr
stat_expr

So, perhaps your new names should be changed slightly to look more
similar to those?

======
src/backend/statistics/extended_stats.c

2.6.
This seems to be in the wrong patch because here you renamed the local
var, not the inner one, as the patch commit message says.

======
src/backend/utils/adt/jsonpath_exec.c

2.7.
Wondering if 'vals' might be a better name than 'foundJV' (there is
already another JsonValueList vals elsewhere in this code).

======
src/bin/pgbench/pgbench.c

2.8.
Wondering if 'nskipped' is a better name than 'skips'.

//////////
Patch v3-0003
//////////

LGTM.

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

  16. Re: Cleanup shadows variable warnings, round 1

    Chao Li <li.evan.chao@gmail.com> — 2025-12-04T09:10:42Z

    On Dec 4, 2025, at 09:08, Peter Smith <smithpb2250@gmail.com> wrote:

FWIW... A few more review comments for v3.

//////////
Patch v3-0001.
//////////


This is actually 0002.


======
src/backend/access/gist/gistbuild.c

2.1.
OK, but should you take it 1 step further?

BEFORE
foreach(lc, splitinfo)
{
 GISTPageSplitInfo *si = lfirst(lc);
AFTER
foreach_ptr(GISTPageSplitInfo, si, splitinfo)
{

======
src/backend/commands/schemacmds.c


Fixed.

2.2.
OK, but should you take it 1 step further?

BEFORE
foreach(parsetree_item, parsetree_list)
{
 Node    *substmt = (Node *) lfirst(parsetree_item);
AFTER
foreach_ptr(Node, substmt, parsetree_list)
{


Fixed.

======
src/backend/commands/statscmds.c

2.3.
OK, but I felt 'attnums_bms' might be a better replacement name than
'attnumsbm'


Fixed.

======
src/backend/executor/nodeValuesscan.c

2.4.
OK, but should you take it 1 step further?

BEFORE
foreach(lc, exprstatelist)
{
 ExprState  *exprstate = (ExprState *) lfirst(lc);
AFTER
foreach_ptr(ExprState, exprstate, exprstatelist)
{


Fixed.

======
src/backend/statistics/dependencies.c

2.5.
The other variable in other parts of this function had names like:
clause_expr
bool_expr
or_expr
stat_expr

So, perhaps your new names should be changed slightly to look more
similar to those?


Fixed.

======
src/backend/statistics/extended_stats.c

2.6.
This seems to be in the wrong patch because here you renamed the local
var, not the inner one, as the patch commit message says.


Moved to 0003.

======
src/backend/utils/adt/jsonpath_exec.c

2.7.
Wondering if 'vals' might be a better name than 'foundJV' (there is
already another JsonValueList vals elsewhere in this code).


Fixed.

======
src/bin/pgbench/pgbench.c

2.8.
Wondering if 'nskipped' is a better name than 'skips'.


The around variables all use “s”:
```
int64 skips = 0;
int64 serialization_failures = 0;
int64 deadlock_failures = 0;
int64 other_sql_failures = 0;
int64 retried = 0;
int64 retries = 0;
```

That’s why I used the “s” form.

All fixes are in v5.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  17. Re: Cleanup shadows variable warnings, round 1

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-12-04T11:21:15Z

    On 2025-Dec-04, Chao Li wrote:

> The motivation is that CF’s CI currently fails on shadow-variable warnings.
> If you touch a file like a.c, and that file already has a legacy shadowing
> issue, CI will still fail your patch even if your changes are correct. Then
> you’re forced to fix unrelated shadow-variable problems just to get a clean
> CI run. I’ve run into this myself, and it’s disruptive for both patch
> authors and reviewers.

Hmm, maybe that should be turned off.  It sounds seriously unhelpful.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"En las profundidades de nuestro inconsciente hay una obsesiva necesidad
de un universo lógico y coherente. Pero el universo real se halla siempre
un paso más allá de la lógica" (Irulan)

  18. Re: Cleanup shadows variable warnings, round 1

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2025-12-04T13:04:07Z

    Hi,

On Thu, 4 Dec 2025 at 14:21, Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> On 2025-Dec-04, Chao Li wrote:
>
> > The motivation is that CF’s CI currently fails on shadow-variable warnings.
> > If you touch a file like a.c, and that file already has a legacy shadowing
> > issue, CI will still fail your patch even if your changes are correct. Then
> > you’re forced to fix unrelated shadow-variable problems just to get a clean
> > CI run. I’ve run into this myself, and it’s disruptive for both patch
> > authors and reviewers.
>
> Hmm, maybe that should be turned off.  It sounds seriously unhelpful.

To test that I created this CI run [1], which edits the brin.c file.
That file has a legacy shadowing issue but the CI did not fail. Could
you please show an example CI run?

[1] https://cirrus-ci.com/build/5444936843132928

--
Regards,
Nazir Bilal Yavuz
Microsoft

  19. Re: Cleanup shadows variable warnings, round 1

    Chao Li <li.evan.chao@gmail.com> — 2026-03-03T09:04:47Z

    
> On Dec 4, 2025, at 17:10, Chao Li <li.evan.chao@gmail.com> wrote:
> 
> 
> 
>> On Dec 4, 2025, at 09:08, Peter Smith <smithpb2250@gmail.com> wrote:
>> 
>> FWIW... A few more review comments for v3.
>> 
>> //////////
>> Patch v3-0001.
>> //////////
> 
> This is actually 0002.
> 
>> 
>> ======
>> src/backend/access/gist/gistbuild.c
>> 
>> 2.1.
>> OK, but should you take it 1 step further?
>> 
>> BEFORE
>> foreach(lc, splitinfo)
>> {
>>  GISTPageSplitInfo *si = lfirst(lc);
>> AFTER
>> foreach_ptr(GISTPageSplitInfo, si, splitinfo)
>> {
>> 
>> ======
>> src/backend/commands/schemacmds.c
>> 
> 
> Fixed.
> 
>> 2.2.
>> OK, but should you take it 1 step further?
>> 
>> BEFORE
>> foreach(parsetree_item, parsetree_list)
>> {
>>  Node    *substmt = (Node *) lfirst(parsetree_item);
>> AFTER
>> foreach_ptr(Node, substmt, parsetree_list)
>> {
>> 
> 
> Fixed.
> 
>> ======
>> src/backend/commands/statscmds.c
>> 
>> 2.3.
>> OK, but I felt 'attnums_bms' might be a better replacement name than 'attnumsbm'
>> 
> 
> Fixed.
> 
>> ======
>> src/backend/executor/nodeValuesscan.c
>> 2.4.
>> OK, but should you take it 1 step further?
>> 
>> BEFORE
>> foreach(lc, exprstatelist)
>> {
>>  ExprState  *exprstate = (ExprState *) lfirst(lc);
>> AFTER
>> foreach_ptr(ExprState, exprstate, exprstatelist)
>> {
>> 
> 
> Fixed.
> 
>> ======
>> src/backend/statistics/dependencies.c
>> 
>> 2.5.
>> The other variable in other parts of this function had names like:
>> clause_expr
>> bool_expr
>> or_expr
>> stat_expr
>> 
>> So, perhaps your new names should be changed slightly to look more
>> similar to those?
>> 
> 
> Fixed.
> 
>> ======
>> src/backend/statistics/extended_stats.c
>> 
>> 2.6.
>> This seems to be in the wrong patch because here you renamed the local
>> var, not the inner one, as the patch commit message says.
>> 
> 
> Moved to 0003.
> 
>> ======
>> src/backend/utils/adt/jsonpath_exec.c
>> 
>> 2.7.
>> Wondering if 'vals' might be a better name than 'foundJV' (there is
>> already another JsonValueList vals elsewhere in this code).
>> 
> 
> Fixed.
> 
>> ======
>> src/bin/pgbench/pgbench.c
>> 
>> 2.8.
>> Wondering if 'nskipped' is a better name than 'skips'.
>> 
> 
> The around variables all use “s”:
> ```
> int64 skips = 0;
> int64 serialization_failures = 0;
> int64 deadlock_failures = 0;
> int64 other_sql_failures = 0;
> int64 retried = 0;
> int64 retries = 0;
> ```
> 
> That’s why I used the “s” form.
> 
> All fixes are in v5.
> 
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
> 
> 
> 
> 
> <v5-0003-cleanup-rename-outer-variables-to-avoid-shadowing.patch><v5-0004-cleanup-fix-macro-induced-variable-shadowing-in-i.patch><v5-0005-cleanup-avoid-local-wal_level-and-wal_segment_siz.patch><v5-0002-cleanup-rename-inner-variables-to-avoid-shadowing.patch><v5-0001-cleanup-rename-loop-variables-to-avoid-local-shad.patch><v5-0007-cleanup-rename-local-progname-variables-to-avoid-.patch><v5-0006-cleanup-avoid-local-variables-shadowed-by-static-.patch><v5-0010-cleanup-avoid-local-variables-shadowed-by-static-.patch><v5-0008-cleanup-avoid-local-variables-shadowed-by-static-.patch><v5-0009-cleanup-avoid-local-variables-shadowed-by-globals.patch><v5-0011-cleanup-rename-local-conn-variables-to-avoid-shad.patch><v5-0012-cleanup-avoid-local-variables-shadowed-by-globals.patch><v5-0013-cleanup-avoid-local-variables-shadowed-by-globals.patch>

Gentle ping as I saw a recent commit cdaa67565867ba443afb66b9e82023d65487dc7c that cleaned up a single occurrence in pg_trgm.

Rebased to v6.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  20. Re: Cleanup shadows variable warnings, round 1

    Álvaro Herrera <alvherre@kurilemu.de> — 2026-03-03T10:26:32Z

    Pushed 0001.

Three things about the next ones,

1. if you rename a function argument, then the function declaration
should match the new name as well.

2. xlogrecovery.c has far too many global variables.  Can we use this
opportunity to try to get rid of some of them?  Especially one called
"xlogreader" is I think quite bug-prone.

3. I disagree with some of the choices made; for instance rather than
rename the local "progname" variables in all those places, I would
rename the global to logging_progname in logging.c; in bringetbitmap
(0002) I would rename the outer "tmp" to "sizecheck" or something like
that.  I guess this is mostly matter of mostly arbitrary judgment ...

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

  21. Re: Cleanup shadows variable warnings, round 1

    Chao Li <li.evan.chao@gmail.com> — 2026-03-03T11:55:36Z

    
> On Mar 3, 2026, at 18:26, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> 
> Pushed 0001.

Hi Alvaro, thank you very much for pushing 0001. But I don’t see it. I tried to pull master and refresh https://git.postgresql.org/cgit/postgresql.git/log/?qt=grep&q=, none of them shows 0001 pushed.


> 
> Three things about the next ones,
> 
> 1. if you rename a function argument, then the function declaration
> should match the new name as well.

Sure, I will make sure that in next revision.

> 
> 2. xlogrecovery.c has far too many global variables.  Can we use this
> opportunity to try to get rid of some of them?  Especially one called
> "xlogreader" is I think quite bug-prone.

Okay, I will try to address this.

> 
> 3. I disagree with some of the choices made; for instance rather than
> rename the local "progname" variables in all those places, I would
> rename the global to logging_progname in logging.c; in bringetbitmap
> (0002) I would rename the outer "tmp" to "sizecheck" or something like
> that.  I guess this is mostly matter of mostly arbitrary judgment ...
> 

I will rename them per your suggestion, and check if there are similar things to rename.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  22. Re: Cleanup shadows variable warnings, round 1

    Álvaro Herrera <alvherre@kurilemu.de> — 2026-03-03T12:12:38Z

    On 2026-Mar-03, Chao Li wrote:

> > On Mar 3, 2026, at 18:26, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> > 
> > Pushed 0001.
> 
> Hi Alvaro, thank you very much for pushing 0001. But I don’t see it. I
> tried to pull master and refresh
> https://git.postgresql.org/cgit/postgresql.git/log/?qt=grep&q=, none
> of them shows 0001 pushed.

Oh, pager decided to intervene and hang the whole thing.  Done now.

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

  23. Re: Cleanup shadows variable warnings, round 1

    Chao Li <li.evan.chao@gmail.com> — 2026-03-04T06:17:41Z

    
> On Mar 3, 2026, at 18:26, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> 
> 1. if you rename a function argument, then the function declaration
> should match the new name as well.

Fully addressed.

> 2. xlogrecovery.c has far too many global variables.  Can we use this
> opportunity to try to get rid of some of them?  Especially one called
> "xlogreader" is I think quite bug-prone.

I looked into this. There are quite a few file-scope static variables and global variables, and getting rid of them would likely require a fairly large refactoring.

For now, I worked out an approach that wraps the file-scope static variables into a structure. I moved this change to the last commit and marked it as WIP. I plan to spend more time on the refactoring.

In the meantime, I wonder if it would make sense to handle this refactoring in a separate patch.

> 
> 3. I disagree with some of the choices made; for instance rather than
> rename the local "progname" variables in all those places, I would
> rename the global to logging_progname in logging.c;

The progname conflicts are not caused by logging.c. Instead, it is declared in postmaster.h:
```
extern PGDLLIMPORT const char *progname;
```

I hesitate to rename this global since it is exported, and doing so might lead to additional changes elsewhere. For now, I moved this commit to the second last one, and I may spend more time investigating it.


> in bringetbitmap
> (0002) I would rename the outer "tmp" to "sizecheck" or something like
> that.  I guess this is mostly matter of mostly arbitrary judgment ...
> 

I updated bringetbitmap to rename the outer variable.

I also went through the whole patch and tuned a few other names. Please let me know if you disagree with any of the other renamings.

PFA v7. Each commit is independent, so they do not need to be pushed in the same order as in this patch.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  24. Re: Cleanup shadows variable warnings, round 1

    Chao Li <li.evan.chao@gmail.com> — 2026-04-21T07:01:31Z

    
> On Mar 4, 2026, at 14:17, Chao Li <li.evan.chao@gmail.com> wrote:
> 
> 
> 
>> On Mar 3, 2026, at 18:26, Álvaro Herrera <alvherre@kurilemu.de> wrote:
>> 
>> 1. if you rename a function argument, then the function declaration
>> should match the new name as well.
> 
> Fully addressed.
> 
>> 2. xlogrecovery.c has far too many global variables.  Can we use this
>> opportunity to try to get rid of some of them?  Especially one called
>> "xlogreader" is I think quite bug-prone.
> 
> I looked into this. There are quite a few file-scope static variables and global variables, and getting rid of them would likely require a fairly large refactoring.
> 
> For now, I worked out an approach that wraps the file-scope static variables into a structure. I moved this change to the last commit and marked it as WIP. I plan to spend more time on the refactoring.
> 
> In the meantime, I wonder if it would make sense to handle this refactoring in a separate patch.
> 
>> 
>> 3. I disagree with some of the choices made; for instance rather than
>> rename the local "progname" variables in all those places, I would
>> rename the global to logging_progname in logging.c;
> 
> The progname conflicts are not caused by logging.c. Instead, it is declared in postmaster.h:
> ```
> extern PGDLLIMPORT const char *progname;
> ```
> 
> I hesitate to rename this global since it is exported, and doing so might lead to additional changes elsewhere. For now, I moved this commit to the second last one, and I may spend more time investigating it.
> 
> 
>> in bringetbitmap
>> (0002) I would rename the outer "tmp" to "sizecheck" or something like
>> that.  I guess this is mostly matter of mostly arbitrary judgment ...
>> 
> 
> I updated bringetbitmap to rename the outer variable.
> 
> I also went through the whole patch and tuned a few other names. Please let me know if you disagree with any of the other renamings.
> 
> PFA v7. Each commit is independent, so they do not need to be pushed in the same order as in this patch.
> 
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
> 
> 
> 
> 
> <v7-0001-cleanup-rename-inner-variables-to-avoid-shadowing.patch><v7-0002-cleanup-rename-outer-variables-to-avoid-shadowing.patch><v7-0003-cleanup-fix-macro-induced-variable-shadowing-in-i.patch><v7-0004-cleanup-avoid-local-wal_level-and-wal_segment_siz.patch><v7-0005-cleanup-avoid-local-variables-shadowed-by-static-.patch><v7-0006-cleanup-avoid-local-variables-shadowed-by-globals.patch><v7-0007-cleanup-avoid-local-variables-shadowed-by-static-.patch><v7-0008-cleanup-rename-local-conn-variables-to-avoid-shad.patch><v7-0009-cleanup-avoid-local-variables-shadowed-by-globals.patch><v7-0010-cleanup-avoid-local-variables-shadowed-by-globals.patch><v7-0011-cleanup-rename-local-progname-variables-to-avoid-.patch><v7-0012-WIP-xlogrecovery-consolidate-file-local-mutable-s.patch>

PFA v8 - rebased and fixed a few new occurrences.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  25. Re: Cleanup shadows variable warnings, round 1

    David Rowley <dgrowleyml@gmail.com> — 2026-04-21T11:02:44Z

    On Tue, 21 Apr 2026 at 19:02, Chao Li <li.evan.chao@gmail.com> wrote:
> PFA v8 - rebased and fixed a few new occurrences.

Which of these are new to v19?  Can you separate those ones out? IMO,
we should commit at least those, as those won't cause any backpatching
pain.

I recall that I was motivated to commit the -Wshadow=compatible-local
ones as there'd been a few bugs with shadowed variables. There'd be
more of an argument to cause all this churn if it was for something
more than to make things better for clang users.

David

  26. Re: Cleanup shadows variable warnings, round 1

    Álvaro Herrera <alvherre@kurilemu.de> — 2026-04-21T13:51:10Z

    On 2026-Apr-21, David Rowley wrote:

> On Tue, 21 Apr 2026 at 19:02, Chao Li <li.evan.chao@gmail.com> wrote:
> > PFA v8 - rebased and fixed a few new occurrences.
> 
> Which of these are new to v19?  Can you separate those ones out? IMO,
> we should commit at least those, as those won't cause any backpatching
> pain.

I agree.  The others are v20 material.


Specifically about 0003 (v20 material for sure, as this is ancient
code), I don't like this patch very much.  I wonder if it would be
possible to do away with the idea of using these codeFragment things
without introducing a performance issue here.  Is that doable by turning
these macros into static functions?

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Puedes vivir sólo una vez, pero si lo haces bien, una vez es suficiente"

  27. Re: Cleanup shadows variable warnings, round 1

    Chao Li <li.evan.chao@gmail.com> — 2026-04-22T05:14:01Z

    
> On Apr 21, 2026, at 21:51, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> 
> On 2026-Apr-21, David Rowley wrote:
> 
>> On Tue, 21 Apr 2026 at 19:02, Chao Li <li.evan.chao@gmail.com> wrote:
>>> PFA v8 - rebased and fixed a few new occurrences.
>> 
>> Which of these are new to v19?  Can you separate those ones out? IMO,
>> we should commit at least those, as those won't cause any backpatching
>> pain.
> 
> I agree.  The others are v20 material.
> 

Sounds reasonable.

The attached new v1 patch fixes the v19-only shadow warnings. There are not many. I strictly limited it to warnings newly introduced in v19, without touching any pre-existing ones, even where an old occurrence is very close to a new one.

I intentionally left out one occurrence in ruleutils.c:
```
ruleutils.c:13100:23: warning: declaration shadows a local variable [-Wshadow]
 13100 |                                                 deparse_context context = {0};
       |                                                                 ^
ruleutils.c:12955:67: note: previous declaration is here
 12955 | get_from_clause_item(Node *jtnode, Query *query, deparse_context *context)
       |                                                                   ^
1 warning generated.
```

I saw there is a thread [1] that will remove this deparse_context context = {0};, so I skipped this one to avoid a potential conflict.

Besides the patch file, I am also attaching three files for reference:

* v18-shadow-warnings.txt - all shadow warnings from branch REL_18_STABLE
* v19-shadow-warnings-master.txt - all shadow warnings from current master (9d3e094f12c)
* v19-shadow-warnings-patched.txt - all shadow warnings after applying this v1 patch

Except for the one in ruleutils.c, v19-shadow-warnings-patched.txt is a pure subset of v18-shadow-warnings.txt. You don't need to read these large files; they are attached only for reference.

I will recreate the previous patch set for v20.

> 
> Specifically about 0003 (v20 material for sure, as this is ancient
> code), I don't like this patch very much.  I wonder if it would be
> possible to do away with the idea of using these codeFragment things
> without introducing a performance issue here.  Is that doable by turning
> these macros into static functions?
> 

Okay, I will remove 0003 from this patch set, and use a separate patch to try converting the macros to static functions.

[1] http://postgr.es/m/CAHg+QDcLVa2iBnggkHxY4itZbXtDMfsYHEjnCUYe9hNbnxDi-w@mail.gmail.com

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  28. Re: Cleanup shadows variable warnings, round 1

    Peter Smith <smithpb2250@gmail.com> — 2026-04-22T06:32:25Z

    Hi Chao-San.

A couple of comments for your v1-0001 cleanup patch.

======
src/bin/pg_dump/pg_dumpall.c

I guess you were just making the minimal changes, but I thought
parseDumpFormat could have been simplified more by removing that local
variable entirely.

BEFORE
if (pg_strcasecmp(format, "c") == 0)
  archFormat = archCustom;
else if (pg_strcasecmp(format, "custom") == 0)
  archFormat = archCustom;
else if (pg_strcasecmp(format, "d") == 0)
  archFormat = archDirectory;
else if (pg_strcasecmp(format, "directory") == 0)
  archFormat = archDirectory;
else if (pg_strcasecmp(format, "p") == 0)
  archFormat = archNull;
else if (pg_strcasecmp(format, "plain") == 0)
  archFormat = archNull;
else if (pg_strcasecmp(format, "t") == 0)
  archFormat = archTar;
else if (pg_strcasecmp(format, "tar") == 0)
  archFormat = archTar;

SUGGESTION
if (pg_strcasecmp(format, "c") == 0 ||
  pg_strcasecmp(format, "custom") == 0)
  return archCustom;

if (pg_strcasecmp(format, "d") == 0 ||
  pg_strcasecmp(format, "directory") == 0)
  return archDirectory;

if (pg_strcasecmp(format, "p") == 0 ||
  pg_strcasecmp(format, "plain") == 0)
  return archNull;

if (pg_strcasecmp(format, "t") == 0 ||
  pg_strcasecmp(format, "tar") == 0)
  return archTar;

======
src/bin/psql/describe.c

I know you were addressing only "new" issues, but it seemed a bit
strange to fix only this one when there was the same issue earlier
(~line 1780) in the same function.

if (tableinfo.relkind == RELKIND_SEQUENCE)
{
 PGresult   *result = NULL;
 printQueryOpt myopt = pset.popt;

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

  29. Re: Cleanup shadows variable warnings, round 1

    Yuchen Li <liyuchen_xyz@163.com> — 2026-04-22T06:34:46Z

    On 4/22/2026 1:14 PM, Chao Li wrote:
>
>> On Apr 21, 2026, at 21:51, Álvaro Herrera <alvherre@kurilemu.de> wrote:
>>
>> On 2026-Apr-21, David Rowley wrote:
>>
>>> On Tue, 21 Apr 2026 at 19:02, Chao Li <li.evan.chao@gmail.com> wrote:
>>>> PFA v8 - rebased and fixed a few new occurrences.
>>> Which of these are new to v19?  Can you separate those ones out? IMO,
>>> we should commit at least those, as those won't cause any backpatching
>>> pain.
>> I agree.  The others are v20 material.
>>
> Sounds reasonable.
>
> The attached new v1 patch fixes the v19-only shadow warnings. There are not many. I strictly limited it to warnings newly introduced in v19, without touching any pre-existing ones, even where an old occurrence is very close to a new one.
>
> I intentionally left out one occurrence in ruleutils.c:
> ```
> ruleutils.c:13100:23: warning: declaration shadows a local variable [-Wshadow]
>   13100 |                                                 deparse_context context = {0};
>         |                                                                 ^
> ruleutils.c:12955:67: note: previous declaration is here
>   12955 | get_from_clause_item(Node *jtnode, Query *query, deparse_context *context)
>         |                                                                   ^
> 1 warning generated.
> ```
>
> I saw there is a thread [1] that will remove this deparse_context context = {0};, so I skipped this one to avoid a potential conflict.
>
> Besides the patch file, I am also attaching three files for reference:
>
> * v18-shadow-warnings.txt - all shadow warnings from branch REL_18_STABLE
> * v19-shadow-warnings-master.txt - all shadow warnings from current master (9d3e094f12c)
> * v19-shadow-warnings-patched.txt - all shadow warnings after applying this v1 patch
>
> Except for the one in ruleutils.c, v19-shadow-warnings-patched.txt is a pure subset of v18-shadow-warnings.txt. You don't need to read these large files; they are attached only for reference.
>
> I will recreate the previous patch set for v20.
>
>> Specifically about 0003 (v20 material for sure, as this is ancient
>> code), I don't like this patch very much.  I wonder if it would be
>> possible to do away with the idea of using these codeFragment things
>> without introducing a performance issue here.  Is that doable by turning
>> these macros into static functions?
>>
> Okay, I will remove 0003 from this patch set, and use a separate patch to try converting the macros to static functions.
>
> [1] http://postgr.es/m/CAHg+QDcLVa2iBnggkHxY4itZbXtDMfsYHEjnCUYe9hNbnxDi-w@mail.gmail.com
>
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
>
>
>
>
The v1 patch LGTM, and “make check-world” passed after applying it.

Regards,
Yuchen Li

  30. Re: Cleanup shadows variable warnings, round 1

    Chao Li <li.evan.chao@gmail.com> — 2026-04-22T07:24:37Z

    
> On Apr 22, 2026, at 14:32, Peter Smith <smithpb2250@gmail.com> wrote:
> 
> Hi Chao-San.
> 
> A couple of comments for your v1-0001 cleanup patch.

Hi Peter, thank you very much for reviewing.

> 
> ======
> src/bin/pg_dump/pg_dumpall.c
> 
> I guess you were just making the minimal changes, but I thought
> parseDumpFormat could have been simplified more by removing that local
> variable entirely.
> 
> BEFORE
> if (pg_strcasecmp(format, "c") == 0)
>  archFormat = archCustom;
> else if (pg_strcasecmp(format, "custom") == 0)
>  archFormat = archCustom;
> else if (pg_strcasecmp(format, "d") == 0)
>  archFormat = archDirectory;
> else if (pg_strcasecmp(format, "directory") == 0)
>  archFormat = archDirectory;
> else if (pg_strcasecmp(format, "p") == 0)
>  archFormat = archNull;
> else if (pg_strcasecmp(format, "plain") == 0)
>  archFormat = archNull;
> else if (pg_strcasecmp(format, "t") == 0)
>  archFormat = archTar;
> else if (pg_strcasecmp(format, "tar") == 0)
>  archFormat = archTar;
> 
> SUGGESTION
> if (pg_strcasecmp(format, "c") == 0 ||
>  pg_strcasecmp(format, "custom") == 0)
>  return archCustom;
> 
> if (pg_strcasecmp(format, "d") == 0 ||
>  pg_strcasecmp(format, "directory") == 0)
>  return archDirectory;
> 
> if (pg_strcasecmp(format, "p") == 0 ||
>  pg_strcasecmp(format, "plain") == 0)
>  return archNull;
> 
> if (pg_strcasecmp(format, "t") == 0 ||
>  pg_strcasecmp(format, "tar") == 0)
>  return archTar;
> 

Yes, I was trying to keep the changes minimal. Consider that if we later submit a trivial patch just to refactor this function as you suggested, it might be hard to get it through the process. So, as touching the code, it might make sense to do the refactoring now.

Looks like ending a non-void function with pg_fatal() does not trigger a compile warning. I also noticed that get_encoding_id() in initdb.c returns int and has pg_fatal() as its last statement, which makes me more comfortable doing this refactoring.

> ======
> src/bin/psql/describe.c
> 
> I know you were addressing only "new" issues, but it seemed a bit
> strange to fix only this one when there was the same issue earlier
> (~line 1780) in the same function.
> 
> if (tableinfo.relkind == RELKIND_SEQUENCE)
> {
> PGresult   *result = NULL;
> printQueryOpt myopt = pset.popt;
> 

I also found that a bit odd, which is why I specially said in my previous email: "I strictly limited it to warnings newly introduced in v19, without touching any pre-existing ones, even where an old occurrence is very close to a new one.”

So for this case, I’d prefer to hear David’s or Alvaro’s view, since they asked to limit this to v19-only occurrences.

PFA v2: refactoring parseDumpFormat as Peter suggested.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  31. Re: Cleanup shadows variable warnings, round 1

    David Rowley <dgrowleyml@gmail.com> — 2026-04-23T05:19:22Z

    On Wed, 22 Apr 2026 at 17:14, Chao Li <li.evan.chao@gmail.com> wrote:
> The attached new v1 patch fixes the v19-only shadow warnings. There are not many. I strictly limited it to warnings newly introduced in v19, without touching any pre-existing ones, even where an old occurrence is very close to a new one.

Thank you. I pushed those after some adjustments.

While doing that, I did think more on if we should do more of this for
v20. I keep thinking back to the times when I've had to write 6
different versions of a patch to back patch to 6 different branches.
It's rarely that bad, but it sure does make you swear when the 6th
"git am" fails, especially when you find out that it was for a very
trivial thing, such as a spelling mistake fix. You really have to
fight off the temptation of complacency after the first 3 or so failed
git ams.

A worse category of problems that this particular set of patches could
cause is no conflict when we want one. I personally always write bug
fixes for master and back-patch them, but if anyone were to work
forward to newer versions, then imagine someone adding some code to a
function that does something with a local variable that's shadowed
globally. If they forward patch that to a version where the local
variable has been renamed, everything compiles and might appear to
work, but it's now the global that's being changed when the new code
was meant to change the local. Maybe no committers work that way, but
if they do, it's a real risk.

IMO, without any references to recent bugs that have been fixed due to
shadowing, then I can't see beyond the fact that this might be more
likely to cause bugs than to prevent them. As I recall, we were about
borderline on doing -Wshadow=compatible-local.  At least for
non-compatible variables, I'd expect you'd get a warning or error
during compilation. For the record, I got motivated for Justin's work
on the compatible-local due to af7d270dd. I removed a shadowed
variable which was incorrect. In my view, Justin Pryzby's proposal to
do something about this was well timed. I'm not seeing the same thing
happen here. Maybe I missed it?

David

  32. Re: Cleanup shadows variable warnings, round 1

    Chao Li <li.evan.chao@gmail.com> — 2026-04-23T06:34:03Z

    
> On Apr 23, 2026, at 13:19, David Rowley <dgrowleyml@gmail.com> wrote:
> 
> On Wed, 22 Apr 2026 at 17:14, Chao Li <li.evan.chao@gmail.com> wrote:
>> The attached new v1 patch fixes the v19-only shadow warnings. There are not many. I strictly limited it to warnings newly introduced in v19, without touching any pre-existing ones, even where an old occurrence is very close to a new one.
> 
> Thank you. I pushed those after some adjustments.
> 
> While doing that, I did think more on if we should do more of this for
> v20. I keep thinking back to the times when I've had to write 6
> different versions of a patch to back patch to 6 different branches.
> It's rarely that bad, but it sure does make you swear when the 6th
> "git am" fails, especially when you find out that it was for a very
> trivial thing, such as a spelling mistake fix. You really have to
> fight off the temptation of complacency after the first 3 or so failed
> git ams.
> 
> A worse category of problems that this particular set of patches could
> cause is no conflict when we want one. I personally always write bug
> fixes for master and back-patch them, but if anyone were to work
> forward to newer versions, then imagine someone adding some code to a
> function that does something with a local variable that's shadowed
> globally. If they forward patch that to a version where the local
> variable has been renamed, everything compiles and might appear to
> work, but it's now the global that's being changed when the new code
> was meant to change the local. Maybe no committers work that way, but
> if they do, it's a real risk.
> 
> IMO, without any references to recent bugs that have been fixed due to
> shadowing, then I can't see beyond the fact that this might be more
> likely to cause bugs than to prevent them. As I recall, we were about
> borderline on doing -Wshadow=compatible-local.  At least for
> non-compatible variables, I'd expect you'd get a warning or error
> during compilation. For the record, I got motivated for Justin's work
> on the compatible-local due to af7d270dd. I removed a shadowed
> variable which was incorrect. In my view, Justin Pryzby's proposal to
> do something about this was well timed. I'm not seeing the same thing
> happen here. Maybe I missed it?
> 
> David

Hi David,

Thank you very much for accepting this v19-only patch.

I helped prepare back-patch diff files for [1] today, from v10 to v18. It was only a tiny change, but I still ended up with 3 diff files across 9 branches, which was quite painful. I can understand that, as a committer, you probably run into that kind of pain regularly, and would prefer to avoid adding more of it.

I’ll hold off on the rest of this cleanup unless there is a concrete reason to revisit it in the future.

[1] https://postgr.es/21E668C0-CEAE-44F8-B585-319F31883AFE@gmail.com

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/