Cleanup shadows variable warnings, round 1

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

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] /messages/by-id/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/

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
</messages/by-id/20220817145434.GC26426@telsasoft.com&gt;
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.

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 <
/messages/by-id/20220817145434.GC26426@telsasoft.com&gt;
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/

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

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/

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/

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/

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: /messages/by-id/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

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

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/

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/

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

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

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/

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)

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]https://cirrus-ci.com/build/5444936843132928, 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

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/

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/