> On Apr 22, 2026, at 14:32, Peter Smith <smithpb2250@gmail.com> 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
functionas you suggested, it might be hard to get it through the process. So, as touching the code, it might make sense
todo 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
doingthis 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
newlyintroduced 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/
