public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: Peter Smith <[email protected]>
To: Álvaro Herrera <[email protected]>
To: David Rowley <[email protected]>
Cc: Postgres hackers <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Subject: Re: Cleanup shadows variable warnings, round 1
Date: Wed, 22 Apr 2026 15:24:37 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAHut+PuK3t_o63=x4N26sMKBF1koxCJdzQ75zuc-igyskigvtQ@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<CAHut+PuK3t_o63=x4N26sMKBF1koxCJdzQ75zuc-igyskigvtQ@mail.gmail.com>
> 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/
Attachments:
[application/octet-stream] v2-0001-Cleanup-v19-introduced-shadow-variable-warnings.patch (8.9K, 2-v2-0001-Cleanup-v19-introduced-shadow-variable-warnings.patch)
download | inline diff:
From ba90cf5ce8dd34594234651bed461f26e5b7f99e Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
Date: Wed, 22 Apr 2026 11:33:16 +0800
Subject: [PATCH v2] Cleanup v19 introduced shadow-variable warnings
Suggested-by: David Rowley <[email protected]>
Author: Chao Li <[email protected]>
Reviewed-by: Peter Smith <[email protected]>
Reviewed-by: Yuchen Li <[email protected]>
Discussion: https://postgr.es/m/CAEoWx2kQ2x5gMaj8tHLJ3=jfC+p5YXHkJyHrDTiQw2nn2FJTmQ@mail.gmail.com
---
src/backend/commands/tablecmds.c | 28 ++++++-------
src/backend/commands/wait.c | 12 +++---
src/backend/postmaster/datachecksum_state.c | 8 ++--
src/bin/pg_dump/pg_dumpall.c | 45 +++++++++------------
src/bin/psql/describe.c | 12 +++---
5 files changed, 50 insertions(+), 55 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1068d7c7b6a..ec8207687c0 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -22610,7 +22610,7 @@ createTableConstraints(List **wqueue, AlteredTableInfo *tab,
bool ccvalid = constr->check[ccnum].ccvalid;
Node *ccbin_node;
bool found_whole_row;
- Constraint *constr;
+ Constraint *con;
/*
* The partitioned table can not have a NO INHERIT check constraint
@@ -22632,19 +22632,19 @@ createTableConstraints(List **wqueue, AlteredTableInfo *tab,
ccname,
RelationGetRelationName(parent_rel));
- constr = makeNode(Constraint);
- constr->contype = CONSTR_CHECK;
- constr->conname = pstrdup(ccname);
- constr->deferrable = false;
- constr->initdeferred = false;
- constr->is_enforced = ccenforced;
- constr->skip_validation = !ccvalid;
- constr->initially_valid = ccvalid;
- constr->is_no_inherit = ccnoinherit;
- constr->raw_expr = NULL;
- constr->cooked_expr = nodeToString(ccbin_node);
- constr->location = -1;
- constraints = lappend(constraints, constr);
+ con = makeNode(Constraint);
+ con->contype = CONSTR_CHECK;
+ con->conname = pstrdup(ccname);
+ con->deferrable = false;
+ con->initdeferred = false;
+ con->is_enforced = ccenforced;
+ con->skip_validation = !ccvalid;
+ con->initially_valid = ccvalid;
+ con->is_no_inherit = ccnoinherit;
+ con->raw_expr = NULL;
+ con->cooked_expr = nodeToString(ccbin_node);
+ con->location = -1;
+ constraints = lappend(constraints, con);
}
/* Install all CHECK constraints. */
diff --git a/src/backend/commands/wait.c b/src/backend/commands/wait.c
index 382d5c2d44f..7fe53a6f045 100644
--- a/src/backend/commands/wait.c
+++ b/src/backend/commands/wait.c
@@ -92,7 +92,7 @@ ExecWaitStmt(ParseState *pstate, WaitStmt *stmt, bool isTopLevel,
{
char *timeout_str;
const char *hintmsg;
- double result;
+ double val;
if (timeout_specified)
errorConflictingDefElem(defel, pstate);
@@ -100,7 +100,7 @@ ExecWaitStmt(ParseState *pstate, WaitStmt *stmt, bool isTopLevel,
timeout_str = defGetString(defel);
- if (!parse_real(timeout_str, &result, GUC_UNIT_MS, &hintmsg))
+ if (!parse_real(timeout_str, &val, GUC_UNIT_MS, &hintmsg))
{
ereport(ERROR,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
@@ -113,20 +113,20 @@ ExecWaitStmt(ParseState *pstate, WaitStmt *stmt, bool isTopLevel,
* don't fail on just-out-of-range values that would round into
* range.
*/
- result = rint(result);
+ val = rint(val);
/* Range check */
- if (unlikely(isnan(result) || !FLOAT8_FITS_IN_INT64(result)))
+ if (unlikely(isnan(val) || !FLOAT8_FITS_IN_INT64(val)))
ereport(ERROR,
errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("timeout value is out of range"));
- if (result < 0)
+ if (val < 0)
ereport(ERROR,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("timeout cannot be negative"));
- timeout = (int64) result;
+ timeout = (int64) val;
}
else if (strcmp(defel->defname, "no_throw") == 0)
{
diff --git a/src/backend/postmaster/datachecksum_state.c b/src/backend/postmaster/datachecksum_state.c
index b3e99170669..9f9a2f9c40e 100644
--- a/src/backend/postmaster/datachecksum_state.c
+++ b/src/backend/postmaster/datachecksum_state.c
@@ -546,7 +546,7 @@ StartDataChecksumsWorkerLauncher(DataChecksumsWorkerOperation op,
{
BackgroundWorker bgw;
BackgroundWorkerHandle *bgw_handle;
- bool launcher_running;
+ bool is_running;
DataChecksumsWorkerOperation launcher_running_op;
#ifdef USE_ASSERT_CHECKING
@@ -565,8 +565,8 @@ StartDataChecksumsWorkerLauncher(DataChecksumsWorkerOperation op,
DataChecksumState->launch_cost_limit = cost_limit;
/* Is the launcher already running? If so, what is it doing? */
- launcher_running = DataChecksumState->launcher_running;
- if (launcher_running)
+ is_running = DataChecksumState->launcher_running;
+ if (is_running)
launcher_running_op = DataChecksumState->operation;
LWLockRelease(DataChecksumsWorkerLock);
@@ -589,7 +589,7 @@ StartDataChecksumsWorkerLauncher(DataChecksumsWorkerOperation op,
* already in the desired state, i.e. if the checksums are already enabled
* and you call pg_enable_data_checksums().
*/
- if (!launcher_running)
+ if (!is_running)
{
/*
* Prepare the BackgroundWorker and launch it.
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 9e904f76baa..ba780bd6c29 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -83,7 +83,7 @@ static void buildShSecLabels(PGconn *conn,
PQExpBuffer buffer);
static void executeCommand(PGconn *conn, const char *query);
static void check_for_invalid_global_names(PGconn *conn,
- SimpleStringList *database_exclude_names);
+ SimpleStringList *db_exclude_names);
static void expand_dbname_patterns(PGconn *conn, SimpleStringList *patterns,
SimpleStringList *names);
static void read_dumpall_filters(const char *filename, SimpleStringList *pattern);
@@ -2269,7 +2269,7 @@ executeCommand(PGconn *conn, const char *query)
*/
static void
check_for_invalid_global_names(PGconn *conn,
- SimpleStringList *database_exclude_names)
+ SimpleStringList *db_exclude_names)
{
PGresult *res;
int i;
@@ -2296,7 +2296,7 @@ check_for_invalid_global_names(PGconn *conn,
/* Skip excluded databases since they won't be in map.dat */
if (strcmp(objtype, "database") == 0 &&
- simple_string_list_member(database_exclude_names, objname))
+ simple_string_list_member(db_exclude_names, objname))
continue;
if (strpbrk(objname, "\n\r"))
@@ -2406,29 +2406,24 @@ read_dumpall_filters(const char *filename, SimpleStringList *pattern)
static ArchiveFormat
parseDumpFormat(const char *format)
{
- ArchiveFormat archDumpFormat;
-
- if (pg_strcasecmp(format, "c") == 0)
- archDumpFormat = archCustom;
- else if (pg_strcasecmp(format, "custom") == 0)
- archDumpFormat = archCustom;
- else if (pg_strcasecmp(format, "d") == 0)
- archDumpFormat = archDirectory;
- else if (pg_strcasecmp(format, "directory") == 0)
- archDumpFormat = archDirectory;
- else if (pg_strcasecmp(format, "p") == 0)
- archDumpFormat = archNull;
- else if (pg_strcasecmp(format, "plain") == 0)
- archDumpFormat = archNull;
- else if (pg_strcasecmp(format, "t") == 0)
- archDumpFormat = archTar;
- else if (pg_strcasecmp(format, "tar") == 0)
- archDumpFormat = archTar;
- else
- pg_fatal("unrecognized output format \"%s\"; please specify \"c\", \"d\", \"p\", or \"t\"",
- format);
+ 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;
- return archDumpFormat;
+ pg_fatal("unrecognized output format \"%s\"; please specify \"c\", \"d\", \"p\", or \"t\"",
+ format);
}
/*
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index dd1179ef927..974dbc5ec2c 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1938,7 +1938,7 @@ describeOneTableDetails(const char *schemaname,
*/
if (tableinfo.relkind == RELKIND_PROPGRAPH)
{
- printQueryOpt myopt = pset.popt;
+ printQueryOpt mypopt = pset.popt;
char *footers[3] = {NULL, NULL, NULL};
printfPQExpBuffer(&buf, "/* %s */\n", _("Get property graph information"));
@@ -1993,12 +1993,12 @@ describeOneTableDetails(const char *schemaname,
}
}
- myopt.footers = footers;
- myopt.topt.default_footer = false;
- myopt.title = title.data;
- myopt.translate_header = true;
+ mypopt.footers = footers;
+ mypopt.topt.default_footer = false;
+ mypopt.title = title.data;
+ mypopt.translate_header = true;
- printQuery(res, &myopt, pset.queryFout, false, pset.logfile);
+ printQuery(res, &mypopt, pset.queryFout, false, pset.logfile);
free(footers[0]);
free(footers[1]);
--
2.50.1 (Apple Git-155)
view thread (30+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Cleanup shadows variable warnings, round 1
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox