On Sat, May 23, 2026 at 2:49 PM Imran Zaheer <[email protected]> wrote:
>
> Hi
>
> Thanks for the review. In the attached patch, I added an argument that
> will help explicitly control whether to stop logical decoding or not.
>
> -ReplicationSlotDropAcquired(void)
> +ReplicationSlotDropAcquired(bool disable_logical_decoding)
>
> I hope this will be enough to make the caller intent more explicit and
> will prevent future omissions like this.
>
Overall it looks good. I have a few trivial comments; please
incorporate them if you agree.
1)
+ReplicationSlotDropAcquired(bool disable_logical_decoding)
We can change comments atop this function. Suggestion :
/*
* Permanently drop the currently acquired replication slot and
* request the checkpointer to disable logical decoding if requested
* by the caller.
*/
2)
Additionally I think it will be good to have below sanity check in
ReplicationSlotDropAcquired(). Thoughts?
/* Ensure that slot is logical if caller has requested to disable
logical decoding */
Assert(!disable_logical_decoding || SlotIsLogical(MyReplicationSlot));
3)
@@ -567,7 +567,7 @@ drop_local_obsolete_slots(List *remote_slot_list)
if (synced_slot)
{
ReplicationSlotAcquire(NameStr(local_slot->data.name), true, false);
- ReplicationSlotDropAcquired();
+ ReplicationSlotDropAcquired(false);
}
Since it is a logical slot and we are still passing false, we may add
a comment. Suggestion:
/*
* We don't want to disable logical decoding during slot drop on the
* physical standby, since the standby derives the logical decoding
* state from the upstream server in the replication chain.
*/
thanks
Shveta
