Improve style of pg_upgrade task callback functions.

I wanted to avoid adjusting this code too much when converting
these tasks to use the new parallelization framework (see commit
40e2e5e92b), which is why this is being done as a follow-up commit.
These stylistic adjustments result in fewer lines of code and fewer
levels of indentation in some places.

While at it, add names to the UpgradeTaskSlotState enum and the
UpgradeTaskSlot struct.  I'm not aware of any established project
policy in this area, but let's at least be consistent within the
same file.

Reviewed-by: Daniel Gustafsson
Discussion: https://postgr.es/m/ZunW7XHLd2uTts4f%40nathan

diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index c9a3f06b80b6a6722bf227b61b0fe1202de900c6..12735a426877f629d876962293a839bec9cfab54 100644 (file)
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -320,7 +320,7 @@ static DataTypesUsageChecks data_types_usage_checks[] =
 struct data_type_check_state
 {
        DataTypesUsageChecks *check;    /* the check for this step */
-       bool       *result;                     /* true if check failed for any database */
+       bool            result;                 /* true if check failed for any database */
        PQExpBuffer *report;            /* buffer for report on failed checks */
 };
 
@@ -390,69 +390,54 @@ 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 = 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);
 }
 
 /*
@@ -477,7 +462,6 @@ process_data_type_check(DbInfo *dbinfo, PGresult *res, void *arg)
 static void
 check_for_data_types_usage(ClusterInfo *cluster)
 {
-       bool       *results;
        PQExpBuffer report = NULL;
        DataTypesUsageChecks *tmp = data_types_usage_checks;
        int                     n_data_types_usage_checks = 0;
@@ -494,8 +478,7 @@ check_for_data_types_usage(ClusterInfo *cluster)
                tmp++;
        }
 
-       /* Prepare an array to store the results of checks in */
-       results = pg_malloc0(sizeof(bool) * n_data_types_usage_checks);
+       /* Allocate memory for queries and for task states */
        queries = pg_malloc0(sizeof(char *) * n_data_types_usage_checks);
        states = pg_malloc0(sizeof(struct data_type_check_state) * n_data_types_usage_checks);
 
@@ -525,7 +508,6 @@ check_for_data_types_usage(ClusterInfo *cluster)
                queries[i] = data_type_check_query(i);
 
                states[i].check = check;
-               states[i].result = &results[i];
                states[i].report = &report;
 
                upgrade_task_add_step(task, queries[i], process_data_type_check,
@@ -545,7 +527,6 @@ check_for_data_types_usage(ClusterInfo *cluster)
                destroyPQExpBuffer(report);
        }
 
-       pg_free(results);
        for (int i = 0; i < n_data_types_usage_checks; i++)
        {
                if (queries[i])
@@ -1234,7 +1215,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 +1223,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 +1303,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 +1312,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 +1396,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 +1403,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 +1529,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 +1659,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 +1667,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 +1932,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 +1940,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/task.c b/src/bin/pg_upgrade/task.c
index 8ef5d26eb591eb1bd3bd946f194fd46c591edc92..ba1726c25e3a6067063dc4063b1e38e58270bbd1 100644 (file)
--- 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 */

diff --git a/src/bin/pg_upgrade/version.c b/src/bin/pg_upgrade/version.c
index 5084b088057e9346b52dd7d7c664a6c81e2f9a0d..2a3b6ebb34783f39e9e455539c513d3287aa113e 100644 (file)
--- 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)));
-       }
 }
 
 /*