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