Here are a few miscellaneous cleanup patches for pg_upgrade. I don't think
there's anything controversial here.
0001 removes some extra whitespace in the status message for failed data
type checks. I noticed that when the check fails, this status message is
indented beyond all the other output. This appears to have been introduced
in commit 347758b, so I'd back-patch this one to v17.
0002 improves the coding style in many of the new upgrade task callback
functions. I refrained from adjusting this code too much when converting
these tasks to use the new pg_upgrade task framework (see commit 40e2e5e),
but now I think we should. This decreases the amount of indentation in
some places and removes a few dozen lines of code.
0003 adds names to the UpgradeTaskSlotState enum and the UpgradeTaskSlot
struct. I'm not aware of any established project policy in this area, but
I figured it'd be good to at least be consistent within the same file.
Thoughts?
--
nathan
>From c958311e5fd6df460e0c1289646fe99b9dd5f7f7 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Tue, 17 Sep 2024 11:30:50 -0500
Subject: [PATCH v1 1/3] remove extra whitespace in pg_upgrade report
---
src/bin/pg_upgrade/check.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 1c277ce17b..c9a3f06b80 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -419,7 +419,7 @@ process_data_type_check(DbInfo *dbinfo, PGresult *res, void
*arg)
*/
if (!(*state->result))
{
- pg_log(PG_REPORT, " failed check: %s",
_(state->check->status));
+ pg_log(PG_REPORT, "failed check: %s",
_(state->check->status));
appendPQExpBuffer(*state->report, "\n%s\n%s %s\n",
_(state->check->report_text),
_("A list of the
problem columns is in the file:"),
--
2.39.3 (Apple Git-146)
>From 6f3cd97974eb6f73c1513c4e6093c1222268c42c Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Tue, 17 Sep 2024 11:57:52 -0500
Subject: [PATCH v1 2/3] improve style of upgrade task callback functions
---
src/bin/pg_upgrade/check.c | 205 +++++++++++++++--------------------
src/bin/pg_upgrade/version.c | 29 +++--
2 files changed, 99 insertions(+), 135 deletions(-)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index c9a3f06b80..bc4c9b11a0 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -390,69 +390,55 @@ process_data_type_check(DbInfo *dbinfo, PGresult *res,
void *arg)
{
struct data_type_check_state *state = (struct data_type_check_state *)
arg;
int ntups = PQntuples(res);
+ char output_path[MAXPGPATH];
+ int i_nspname = PQfnumber(res, "nspname");
+ int i_relname = PQfnumber(res, "relname");
+ int i_attname = PQfnumber(res, "attname");
+ FILE *script = NULL;
AssertVariableIsOfType(&process_data_type_check, UpgradeTaskProcessCB);
- if (ntups)
- {
- char output_path[MAXPGPATH];
- int i_nspname;
- int i_relname;
- int i_attname;
- FILE *script = NULL;
- bool db_used = false;
+ if (ntups == 0)
+ return;
- snprintf(output_path, sizeof(output_path), "%s/%s",
- log_opts.basedir,
- state->check->report_filename);
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ state->check->report_filename);
- /*
- * Make sure we have a buffer to save reports to now that we
found a
- * first failing check.
- */
- if (*state->report == NULL)
- *state->report = createPQExpBuffer();
+ /*
+ * Make sure we have a buffer to save reports to now that we found a
first
+ * failing check.
+ */
+ if (*state->report == NULL)
+ *state->report = createPQExpBuffer();
- /*
- * If this is the first time we see an error for the check in
question
- * then print a status message of the failure.
- */
- if (!(*state->result))
- {
- pg_log(PG_REPORT, "failed check: %s",
_(state->check->status));
- appendPQExpBuffer(*state->report, "\n%s\n%s %s\n",
-
_(state->check->report_text),
- _("A list of the
problem columns is in the file:"),
- output_path);
- }
- *state->result = true;
+ /*
+ * If this is the first time we see an error for the check in question
+ * then print a status message of the failure.
+ */
+ if (!(*state->result))
+ {
+ pg_log(PG_REPORT, "failed check: %s", _(state->check->status));
+ appendPQExpBuffer(*state->report, "\n%s\n%s %s\n",
+ _(state->check->report_text),
+ _("A list of the problem
columns is in the file:"),
+ output_path);
+ }
+ *state->result = true;
- i_nspname = PQfnumber(res, "nspname");
- i_relname = PQfnumber(res, "relname");
- i_attname = PQfnumber(res, "attname");
+ if (script == NULL &&
+ (script = fopen_priv(output_path, "a")) == NULL)
+ pg_fatal("could not open file \"%s\": %m", output_path);
- for (int rowno = 0; rowno < ntups; rowno++)
- {
- if (script == NULL && (script = fopen_priv(output_path,
"a")) == NULL)
- pg_fatal("could not open file \"%s\": %m",
output_path);
+ fprintf(script, "In database: %s\n", dbinfo->db_name);
- if (!db_used)
- {
- fprintf(script, "In database: %s\n",
dbinfo->db_name);
- db_used = true;
- }
- fprintf(script, " %s.%s.%s\n",
- PQgetvalue(res, rowno, i_nspname),
- PQgetvalue(res, rowno, i_relname),
- PQgetvalue(res, rowno, i_attname));
- }
+ for (int rowno = 0; rowno < ntups; rowno++)
+ fprintf(script, " %s.%s.%s\n",
+ PQgetvalue(res, rowno, i_nspname),
+ PQgetvalue(res, rowno, i_relname),
+ PQgetvalue(res, rowno, i_attname));
- if (script)
- {
- fclose(script);
- script = NULL;
- }
- }
+ fclose(script);
}
/*
@@ -1234,7 +1220,6 @@ check_for_prepared_transactions(ClusterInfo *cluster)
static void
process_isn_and_int8_passing_mismatch(DbInfo *dbinfo, PGresult *res, void *arg)
{
- bool db_used = false;
int ntups = PQntuples(res);
int i_nspname = PQfnumber(res, "nspname");
int i_proname = PQfnumber(res, "proname");
@@ -1243,20 +1228,19 @@ process_isn_and_int8_passing_mismatch(DbInfo *dbinfo,
PGresult *res, void *arg)
AssertVariableIsOfType(&process_isn_and_int8_passing_mismatch,
UpgradeTaskProcessCB);
+ if (ntups == 0)
+ return;
+
+ if (report->file == NULL &&
+ (report->file = fopen_priv(report->path, "w")) == NULL)
+ pg_fatal("could not open file \"%s\": %m", report->path);
+
+ fprintf(report->file, "In database: %s\n", dbinfo->db_name);
+
for (int rowno = 0; rowno < ntups; rowno++)
- {
- if (report->file == NULL &&
- (report->file = fopen_priv(report->path, "w")) == NULL)
- pg_fatal("could not open file \"%s\": %m",
report->path);
- if (!db_used)
- {
- fprintf(report->file, "In database: %s\n",
dbinfo->db_name);
- db_used = true;
- }
fprintf(report->file, " %s.%s\n",
PQgetvalue(res, rowno, i_nspname),
PQgetvalue(res, rowno, i_proname));
- }
}
/*
@@ -1324,7 +1308,6 @@ process_user_defined_postfix_ops(DbInfo *dbinfo, PGresult
*res, void *arg)
{
UpgradeTaskReport *report = (UpgradeTaskReport *) arg;
int ntups = PQntuples(res);
- bool db_used = false;
int i_oproid = PQfnumber(res, "oproid");
int i_oprnsp = PQfnumber(res, "oprnsp");
int i_oprname = PQfnumber(res, "oprname");
@@ -1334,26 +1317,22 @@ process_user_defined_postfix_ops(DbInfo *dbinfo,
PGresult *res, void *arg)
AssertVariableIsOfType(&process_user_defined_postfix_ops,
UpgradeTaskProcessCB);
- if (!ntups)
+ if (ntups == 0)
return;
+ if (report->file == NULL &&
+ (report->file = fopen_priv(report->path, "w")) == NULL)
+ pg_fatal("could not open file \"%s\": %m", report->path);
+
+ fprintf(report->file, "In database: %s\n", dbinfo->db_name);
+
for (int rowno = 0; rowno < ntups; rowno++)
- {
- if (report->file == NULL &&
- (report->file = fopen_priv(report->path, "w")) == NULL)
- pg_fatal("could not open file \"%s\": %m",
report->path);
- if (!db_used)
- {
- fprintf(report->file, "In database: %s\n",
dbinfo->db_name);
- db_used = true;
- }
fprintf(report->file, " (oid=%s) %s.%s (%s.%s, NONE)\n",
PQgetvalue(res, rowno, i_oproid),
PQgetvalue(res, rowno, i_oprnsp),
PQgetvalue(res, rowno, i_oprname),
PQgetvalue(res, rowno, i_typnsp),
PQgetvalue(res, rowno, i_typname));
- }
}
/*
@@ -1422,7 +1401,6 @@ static void
process_incompat_polymorphics(DbInfo *dbinfo, PGresult *res, void *arg)
{
UpgradeTaskReport *report = (UpgradeTaskReport *) arg;
- bool db_used = false;
int ntups = PQntuples(res);
int i_objkind = PQfnumber(res, "objkind");
int i_objname = PQfnumber(res, "objname");
@@ -1430,21 +1408,19 @@ process_incompat_polymorphics(DbInfo *dbinfo, PGresult
*res, void *arg)
AssertVariableIsOfType(&process_incompat_polymorphics,
UpgradeTaskProcessCB);
- for (int rowno = 0; rowno < ntups; rowno++)
- {
- if (report->file == NULL &&
- (report->file = fopen_priv(report->path, "w")) == NULL)
- pg_fatal("could not open file \"%s\": %m",
report->path);
- if (!db_used)
- {
- fprintf(report->file, "In database: %s\n",
dbinfo->db_name);
- db_used = true;
- }
+ if (ntups == 0)
+ return;
+
+ if (report->file == NULL &&
+ (report->file = fopen_priv(report->path, "w")) == NULL)
+ pg_fatal("could not open file \"%s\": %m", report->path);
+ fprintf(report->file, "In database: %s\n", dbinfo->db_name);
+
+ for (int rowno = 0; rowno < ntups; rowno++)
fprintf(report->file, " %s: %s\n",
PQgetvalue(res, rowno, i_objkind),
PQgetvalue(res, rowno, i_objname));
- }
}
/*
@@ -1558,30 +1534,25 @@ static void
process_with_oids_check(DbInfo *dbinfo, PGresult *res, void *arg)
{
UpgradeTaskReport *report = (UpgradeTaskReport *) arg;
- bool db_used = false;
int ntups = PQntuples(res);
int i_nspname = PQfnumber(res, "nspname");
int i_relname = PQfnumber(res, "relname");
AssertVariableIsOfType(&process_with_oids_check, UpgradeTaskProcessCB);
- if (!ntups)
+ if (ntups == 0)
return;
+ if (report->file == NULL &&
+ (report->file = fopen_priv(report->path, "w")) == NULL)
+ pg_fatal("could not open file \"%s\": %m", report->path);
+
+ fprintf(report->file, "In database: %s\n", dbinfo->db_name);
+
for (int rowno = 0; rowno < ntups; rowno++)
- {
- if (report->file == NULL &&
- (report->file = fopen_priv(report->path, "w")) == NULL)
- pg_fatal("could not open file \"%s\": %m",
report->path);
- if (!db_used)
- {
- fprintf(report->file, "In database: %s\n",
dbinfo->db_name);
- db_used = true;
- }
fprintf(report->file, " %s.%s\n",
PQgetvalue(res, rowno, i_nspname),
PQgetvalue(res, rowno, i_relname));
- }
}
/*
@@ -1693,7 +1664,6 @@ static void
process_user_defined_encoding_conversions(DbInfo *dbinfo, PGresult *res, void
*arg)
{
UpgradeTaskReport *report = (UpgradeTaskReport *) arg;
- bool db_used = false;
int ntups = PQntuples(res);
int i_conoid = PQfnumber(res, "conoid");
int i_conname = PQfnumber(res, "conname");
@@ -1702,24 +1672,20 @@ process_user_defined_encoding_conversions(DbInfo
*dbinfo, PGresult *res, void *a
AssertVariableIsOfType(&process_user_defined_encoding_conversions,
UpgradeTaskProcessCB);
- if (!ntups)
+ if (ntups == 0)
return;
+ if (report->file == NULL &&
+ (report->file = fopen_priv(report->path, "w")) == NULL)
+ pg_fatal("could not open file \"%s\": %m", report->path);
+
+ fprintf(report->file, "In database: %s\n", dbinfo->db_name);
+
for (int rowno = 0; rowno < ntups; rowno++)
- {
- if (report->file == NULL &&
- (report->file = fopen_priv(report->path, "w")) == NULL)
- pg_fatal("could not open file \"%s\": %m",
report->path);
- if (!db_used)
- {
- fprintf(report->file, "In database: %s\n",
dbinfo->db_name);
- db_used = true;
- }
fprintf(report->file, " (oid=%s) %s.%s\n",
PQgetvalue(res, rowno, i_conoid),
PQgetvalue(res, rowno, i_nspname),
PQgetvalue(res, rowno, i_conname));
- }
}
/*
@@ -1971,7 +1937,7 @@ static void
process_old_sub_state_check(DbInfo *dbinfo, PGresult *res, void *arg)
{
UpgradeTaskReport *report = (UpgradeTaskReport *) arg;
- int ntup = PQntuples(res);
+ int ntups = PQntuples(res);
int i_srsubstate = PQfnumber(res, "srsubstate");
int i_subname = PQfnumber(res, "subname");
int i_nspname = PQfnumber(res, "nspname");
@@ -1979,19 +1945,20 @@ process_old_sub_state_check(DbInfo *dbinfo, PGresult
*res, void *arg)
AssertVariableIsOfType(&process_old_sub_state_check,
UpgradeTaskProcessCB);
- for (int i = 0; i < ntup; i++)
- {
- if (report->file == NULL &&
- (report->file = fopen_priv(report->path, "w")) == NULL)
- pg_fatal("could not open file \"%s\": %m",
report->path);
+ if (ntups == 0)
+ return;
+ if (report->file == NULL &&
+ (report->file = fopen_priv(report->path, "w")) == NULL)
+ pg_fatal("could not open file \"%s\": %m", report->path);
+
+ for (int i = 0; i < ntups; i++)
fprintf(report->file, "The table sync state \"%s\" is not
allowed for database:\"%s\" subscription:\"%s\" schema:\"%s\"
relation:\"%s\"\n",
PQgetvalue(res, i, i_srsubstate),
dbinfo->db_name,
PQgetvalue(res, i, i_subname),
PQgetvalue(res, i, i_nspname),
PQgetvalue(res, i, i_relname));
- }
}
/*
diff --git a/src/bin/pg_upgrade/version.c b/src/bin/pg_upgrade/version.c
index 5084b08805..2a3b6ebb34 100644
--- a/src/bin/pg_upgrade/version.c
+++ b/src/bin/pg_upgrade/version.c
@@ -147,31 +147,28 @@ old_9_6_invalidate_hash_indexes(ClusterInfo *cluster,
bool check_mode)
static void
process_extension_updates(DbInfo *dbinfo, PGresult *res, void *arg)
{
- bool db_used = false;
int ntups = PQntuples(res);
int i_name = PQfnumber(res, "name");
UpgradeTaskReport *report = (UpgradeTaskReport *) arg;
+ PQExpBufferData connectbuf;
AssertVariableIsOfType(&process_extension_updates,
UpgradeTaskProcessCB);
- for (int rowno = 0; rowno < ntups; rowno++)
- {
- if (report->file == NULL &&
- (report->file = fopen_priv(report->path, "w")) == NULL)
- pg_fatal("could not open file \"%s\": %m",
report->path);
- if (!db_used)
- {
- PQExpBufferData connectbuf;
+ if (ntups == 0)
+ return;
- initPQExpBuffer(&connectbuf);
- appendPsqlMetaConnect(&connectbuf, dbinfo->db_name);
- fputs(connectbuf.data, report->file);
- termPQExpBuffer(&connectbuf);
- db_used = true;
- }
+ if (report->file == NULL &&
+ (report->file = fopen_priv(report->path, "w")) == NULL)
+ pg_fatal("could not open file \"%s\": %m", report->path);
+
+ initPQExpBuffer(&connectbuf);
+ appendPsqlMetaConnect(&connectbuf, dbinfo->db_name);
+ fputs(connectbuf.data, report->file);
+ termPQExpBuffer(&connectbuf);
+
+ for (int rowno = 0; rowno < ntups; rowno++)
fprintf(report->file, "ALTER EXTENSION %s UPDATE;\n",
quote_identifier(PQgetvalue(res, rowno,
i_name)));
- }
}
/*
--
2.39.3 (Apple Git-146)
>From 516b02f446572a8ae6550b39213bba3360263baf Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Tue, 17 Sep 2024 13:57:21 -0500
Subject: [PATCH v1 3/3] add names to some upgrade task structs
---
src/bin/pg_upgrade/task.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/bin/pg_upgrade/task.c b/src/bin/pg_upgrade/task.c
index 8ef5d26eb5..ba1726c25e 100644
--- a/src/bin/pg_upgrade/task.c
+++ b/src/bin/pg_upgrade/task.c
@@ -89,7 +89,7 @@ struct UpgradeTask
/*
* The different states for a parallel slot.
*/
-typedef enum
+typedef enum UpgradeTaskSlotState
{
FREE, /* slot available for
use in a new database */
CONNECTING, /* waiting for
connection to be established */
@@ -99,7 +99,7 @@ typedef enum
/*
* We maintain an array of user_opts.jobs slots to execute the task.
*/
-typedef struct
+typedef struct UpgradeTaskSlot
{
UpgradeTaskSlotState state; /* state of the slot */
int db_idx; /* index of the
database assigned to slot */
--
2.39.3 (Apple Git-146)
