> On Apr 22, 2026, at 14:32, Peter Smith <[email protected]> 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/
v2-0001-Cleanup-v19-introduced-shadow-variable-warnings.patch
Description: Binary data
