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

Hi,

Thanks Shveta,  I agree with the additional sanity checks and comments.

Here is the updated patch.


Thanks
Imran Zaheer

On Mon, May 25, 2026 at 9:11 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Sat, May 23, 2026 at 2:49 PM Imran Zaheer <imran.zhir@gmail.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