From: Nathan Bossart Date: Thu, 26 Sep 2024 18:54:37 +0000 (-0500) Subject: Improve style of pg_upgrade task callback functions. X-Git-Tag: REL_18_BETA1~1843 X-Git-Url: http://git.postgresql.org/gitweb/?a=commitdiff_plain;h=9726653185de62f2f8bf42a25e961bc56895a41b;p=postgresql.git 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 c9a3f06b80b..12735a42687 100644 --- 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 8ef5d26eb59..ba1726c25e3 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 */ diff --git a/src/bin/pg_upgrade/version.c b/src/bin/pg_upgrade/version.c index 5084b088057..2a3b6ebb347 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))); - } } /*