On Mon, Jul 22, 2024 at 7:35 AM Michael Paquier <mich...@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.
