Subject: Re: Cleanup shadows variable warnings, round 1

From: Chao Li <[email-masked]>
Date: Tue, 3 Mar 2026 19:55:36 +0800

> On Mar 3, 2026, at 18:26, Álvaro Herrera <[email-masked]> 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/

---

From: Chao Li <[email-masked]>
Date: Wed, 4 Mar 2026 14:17:41 +0800

> should match the new name as well.

Fully addressed.

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

> 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

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,

---

From: Chao Li <[email-masked]>
Date: Tue, 21 Apr 2026 15:01:31 +0800

> On Mar 4, 2026, at 14:17, Chao Li <[email-masked]> wrote:
> 
> 
> 
> <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,

---

From: David Rowley <[email-masked]>
Date: Tue, 21 Apr 2026 23:02:44 +1200

> 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

---

From: Álvaro Herrera <[email-masked]>
Date: Tue, 21 Apr 2026 15:51:10 +0200

> we should commit at least those, as those won't cause any backpatching

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"

---

From: Chao Li <[email-masked]>
Date: Wed, 22 Apr 2026 13:14:01 +0800

> On Apr 21, 2026, at 21:51, Álvaro Herrera <[email-masked]> wrote:
> 

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.

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

Best regards,

---

From: Peter Smith <[email-masked]>
Date: Wed, 22 Apr 2026 16:32:25 +1000

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

---

From: Yuchen Li <[email-masked]>
Date: Wed, 22 Apr 2026 14:34:46 +0800

> [1] http://postgr.es/m/[email-masked]
The v1 patch LGTM, and “make check-world” passed after applying it.

Regards,
Yuchen Li

---

From: Chao Li <[email-masked]>
Date: Wed, 22 Apr 2026 15:24:37 +0800

> On Apr 22, 2026, at 14:32, Peter Smith <[email-masked]> wrote:
> 
> Hi Chao-San.
> 

Hi Peter, thank you very much for reviewing.

>  pg_strcasecmp(format, "tar") == 0)

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.

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

---

From: David Rowley <[email-masked]>
Date: Thu, 23 Apr 2026 17:19:22 +1200

> 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 ol…

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

---

From: Chao Li <[email-masked]>
Date: Thu, 23 Apr 2026 14:34:03 +0800

> On Apr 23, 2026, at 13:19, David Rowley <[email-masked]> wrote:
> 

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

Best regards,