MIME-Version: 1.0
References: 
 <CA+UBfaktds57dw2M8BEv_kS-=ixph3w+3MxKixtaDQMi_k7Ybg@mail.gmail.com>
 <CAJpy0uDmBEHJAk7i5afYaqGA8V8j1XdfAT_=WYwZ=AhHZu6Z1A@mail.gmail.com>
 <CAD21AoBSUi58=aL0uRPO57d8Qjrd_cakGeuguDEy+fPRKRLh2Q@mail.gmail.com>
 <CAJpy0uDzMzyfc2VOt+H1B96mjRXjgJ8oVLatxkNjPHzpHuQifA@mail.gmail.com>
 <CA+UBfakjtK=Dh9NQTf7g5F+eMoDbEQUSa8BN6RBGPFzGr3H-sA@mail.gmail.com>
In-Reply-To: 
 <CA+UBfakjtK=Dh9NQTf7g5F+eMoDbEQUSa8BN6RBGPFzGr3H-sA@mail.gmail.com>
From: shveta malik <shveta.malik@gmail.com>
Date: Mon, 25 May 2026 09:41:38 +0530
Message-ID: 
 <CAJpy0uCV9L+D4kevxHGtxHKoOCnpdmW-3j4ssDbFtycThFC02A@mail.gmail.com>
Subject: Re: effective_wal_level is not decreasing after using REPACK
 (CONCURRENTLY)
To: Imran Zaheer <imran.zhir@gmail.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>,
	shveta malik <shvetamalik@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAJpy0uCV9L%2BD4kevxHGtxHKoOCnpdmW-3j4ssDbFtycThFC02A%40mail.gmail.com>
Precedence: bulk

On Sat, May 23, 2026 at 2:49=E2=80=AFPM 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