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

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.

Thanks
Imran Zaheer

On Fri, May 22, 2026 at 1:57 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Fri, May 22, 2026 at 11:40 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Thu, May 21, 2026 at 9:19 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> > >
> > > On Thu, May 21, 2026 at 10:02 PM Imran Zaheer <imran(dot)zhir(at)gmail(dot)com> wrote:
> > > >
> > > > Hi
> > > >
> > > > The recent support for dynamic toggling of logical decoding (67c2097)
> > > > disables logical
> > > > decoding if no logical slots are present. But the repack command doesn't seem to
> > > > coordinate with this toggling. The effective_wal_level is not
> > > > decreasing after using repack concurrently.
> > > >
> > > > postgres=# show effective_wal_level;
> > > > effective_wal_level
> > > > ---------------------
> > > > replica
> > > > (1 row)
> > > >
> > > > postgres=# create table foo(a int primary key);
> > > > CREATE TABLE
> > > > postgres=# REPACK (CONCURRENTLY) foo;
> > > > 2026-05-21 20:46:25.423 PKT [1591896] LOG: logical decoding is
> > > > enabled upon creating a new logical replication slot
> > > > 2026-05-21 20:46:25.634 PKT [1591896] LOG: logical decoding found
> > > > consistent point at 0/018F36D0
> > > > 2026-05-21 20:46:25.634 PKT [1591896] DETAIL: There are no running
> > > > transactions.
> > > > REPACK
> > > > postgres=# select slot_name from pg_replication_slots;
> > > > slot_name
> > > > -----------
> > > > (0 rows)
> > > >
> > > > postgres=# show effective_wal_level;
> > > > effective_wal_level
> > > > ---------------------
> > > > logical
> > > > (1 row)
> > > >
> > > >
> > > > The server has to be restarted in order to decrease the
> > > > effective_wal_level. REPACK CONCURRENTLY uses a temporary slot that is
> > > > dropped at the time of cleanup, but logical decoding is not disabled.
> > > >
> > > > This may be related to both commits, 28d534e and 67c2097
> > > >
> > > > The attached patch adds the `RequestDisableLogicalDecoding` call to
> > > > `repack_cleanup_logical_decoding` after the replication slot is
> > > > dropped so the checkpointer will take care of it..
> > > >
> >
> > Good catch!
> >
> > >
> > > Thanks for reporting the issue. I agree with both the problem
> > > statement and the proposed fix.
> > >
> > > The fix LGTM. The only point I’d like to discuss is whether it would
> > > make more sense for RequestDisableLogicalDecoding() to be called
> > > directly from ReplicationSlotDropAcquired().
> > >
> > > Currently, ReplicationSlotRelease(), ReplicationSlotDrop(), and now
> > > repack_cleanup_logical_decoding() all invoke
> > > RequestDisableLogicalDecoding() immediately after
> > > ReplicationSlotDropAcquired(). Given this pattern, it may be cleaner
> > > and less error-prone to make RequestDisableLogicalDecoding() part of
> > > ReplicationSlotDropAcquired() itself, which could also help avoid
> > > similar bugs in the future.
> > >
> > > That said, one concern is that ReplicationSlotsDropDBSlots() could end
> > > up issuing too many disable requests if there are many logical slots
> > > in the target database, so I’m not entirely sure whether this is the
> > > right direction. Thoughts?
> >
> > Good point. I think we can have ReplicationSlotDropAcquired() have a
> > flag to skip sending a deactivation request. That way,
> > ReplicationSlotsDropDBSlots() can check the logical slot presence
> > after processing all slots and other callers can request the
> > deactivation after dropping the slot. It would help simplify the code
> > somewhat. It's conventional that when dropping a slot we acquire the
> > slot first and call RepicationSlotDropAcquired() to reliably drop a
> > slot (ReplicationSlotCleanup() is an exception). Therefore, I think
> > that having a flag to ReplicationSlotDropAcquired() could help future
> > developers to make sure to disable logical decoding at the slot drop.
> >
>
> Yes, that seems like a good proposal. Having an explicit argument
> would require authors to consciously review and decide whether logical
> decoding should be disabled based on their specific use case. That
> would help prevent such bugs from being introduced unintentionally.
>
> thanks
> Shveta