MIME-Version: 1.0
References: 
 <TYCPR01MB12077B16EEDA360BA645B96F8F54C2@TYCPR01MB12077.jpnprd01.prod.outlook.com>
 <CALDaNm3h7ReG5wB9bhPpU=GpVp9Cfotx36tRErw5oKEXu+cKTA@mail.gmail.com>
 <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>
In-Reply-To: <Zp2-XDir9teSyAW6@paquier.xyz>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Mon, 22 Jul 2024 15:45:19 +0530
Message-ID: 
 <CAA4eK1+P6Up5c9C5ORBCsAFvad-Rf847LmjsfohpQopVUUb5DQ@mail.gmail.com>
Subject: Re: pg_upgrade and logical replication
To: Michael Paquier <michael@paquier.xyz>
Cc: Nathan Bossart <nathandbossart@gmail.com>,
 vignesh C <vignesh21@gmail.com>,
	"Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>,
 Justin Pryzby <pryzby@telsasoft.com>,
	Masahiko Sawada <sawada.mshk@gmail.com>, Peter Smith <smithpb2250@gmail.com>,
	Julien Rouhaud <rjuju123@gmail.com>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAA4eK1%2BP6Up5c9C5ORBCsAFvad-Rf847LmjsfohpQopVUUb5DQ%40mail.gmail.com>
Precedence: bulk

On Mon, Jul 22, 2024 at 7:35=E2=80=AFAM Michael Paquier <michael@paquier.xy=
z> 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?

>
> 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.
>
> 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.
>
> 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.

--=20
With Regards,
Amit Kapila.