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 <[email protected]> wrote: > > On Fri, May 22, 2026 at 11:40 AM Masahiko Sawada <[email protected]> > wrote: > > > > On Thu, May 21, 2026 at 9:19 PM shveta malik <[email protected]> wrote: > > > > > > On Thu, May 21, 2026 at 10:02 PM Imran Zaheer <[email protected]> > > > 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
v2-0001-Disable-logical-decoding-after-REPACK-CONCURRENTL.patch
Description: Binary data
