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)));
-   }
 }
 
 /*