Re: effective_wal_level is not decreasing after using REPACK (CONCURRENTLY)

On Sat, May 23, 2026 at 2:49 PM Imran Zaheer <imran(dot)zhir(at)gmail(dot)com> 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