public inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Imran Zaheer <imran.zhir@gmail.com>
To: shveta malik <shveta.malik@gmail.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Cc: shveta malik <shvetamalik@gmail.com>
Subject: Re: effective_wal_level is not decreasing after using REPACK (CONCURRENTLY)
Date: Sat, 23 May 2026 14:19:30 +0500
Message-ID: <CA+UBfakjtK=Dh9NQTf7g5F+eMoDbEQUSa8BN6RBGPFzGr3H-sA@mail.gmail.com> (raw)
In-Reply-To: <CAJpy0uDzMzyfc2VOt+H1B96mjRXjgJ8oVLatxkNjPHzpHuQifA@mail.gmail.com>
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>
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.malik@gmail.com> wrote:
>
> On Fri, May 22, 2026 at 11:40 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Thu, May 21, 2026 at 9:19 PM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > On Thu, May 21, 2026 at 10:02 PM Imran Zaheer <imran.zhir@gmail.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
Attachments:
[application/octet-stream] v2-0001-Disable-logical-decoding-after-REPACK-CONCURRENTL.patch (6.2K, 2-v2-0001-Disable-logical-decoding-after-REPACK-CONCURRENTL.patch)
download | inline diff:
From a605ebb3a6dc18b17ca1f7886dc007627b6cda0e Mon Sep 17 00:00:00 2001
From: Imran Zaheer <imran.zhir@gmail.com>
Date: Sat, 23 May 2026 14:02:04 +0500
Subject: [PATCH v2] 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 | 2 +-
src/backend/replication/slot.c | 25 ++++++++-----------
src/include/replication/slot.h | 2 +-
.../recovery/t/051_effective_wal_level.pl | 14 +++++++++++
6 files changed, 29 insertions(+), 18 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..fe09ee04b6e 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -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);
}
UnlockSharedObject(DatabaseRelationId, local_slot->data.database,
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 83fcde74718..a87ef9dd82b 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);
}
/*
@@ -1039,7 +1032,7 @@ ReplicationSlotAlter(const char *name, const bool *failover,
* Permanently drop the currently acquired replication slot.
*/
void
-ReplicationSlotDropAcquired(void)
+ReplicationSlotDropAcquired(bool disable_logical_decoding)
{
ReplicationSlot *slot = MyReplicationSlot;
@@ -1049,6 +1042,10 @@ ReplicationSlotDropAcquired(void)
MyReplicationSlot = NULL;
ReplicationSlotDropPtr(slot);
+
+ /* Request checkpointer to disable the logical decoding */
+ if (disable_logical_decoding)
+ RequestDisableLogicalDecoding();
}
/*
@@ -1606,7 +1603,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.49.0
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: imran.zhir@gmail.com, shveta.malik@gmail.com, sawada.mshk@gmail.com, shvetamalik@gmail.com
Subject: Re: effective_wal_level is not decreasing after using REPACK (CONCURRENTLY)
In-Reply-To: <CA+UBfakjtK=Dh9NQTf7g5F+eMoDbEQUSa8BN6RBGPFzGr3H-sA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox