Обсуждение: 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
witha shadows-variable warning. Now I understand that -Wall doesn't by default enable -Wshadows with some compilers
likeclang. 
>
> I did a clean build with manually enabling -Wshadow and surprisingly found there are a lot of such warnings in the
currentcode 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
toproceed 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/

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.

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

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/