Re: pg_upgrade and logical replication

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?

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

-- 
With Regards,
Amit Kapila.