Subject: Cleanup shadows variable warnings, round 1

From: Chao Li <[email-masked]>
Date: Fri, 28 Nov 2025 16:16:59 +0800

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=-
[email-masked]

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

---

From: Heikki Linnakangas <[email-masked]>
Date: Fri, 28 Nov 2025 11:11:04 +0200

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

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

---

From: Chao Li <[email-masked]>
Date: Fri, 28 Nov 2025 21:39:33 +0800

> On Nov 28, 2025, at 16:16, Chao Li <[email-masked]> 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=[email-masked]
> 

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

Best regards,
--
Chao Li (Evan)

---

From: Peter Eisentraut <[email-masked]>
Date: Sat, 29 Nov 2025 16:29:47 +0100

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

---

From: Chao Li <[email-masked]>
Date: Wed, 3 Dec 2025 10:28:36 +0800

Hi Peter,

On Nov 29, 2025, at 23:29, Peter Eisentraut <[email-masked]> wrote:

Hi Hackers,

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,

---

From: Peter Smith <[email-masked]>
Date: Wed, 3 Dec 2025 17:42:33 +1100

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

---

From: Chao Li <[email-masked]>
Date: Wed, 3 Dec 2025 14:52:14 +0800

> On Dec 3, 2025, at 14:42, Peter Smith <[email-masked]> wrote:
> 
> Patch v2-0001.
> 

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,

---

From: Chao Li <[email-masked]>
Date: Wed, 3 Dec 2025 18:56:33 +0800

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

---

From: Álvaro Herrera <[email-masked]>
Date: Wed, 3 Dec 2025 15:36:05 +0100

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

---

From: Andres Freund <[email-masked]>
Date: Wed, 3 Dec 2025 10:17:28 -0500

Hi,

> 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)" <[email-masked]>
> 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 <[email-masked]>
> Discussion: https://postgr.es/m/CAEoWx2kQ2x5gMaj8tHLJ3=[email-masked]
> ---
>  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

---

From: Peter Smith <[email-masked]>
Date: Thu, 4 Dec 2025 08:00:51 +1100

> 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

---

From: Chao Li <[email-masked]>
Date: Thu, 4 Dec 2025 07:04:02 +0800

> On Dec 4, 2025, at 05:00, Peter Smith <[email-masked]> wrote:
> 

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

Best regards,

---

From: Chao Li <[email-masked]>
Date: Thu, 4 Dec 2025 07:24:36 +0800

> name.)  This could be left alone, I guess -- not important.

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

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

 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.

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

---

From: Michael Paquier <[email-masked]>
Date: Thu, 4 Dec 2025 09:56:07 +0900

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

---

From: Peter Smith <[email-masked]>
Date: Thu, 4 Dec 2025 12:08:37 +1100

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

---

From: Chao Li <[email-masked]>
Date: Thu, 4 Dec 2025 17:10:42 +0800

On Dec 4, 2025, at 09:08, Peter Smith <[email-masked]> wrote:

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

This is actually 0002.

======

2.1.

BEFORE
foreach(lc, splitinfo)
{

======

Fixed.

2.2.

BEFORE

Fixed.

======

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

Fixed.

======

2.4.

BEFORE

Fixed.

======

2.5.

Fixed.

======

2.6.

Moved to 0003.

======

2.7.

Fixed.

======

2.8.

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,

---

From: Álvaro Herrera <[email-masked]>
Date: Thu, 4 Dec 2025 12:21:15 +0100

> CI run. I’ve run into this myself, and it’s disruptive for both patch

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)

---

From: Bilal Yavuz <[email-masked]>
Date: Thu, 4 Dec 2025 16:04:07 +0300

Hi,

> 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

---

From: Chao Li <[email-masked]>
Date: Tue, 3 Mar 2026 17:04:47 +0800

> On Dec 4, 2025, at 17:10, Chao Li <[email-masked]> wrote:
> 
> 
> 
> <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,