From: Nathan Bossart <nathandbossart@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: vignesh C <vignesh21@gmail.com>
Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>
Cc: Julien Rouhaud <rjuju123@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Subject: Re: pg_upgrade and logical replication
Date: Mon, 22 Jul 2024 09:46:29 -0500
Message-ID: <Zp5wxYCy9GyJgRVI@nathan> (raw)
In-Reply-To: <CAA4eK1+P6Up5c9C5ORBCsAFvad-Rf847LmjsfohpQopVUUb5DQ@mail.gmail.com>
References: <TYCPR01MB12077C4D38046B89599322EBBF54C2@TYCPR01MB12077.jpnprd01.prod.outlook.com>
<CALDaNm1kdAnfx8N1AUQUuxc0zXbghFKi3-4vFAym_Y3hPOm1hA@mail.gmail.com>
<TYCPR01MB120778982E38315F541D61AD1F5512@TYCPR01MB12077.jpnprd01.prod.outlook.com>
<CAA4eK1+B47ooT2HD1OTbSkVedK1BJLbr2K-seTd9pg9-wTgOdQ@mail.gmail.com>
<CALDaNm0z=fLtio1h50K8WossUGXU+gy0H9y9=RYh1DDZiq2EDw@mail.gmail.com>
<CAA4eK1LrO2qGcQot-9EgxpQRiT1O5+enHb57oN7OSNKSTn2hNQ@mail.gmail.com>
<ZprQJv_TxccN3tkr@nathan>
<ZpxsWx5jwlMCBZfn@nathan>
<Zp2-XDir9teSyAW6@paquier.xyz>
<CAA4eK1+P6Up5c9C5ORBCsAFvad-Rf847LmjsfohpQopVUUb5DQ@mail.gmail.com>
On Mon, Jul 22, 2024 at 03:45:19PM +0530, Amit Kapila wrote:
> On Mon, Jul 22, 2024 at 7:35 AM Michael Paquier <michael@paquier.xyz> wrote:
>> On Sat, Jul 20, 2024 at 09:03:07PM -0500, Nathan Bossart wrote:
>> >> This is an extremely expensive way to perform that check, and so I'm
>> >> wondering why we don't just do
>> >>
>> >> SELECT count(*) FROM pg_catalog.pg_subscription;
>> >>
>> >> once in count_old_cluster_subscriptions().
>> >
>> > Like so...
>
> Isn't it better to directly invoke get_subscription_count() in
> check_new_cluster_subscription_configuration() where it is required
> rather than in a db-specific general function?
IIUC the old cluster won't be running at that point.
>> Ah, good catch. That sounds like a good thing to do because we don't
>> care about the number of subscriptions for each database in the
>> current code.
>>
>> This is something that qualifies as an open item, IMO, as this code
>> is new to PG17.
+1
>> A comment in get_db_rel_and_slot_infos() becomes incorrect where
>> get_old_cluster_logical_slot_infos() is called; it is still referring
>> to the subscription count.
I removed this comment since IMHO it doesn't add much.
>> Actually, on the same grounds, couldn't we do the logical slot info
>> retrieval in get_old_cluster_logical_slot_infos() in a single pass as
>> well? pg_replication_slots reports some information about all the
>> slots, and the current code has a qual on current_database(). It
>> looks to me that this could be replaced by a single query, ordering
>> the slots by database names, assigning the slot infos in each
>> database's DbInfo at the end.
>
> Unlike subscriptions, logical slots are database-specific objects. We
> have some checks in the code like the one in CreateDecodingContext()
> for MyDatabaseId which may or may not create a problem for this case
> as we don't consume changes when checking
> LogicalReplicationSlotHasPendingWal via
> binary_upgrade_logical_slot_has_caught_up() but I think this needs
> more analysis than what Nathan has proposed. So, I suggest taking up
> this task for PG18 if we want to optimize this code path.
I see what you mean.
--
nathan
Attachments:v2-0001-pg_upgrade-retrieve-subscription-count-more-effic.patch (text/plain, 5.5K) download
From b3c3c6533e7f221dc869e613e78802a3054d42b3 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Sat, 20 Jul 2024 21:01:29 -0500
Subject: [PATCH v2 1/1] pg_upgrade: retrieve subscription count more
efficiently
---
src/bin/pg_upgrade/check.c | 9 +++----
src/bin/pg_upgrade/info.c | 43 +++++++--------------------------
src/bin/pg_upgrade/pg_upgrade.h | 3 +--
3 files changed, 13 insertions(+), 42 deletions(-)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.cindex 27924159d6..39d14b7b92 100644--- a/src/bin/pg_upgrade/check.c+++ b/src/bin/pg_upgrade/check.c@@ -1797,17 +1797,14 @@ check_new_cluster_subscription_configuration(void)
{
PGresult *res;
PGconn *conn;
- int nsubs_on_old;
int max_replication_slots;
/* Subscriptions and their dependencies can be migrated since PG17. */
if (GET_MAJOR_VERSION(old_cluster.major_version) < 1700)
return;
- nsubs_on_old = count_old_cluster_subscriptions();-
/* Quick return if there are no subscriptions to be migrated. */
- if (nsubs_on_old == 0)+ if (old_cluster.nsubs == 0)
return;
prep_status("Checking for new cluster configuration for subscriptions");
@@ -1821,10 +1818,10 @@ check_new_cluster_subscription_configuration(void)
pg_fatal("could not determine parameter settings on new cluster");
max_replication_slots = atoi(PQgetvalue(res, 0, 0));
- if (nsubs_on_old > max_replication_slots)+ if (old_cluster.nsubs > max_replication_slots)
pg_fatal("\"max_replication_slots\" (%d) must be greater than or equal to the number of "
"subscriptions (%d) on the old cluster",
- max_replication_slots, nsubs_on_old);+ max_replication_slots, old_cluster.nsubs);
PQclear(res);
PQfinish(conn);
diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.cindex 95c22a7200..e43be79aa5 100644--- a/src/bin/pg_upgrade/info.c+++ b/src/bin/pg_upgrade/info.c@@ -28,7 +28,7 @@ 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_db_subscription_count(DbInfo *dbinfo);+static void get_subscription_count(ClusterInfo *cluster);
/*
@@ -293,17 +293,13 @@ get_db_rel_and_slot_infos(ClusterInfo *cluster, bool live_check)
get_rel_infos(cluster, pDbInfo);
- /*- * Retrieve the logical replication slots infos and the subscriptions- * count for the old cluster.- */
if (cluster == &old_cluster)
- {
get_old_cluster_logical_slot_infos(pDbInfo, live_check);
- get_db_subscription_count(pDbInfo);- }
}
+ if (cluster == &old_cluster)+ get_subscription_count(cluster);+
if (cluster == &old_cluster)
pg_log(PG_VERBOSE, "\nsource databases:");
else
@@ -750,14 +746,14 @@ count_old_cluster_logical_slots(void)
/*
* get_db_subscription_count()
*
- * Gets the number of subscriptions in the database referred to by "dbinfo".+ * Gets the number of subscriptions in the cluster.
*
* Note: This function will not do anything if the old cluster is pre-PG17.
* This is because before that the logical slots are not upgraded, so we will
* not be able to upgrade the logical replication clusters completely.
*/
static void
-get_db_subscription_count(DbInfo *dbinfo)+get_subscription_count(ClusterInfo *cluster)
{
PGconn *conn;
PGresult *res;
@@ -766,36 +762,15 @@ get_db_subscription_count(DbInfo *dbinfo)
if (GET_MAJOR_VERSION(old_cluster.major_version) < 1700)
return;
- conn = connectToServer(&old_cluster, dbinfo->db_name);+ conn = connectToServer(&old_cluster, "template1");
res = executeQueryOrDie(conn, "SELECT count(*) "
- "FROM pg_catalog.pg_subscription WHERE subdbid = %u",- dbinfo->db_oid);- dbinfo->nsubs = atoi(PQgetvalue(res, 0, 0));+ "FROM pg_catalog.pg_subscription");+ cluster->nsubs = atoi(PQgetvalue(res, 0, 0));
PQclear(res);
PQfinish(conn);
}
-/*- * count_old_cluster_subscriptions()- *- * Returns the number of subscriptions for all databases.- *- * Note: this function always returns 0 if the old_cluster is PG16 and prior- * because we gather subscriptions only for cluster versions greater than or- * equal to PG17. See get_db_subscription_count().- */-int-count_old_cluster_subscriptions(void)-{- int nsubs = 0;-- for (int dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)- nsubs += old_cluster.dbarr.dbs[dbnum].nsubs;-- return nsubs;-}-
static void
free_db_and_rel_infos(DbInfoArr *db_arr)
{
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.hindex 8afe240bdf..e6dbbe6a93 100644--- a/src/bin/pg_upgrade/pg_upgrade.h+++ b/src/bin/pg_upgrade/pg_upgrade.h@@ -197,7 +197,6 @@ typedef struct
* path */
RelInfoArr rel_arr; /* array of all user relinfos */
LogicalSlotInfoArr slot_arr; /* array of all LogicalSlotInfo */
- int nsubs; /* number of subscriptions */
} DbInfo;
/*
@@ -296,6 +295,7 @@ typedef struct
char major_version_str[64]; /* string PG_VERSION of cluster */
uint32 bin_version; /* version returned from pg_ctl */
const char *tablespace_suffix; /* directory specification */
+ int nsubs; /* number of subscriptions */
} ClusterInfo;
@@ -430,7 +430,6 @@ FileNameMap *gen_db_file_maps(DbInfo *old_db,
const char *new_pgdata);
void get_db_rel_and_slot_infos(ClusterInfo *cluster, bool live_check);
int count_old_cluster_logical_slots(void);
-int count_old_cluster_subscriptions(void);
/* option.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, amit.kapila16@gmail.com, michael@paquier.xyz, vignesh21@gmail.com, kuroda.hayato@fujitsu.com, pryzby@telsasoft.com, sawada.mshk@gmail.com, smithpb2250@gmail.com, rjuju123@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: pg_upgrade and logical replication
In-Reply-To: <Zp5wxYCy9GyJgRVI@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