From: Nathan Bossart <nathandbossart@gmail.com>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Jeff Davis <pgsql@j-davis.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: optimizing pg_upgrade's once-in-each-database steps
Date: Wed, 24 Jul 2024 21:58:52 -0500
Message-ID: <ZqG_bBb7cvjFPRvm@nathan> (raw)
In-Reply-To: <Zp677slGLwhPhVWy@nathan>
References: <ZoMHsBasBy8LqkEU@nathan>
<CA+TgmoaBof36-ZuRWtu=+8rnhGTNQFFGNcmVB0x3aQawfc_JxA@mail.gmail.com>
<ZoMK9ZhOsWEKqxlM@nathan>
<ZowEIGd0lLKh16qe@nathan>
<ZoyveaS6a6jyhYS6@nathan>
<26742670-9539-4F30-B6FA-9C1DB743BC23@yesql.se>
<Zpg4WnS9BRWbgBLi@nathan>
<4493D323-9726-48F7-A702-E71FB991F76B@yesql.se>
<ZprY4e23vR95Nc51@nathan>
<Zp677slGLwhPhVWy@nathan>
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
Attachments:v6-0001-pg_upgrade-Move-live_check-variable-to-user_opts.patch (text/plain, 12.6K) download
From 963ca637e9b2481eb762de35b4d4eaa30faf5f47 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@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.cindex 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.cindex 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.cindex 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.cindex 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.cindex 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.hindex 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)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, daniel@yesql.se, robertmhaas@gmail.com, pgsql@j-davis.com
Subject: Re: optimizing pg_upgrade's once-in-each-database steps
In-Reply-To: <ZqG_bBb7cvjFPRvm@nathan>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox