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 <[email protected]> wrote: > > On Sat, May 23, 2026 at 2:49 PM Imran Zaheer <[email protected]> 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
From 911412f9c76851685114ed2150fb8a908b07412e Mon Sep 17 00:00:00 2001 From: Imran Zaheer <[email protected]> Date: Tue, 26 May 2026 14:33:02 +0500 Subject: [PATCH v3] Disable logical decoding after REPACK (CONCURRENTLY) REPACK (CONCURRENTLY) drops a temporary logical replication slot but never calls RequestDisableLogicalDecoding(), so effective_wal_level remains stuck at 'logical'. Add a disable_logical_decoding flag to ReplicationSlotDropAcquired() so callers explicitly control this behavior. --- src/backend/commands/repack_worker.c | 2 +- src/backend/replication/logical/launcher.c | 2 +- src/backend/replication/logical/slotsync.c | 9 ++++- src/backend/replication/slot.c | 34 +++++++++++-------- src/include/replication/slot.h | 2 +- .../recovery/t/051_effective_wal_level.pl | 14 ++++++++ 6 files changed, 44 insertions(+), 19 deletions(-) diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c index b84041372b8..4f82eb46bec 100644 --- a/src/backend/commands/repack_worker.c +++ b/src/backend/commands/repack_worker.c @@ -323,7 +323,7 @@ repack_cleanup_logical_decoding(LogicalDecodingContext *ctx) ExecDropSingleTupleTableSlot(dstate->slot); FreeDecodingContext(ctx); - ReplicationSlotDropAcquired(); + ReplicationSlotDropAcquired(true); } /* diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c index 50051dea8c7..137da582fe2 100644 --- a/src/backend/replication/logical/launcher.c +++ b/src/backend/replication/logical/launcher.c @@ -1406,7 +1406,7 @@ ApplyLauncherMain(Datum main_arg) if (MyReplicationSlot) { if (!retain_dead_tuples) - ReplicationSlotDropAcquired(); + ReplicationSlotDropAcquired(false); else if (can_update_xmin) update_conflict_slot_xmin(xmin); } diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c index ad3747e598c..8b8b6388aed 100644 --- a/src/backend/replication/logical/slotsync.c +++ b/src/backend/replication/logical/slotsync.c @@ -567,7 +567,14 @@ drop_local_obsolete_slots(List *remote_slot_list) if (synced_slot) { ReplicationSlotAcquire(NameStr(local_slot->data.name), true, false); - ReplicationSlotDropAcquired(); + + /* + * 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. + */ + ReplicationSlotDropAcquired(false); } UnlockSharedObject(DatabaseRelationId, local_slot->data.database, diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 83fcde74718..2cb5de8150f 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -786,16 +786,12 @@ ReplicationSlotRelease(void) * Delete the slot. There is no !PANIC case where this is allowed to * fail, all that may happen is an incomplete cleanup of the on-disk * data. - */ - ReplicationSlotDropAcquired(); - - /* - * Request to disable logical decoding, even though this slot may not - * have been the last logical slot. The checkpointer will verify if + * + * Also request to disable logical decoding, even though this slot may + * not have been the last logical slot. The checkpointer will verify if * logical decoding should actually be disabled. */ - if (is_logical) - RequestDisableLogicalDecoding(); + ReplicationSlotDropAcquired(is_logical); } /* @@ -937,10 +933,7 @@ ReplicationSlotDrop(const char *name, bool nowait) is_logical = SlotIsLogical(MyReplicationSlot); - ReplicationSlotDropAcquired(); - - if (is_logical) - RequestDisableLogicalDecoding(); + ReplicationSlotDropAcquired(is_logical); } /* @@ -1036,19 +1029,30 @@ ReplicationSlotAlter(const char *name, const bool *failover, } /* - * Permanently drop the currently acquired replication slot. + * Permanently drop the currently acquired replication slot and + * request the checkpointer to disable logical decoding if requested + * by the caller. */ void -ReplicationSlotDropAcquired(void) +ReplicationSlotDropAcquired(bool disable_logical_decoding) { ReplicationSlot *slot = MyReplicationSlot; Assert(MyReplicationSlot != NULL); + /* + * Ensure that slot is logical if caller has requested to disable + * logical decoding + */ + Assert(!disable_logical_decoding || SlotIsLogical(MyReplicationSlot)); + /* slot isn't acquired anymore */ MyReplicationSlot = NULL; ReplicationSlotDropPtr(slot); + + if (disable_logical_decoding) + RequestDisableLogicalDecoding(); } /* @@ -1606,7 +1610,7 @@ restart: * beginning each time we release the lock. */ LWLockRelease(ReplicationSlotControlLock); - ReplicationSlotDropAcquired(); + ReplicationSlotDropAcquired(false); dropped = true; goto restart; } diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h index 77c8d0975b6..55e66e827a6 100644 --- a/src/include/replication/slot.h +++ b/src/include/replication/slot.h @@ -335,7 +335,7 @@ extern void ReplicationSlotCreate(const char *name, bool db_specific, bool synced); extern void ReplicationSlotPersist(void); extern void ReplicationSlotDrop(const char *name, bool nowait); -extern void ReplicationSlotDropAcquired(void); +extern void ReplicationSlotDropAcquired(bool disable_logical_decoding); extern void ReplicationSlotAlter(const char *name, const bool *failover, const bool *two_phase); diff --git a/src/test/recovery/t/051_effective_wal_level.pl b/src/test/recovery/t/051_effective_wal_level.pl index c4c2662f72b..663ed730c91 100644 --- a/src/test/recovery/t/051_effective_wal_level.pl +++ b/src/test/recovery/t/051_effective_wal_level.pl @@ -141,6 +141,20 @@ test_wal_level($primary, "replica|replica", "effective_wal_level got decreased to 'replica' after invalidating the last logical slot" ); +# Logical decoding should be disabled after repacking +$primary->safe_psql('postgres', qq[create table foo(a int primary key)]); +$primary->safe_psql('postgres', qq[repack (concurrently) foo;]); +ok( $primary->log_contains( + "logical decoding is enabled upon creating a new logical replication slot" + ), + "logical decoding has been enabled upon creating a temp slot"); + +# Wait for the checkpointer to disable logical decoding. +wait_for_logical_decoding_disabled($primary); +test_wal_level($primary, "replica|replica", + "effective_wal_level got decreased to 'replica' after the REPACK (CONCURRENTLY) command" +); + # Revert the modified settings, and restart the server. $primary->adjust_conf('postgresql.conf', 'max_slot_wal_keep_size', undef); $primary->adjust_conf('postgresql.conf', 'min_wal_size', undef); -- 2.34.1
