Re: pg_upgrade and logical replication

On Mon, Jul 22, 2024 at 09:46:29AM -0500, Nathan Bossart wrote:
> 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(at)paquier(dot)xyz> wrote:
>>> 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.

WFM.

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

I am not sure to get the reason why get_old_cluster_logical_slot_infos()
could not be optimized, TBH. LogicalReplicationSlotHasPendingWal()
uses the fast forward mode where no changes are generated, hence there
should be no need for a dependency to a connection to a specific
database :)

Combined to a hash table based on the database name and/or OID to know
to which dbinfo to attach the information of a slot, then it should be
possible to use one query, making the slot info gathering closer to
O(N) rather than the current O(N^2).
--
Michael