On Mon, Jul 22, 2024 at 03:07:10PM -0500, Nathan Bossart wrote: > Here is a new patch set. I've included the latest revision of the patch to > fix get_db_subscription_count() from the other thread [0] as 0001 since I > expect that to be committed soon. I've also moved the patch that moves the > "live_check" variable to "user_opts" to 0002 since I plan on committing > that sooner than later, too. Otherwise, I've tried to address all feedback > provided thus far.
Here is just the live_check patch. I rebased it, gave it a commit message, and fixed a silly mistake. Barring objections or cfbot indigestion, I plan to commit this within the next couple of days. -- nathan
>From 963ca637e9b2481eb762de35b4d4eaa30faf5f47 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nat...@postgresql.org> Date: Wed, 24 Jul 2024 21:55:15 -0500 Subject: [PATCH v6 1/1] pg_upgrade: Move live_check variable to user_opts. At the moment, pg_upgrade stores whether it is doing a "live check" (i.e., the user specified --check and the old server is still running) in a variable scoped to main(). This live_check variable is passed to several functions. To further complicate matters, a few calls to said functions provide a hard-coded "false" as the live_check argument. Specifically, this is done when calling these functions for the new cluster, for which any live-check-only paths won't apply. This commit moves the live_check variable to the global user_opts variable, which stores information about the options the user provided on the command line. This allows us to remove the "live_check" parameter from several functions. For the functions with callers that provide a hard-coded "false" as the live_check argument (e.g., get_control_data()), we simply verify the given cluster is the old cluster before taking any live-check-only paths. This small refactoring effort aims to simplify proposed future commits that would parallelize many of pg_upgrade's once-in-each-database tasks using libpq's asynchronous APIs. Specifically, by removing the live_check parameter, we can more easily convert the functions to callbacks for the new parallel system. Reviewed-by: Daniel Gustafsson Discussion: https://postgr.es/m/20240516211638.GA1688936%40nathanxps13 --- src/bin/pg_upgrade/check.c | 30 +++++++++++++++--------------- src/bin/pg_upgrade/controldata.c | 3 ++- src/bin/pg_upgrade/info.c | 12 +++++------- src/bin/pg_upgrade/option.c | 4 ++-- src/bin/pg_upgrade/pg_upgrade.c | 21 ++++++++++----------- src/bin/pg_upgrade/pg_upgrade.h | 13 +++++++------ 6 files changed, 41 insertions(+), 42 deletions(-) diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c index 51e30a2f23..5038231731 100644 --- a/src/bin/pg_upgrade/check.c +++ b/src/bin/pg_upgrade/check.c @@ -29,7 +29,7 @@ static void check_for_new_tablespace_dir(void); static void check_for_user_defined_encoding_conversions(ClusterInfo *cluster); static void check_new_cluster_logical_replication_slots(void); static void check_new_cluster_subscription_configuration(void); -static void check_old_cluster_for_valid_slots(bool live_check); +static void check_old_cluster_for_valid_slots(void); static void check_old_cluster_subscription_state(void); /* @@ -555,9 +555,9 @@ fix_path_separator(char *path) } void -output_check_banner(bool live_check) +output_check_banner(void) { - if (user_opts.check && live_check) + if (user_opts.live_check) { pg_log(PG_REPORT, "Performing Consistency Checks on Old Live Server\n" @@ -573,18 +573,18 @@ output_check_banner(bool live_check) void -check_and_dump_old_cluster(bool live_check) +check_and_dump_old_cluster(void) { /* -- OLD -- */ - if (!live_check) + if (!user_opts.live_check) start_postmaster(&old_cluster, true); /* * Extract a list of databases, tables, and logical replication slots from * the old cluster. */ - get_db_rel_and_slot_infos(&old_cluster, live_check); + get_db_rel_and_slot_infos(&old_cluster); init_tablespaces(); @@ -605,7 +605,7 @@ check_and_dump_old_cluster(bool live_check) * Logical replication slots can be migrated since PG17. See comments * atop get_old_cluster_logical_slot_infos(). */ - check_old_cluster_for_valid_slots(live_check); + check_old_cluster_for_valid_slots(); /* * Subscriptions and their dependencies can be migrated since PG17. @@ -669,7 +669,7 @@ check_and_dump_old_cluster(bool live_check) if (!user_opts.check) generate_old_dump(); - if (!live_check) + if (!user_opts.live_check) stop_postmaster(false); } @@ -677,7 +677,7 @@ check_and_dump_old_cluster(bool live_check) void check_new_cluster(void) { - get_db_rel_and_slot_infos(&new_cluster, false); + get_db_rel_and_slot_infos(&new_cluster); check_new_cluster_is_empty(); @@ -828,14 +828,14 @@ check_cluster_versions(void) void -check_cluster_compatibility(bool live_check) +check_cluster_compatibility(void) { /* get/check pg_control data of servers */ - get_control_data(&old_cluster, live_check); - get_control_data(&new_cluster, false); + get_control_data(&old_cluster); + get_control_data(&new_cluster); check_control_data(&old_cluster.controldata, &new_cluster.controldata); - if (live_check && old_cluster.port == new_cluster.port) + if (user_opts.live_check && old_cluster.port == new_cluster.port) pg_fatal("When checking a live server, " "the old and new port numbers must be different."); } @@ -1838,7 +1838,7 @@ check_new_cluster_subscription_configuration(void) * before shutdown. */ static void -check_old_cluster_for_valid_slots(bool live_check) +check_old_cluster_for_valid_slots(void) { char output_path[MAXPGPATH]; FILE *script = NULL; @@ -1877,7 +1877,7 @@ check_old_cluster_for_valid_slots(bool live_check) * Note: This can be satisfied only when the old cluster has been * shut down, so we skip this for live checks. */ - if (!live_check && !slot->caught_up) + if (!user_opts.live_check && !slot->caught_up) { if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL) diff --git a/src/bin/pg_upgrade/controldata.c b/src/bin/pg_upgrade/controldata.c index 1f0ccea3ed..854c6887a2 100644 --- a/src/bin/pg_upgrade/controldata.c +++ b/src/bin/pg_upgrade/controldata.c @@ -33,7 +33,7 @@ * return valid xid data for a running server. */ void -get_control_data(ClusterInfo *cluster, bool live_check) +get_control_data(ClusterInfo *cluster) { char cmd[MAXPGPATH]; char bufin[MAX_STRING]; @@ -76,6 +76,7 @@ get_control_data(ClusterInfo *cluster, bool live_check) uint32 segno = 0; char *resetwal_bin; int rc; + bool live_check = (cluster == &old_cluster && user_opts.live_check); /* * Because we test the pg_resetwal output as strings, it has to be in diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c index c07a69b63e..5de5e10945 100644 --- a/src/bin/pg_upgrade/info.c +++ b/src/bin/pg_upgrade/info.c @@ -27,7 +27,7 @@ static void free_rel_infos(RelInfoArr *rel_arr); static void print_db_infos(DbInfoArr *db_arr); static void print_rel_infos(RelInfoArr *rel_arr); static void print_slot_infos(LogicalSlotInfoArr *slot_arr); -static void get_old_cluster_logical_slot_infos(DbInfo *dbinfo, bool live_check); +static void get_old_cluster_logical_slot_infos(DbInfo *dbinfo); /* @@ -272,11 +272,9 @@ report_unmatched_relation(const RelInfo *rel, const DbInfo *db, bool is_new_db) * * higher level routine to generate dbinfos for the database running * on the given "port". Assumes that server is already running. - * - * live_check would be used only when the target is the old cluster. */ void -get_db_rel_and_slot_infos(ClusterInfo *cluster, bool live_check) +get_db_rel_and_slot_infos(ClusterInfo *cluster) { int dbnum; @@ -293,7 +291,7 @@ get_db_rel_and_slot_infos(ClusterInfo *cluster, bool live_check) get_rel_infos(cluster, pDbInfo); if (cluster == &old_cluster) - get_old_cluster_logical_slot_infos(pDbInfo, live_check); + get_old_cluster_logical_slot_infos(pDbInfo); } if (cluster == &old_cluster) @@ -637,7 +635,7 @@ get_rel_infos(ClusterInfo *cluster, DbInfo *dbinfo) * are included. */ static void -get_old_cluster_logical_slot_infos(DbInfo *dbinfo, bool live_check) +get_old_cluster_logical_slot_infos(DbInfo *dbinfo) { PGconn *conn; PGresult *res; @@ -673,7 +671,7 @@ get_old_cluster_logical_slot_infos(DbInfo *dbinfo, bool live_check) "WHERE slot_type = 'logical' AND " "database = current_database() AND " "temporary IS FALSE;", - live_check ? "FALSE" : + user_opts.live_check ? "FALSE" : "(CASE WHEN invalidation_reason IS NOT NULL THEN FALSE " "ELSE (SELECT pg_catalog.binary_upgrade_logical_slot_has_caught_up(slot_name)) " "END)"); diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c index 548ea4e623..6f41d63eed 100644 --- a/src/bin/pg_upgrade/option.c +++ b/src/bin/pg_upgrade/option.c @@ -470,10 +470,10 @@ adjust_data_dir(ClusterInfo *cluster) * directory. */ void -get_sock_dir(ClusterInfo *cluster, bool live_check) +get_sock_dir(ClusterInfo *cluster) { #if !defined(WIN32) - if (!live_check) + if (!user_opts.live_check || cluster == &new_cluster) cluster->sockdir = user_opts.socketdir; else { diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c index 03eb738fd7..99f3d4543e 100644 --- a/src/bin/pg_upgrade/pg_upgrade.c +++ b/src/bin/pg_upgrade/pg_upgrade.c @@ -65,7 +65,7 @@ static void create_new_objects(void); static void copy_xact_xlog_xid(void); static void set_frozenxids(bool minmxid_only); static void make_outputdirs(char *pgdata); -static void setup(char *argv0, bool *live_check); +static void setup(char *argv0); static void create_logical_replication_slots(void); ClusterInfo old_cluster, @@ -88,7 +88,6 @@ int main(int argc, char **argv) { char *deletion_script_file_name = NULL; - bool live_check = false; /* * pg_upgrade doesn't currently use common/logging.c, but initialize it @@ -123,18 +122,18 @@ main(int argc, char **argv) */ make_outputdirs(new_cluster.pgdata); - setup(argv[0], &live_check); + setup(argv[0]); - output_check_banner(live_check); + output_check_banner(); check_cluster_versions(); - get_sock_dir(&old_cluster, live_check); - get_sock_dir(&new_cluster, false); + get_sock_dir(&old_cluster); + get_sock_dir(&new_cluster); - check_cluster_compatibility(live_check); + check_cluster_compatibility(); - check_and_dump_old_cluster(live_check); + check_and_dump_old_cluster(); /* -- NEW -- */ @@ -331,7 +330,7 @@ make_outputdirs(char *pgdata) static void -setup(char *argv0, bool *live_check) +setup(char *argv0) { /* * make sure the user has a clean environment, otherwise, we may confuse @@ -378,7 +377,7 @@ setup(char *argv0, bool *live_check) pg_fatal("There seems to be a postmaster servicing the old cluster.\n" "Please shutdown that postmaster and try again."); else - *live_check = true; + user_opts.live_check = true; } } @@ -660,7 +659,7 @@ create_new_objects(void) set_frozenxids(true); /* update new_cluster info now that we have objects in the databases */ - get_db_rel_and_slot_infos(&new_cluster, false); + get_db_rel_and_slot_infos(&new_cluster); } /* diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h index e2b99b49fa..2ef76ca2e7 100644 --- a/src/bin/pg_upgrade/pg_upgrade.h +++ b/src/bin/pg_upgrade/pg_upgrade.h @@ -322,6 +322,7 @@ typedef struct typedef struct { bool check; /* check clusters only, don't change any data */ + bool live_check; /* check clusters only, old server is running */ bool do_sync; /* flush changes to disk */ transferMode transfer_mode; /* copy files or link them? */ int jobs; /* number of processes/threads to use */ @@ -366,20 +367,20 @@ extern OSInfo os_info; /* check.c */ -void output_check_banner(bool live_check); -void check_and_dump_old_cluster(bool live_check); +void output_check_banner(void); +void check_and_dump_old_cluster(void); void check_new_cluster(void); void report_clusters_compatible(void); void issue_warnings_and_set_wal_level(void); void output_completion_banner(char *deletion_script_file_name); void check_cluster_versions(void); -void check_cluster_compatibility(bool live_check); +void check_cluster_compatibility(void); void create_script_for_old_cluster_deletion(char **deletion_script_file_name); /* controldata.c */ -void get_control_data(ClusterInfo *cluster, bool live_check); +void get_control_data(ClusterInfo *cluster); void check_control_data(ControlData *oldctrl, ControlData *newctrl); void disable_old_cluster(void); @@ -428,7 +429,7 @@ void check_loadable_libraries(void); FileNameMap *gen_db_file_maps(DbInfo *old_db, DbInfo *new_db, int *nmaps, const char *old_pgdata, const char *new_pgdata); -void get_db_rel_and_slot_infos(ClusterInfo *cluster, bool live_check); +void get_db_rel_and_slot_infos(ClusterInfo *cluster); int count_old_cluster_logical_slots(void); void get_subscription_count(ClusterInfo *cluster); @@ -436,7 +437,7 @@ void get_subscription_count(ClusterInfo *cluster); void parseCommandLine(int argc, char *argv[]); void adjust_data_dir(ClusterInfo *cluster); -void get_sock_dir(ClusterInfo *cluster, bool live_check); +void get_sock_dir(ClusterInfo *cluster); /* relfilenumber.c */ -- 2.39.3 (Apple Git-146)
