On Wed, Sep 16, 2026 at 2:07 AM shveta malik <[email protected]> wrote:
>
> On Wed, Sep 16, 2026 at 5:43 AM Masahiko Sawada <[email protected]> wrote:
> >
> > On Wed, Sep 9, 2026 at 3:21 PM Bharath Rupireddy
> > <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Sep 8, 2026 at 9:24 PM shveta malik <[email protected]>
> > > wrote:
> > > >
> > > > > There can be two cases for external modules implementing logical
> > > > > decoding functionality. A function that unknowingly forgets to call
> > > > > ReplicationSlotRelease(), and a function that intentionally holds the
> > > > > slot across subxact boundaries and releases it later in the top-level
> > > > > transaction. For example
> > > > >
> > > > > ```
> > > > > BeginInternalSubTransaction("xxx");
> > > > > ReplicationSlotAcquire(name, ...);
> > > > > <do something>
> > > > > ReleaseCurrentSubTransaction();
> > > > > <do more something>
> > > > > ReplicationSlotRelease();
> > > > > ```
> > > > >
> > > > > The above seems like a legitimate usage (though we don't know if there
> > > > > is any real user of this pattern today). We can't easily distinguish
> > > > > between the two cases in the subxact commit path. The first case is
> > > > > more of a coding and reviewing problem. In both cases, calling the
> > > > > function twice in a row would hit Assert(MyReplicationSlot == NULL) or
> > > > > silently overwrite the slot, but the intentional case must already be
> > > > > aware of this. Even if the core emits a WARNING and users report it,
> > > > > there may not be anything we can do about it. If they release the slot
> > > > > at the end of the function, it is not a problem. If they forget, they
> > > > > need to fix it themselves.
> > > > >
> > > > > Given all this, emitting a WARNING on a subxact commit may not seem
> > > > > right even on HEAD. Silently handing off the slot to the parent
> > > > > transaction on subxact commit seems like the better approach.
> > > > >
> > > >
> > > > I agree there could be such a scenario in the future, especially since
> > > > we don't document or define a rule that a slot must be released in the
> > > > same subtransaction where it was acquired. Even if no existing user
> > > > exposed slot-function does this today, an extension could.
> > > >
> > > > But I feel there should be at least some way to signal that there's a
> > > > chance of a slot leak, for the cases where it actually is one. How
> > > > about putting in a DEBUG message noting that the slot was retained
> > > > across a subxact boundary? Something like:
> > > >
> > > > elog(DEBUG1,
> > > > "replication slot \"%s\" acquired in subtransaction retained
> > > > across its commit; ownership transferred to parent",
> > > > NameStr(MyReplicationSlot->data.name));
> > >
> > > Upon thinking more and discussing off-list with Amit and Sawada-san,
> > > here is what I have. In the PG20+ branches, I added a WARNING and
> > > removed the assert while handing off the slot across subtransaction
> > > boundaries during commits.
> >
> > Thank you for updating the patch!
> >
> > > We do not know if there are any such
> > > legitimate uses, but if there are, those users would get the WARNING
> > > reported. On HEAD it is easier to remove the WARNING later if it feels
> > > annoying for such users. In the backbranches,
> > > AtEOSubXact_ReplicationSlot() is a no-op for commits because the
> > > WARNING may not be a good idea there, and we do not have a good use
> > > case for it on commits anyway. Hope this simplifies the fix.
> >
> > I looked at the back-branch ones and I think they have a problem that
> > the HEAD patch doesn't have. The back branches return early on subxact
> > commit:
> >
> > + if (isCommit)
> > + return;
> >
> > So once the subxact that acquired the slot commits,
> > MyReplicationSlotSubId keeps the id of a subxact that is already gone.
> > Subxact ids restart at TopSubTransactionId in every transaction since
> > StartTransaction() resets currentSubTransactionId, so the same id
> > comes around again.It's not a problem for the core use cases, but if
> > there is an external SQL function that keeps the slot when the
> > transaction ends, that stale id can match a completely unrelated
> > subxact in a later transaction and we release a slot that subxact
> > never acquired.
> >
> > What bothers me is that this pattern works today on all branches.
> > While I guess it's not a good programming practice, we don't restrict
> > such use cases. So I think it's not a case of not supporting that
> > usage, it's a behavior change we would be introducing in a minor
> > release.
> >
> > That makes me want to reconsider how we split the patches. IIUC the
> > handoff mechanism that the master patch implements is to (1) keep
> > MyReplicationSlotSubId from going stale and (2) give the slot a new
> > guarantee, that the slot is released if an ancestor subxact aborts,
> > which nothing does today. (2) is the part that broadens what an
> > extension can do whereas (1) is just cleaning up after the variable we
> > added. I think we can fix the reported problem only with (1) even
> > without (2). So I guess it would be cleaner to do (1) for all
> > branches, and do (2) only for master. As for (1), we can have a
> > function like AtEOXact_ReplicationSlot() just clearing
> > MyReplicationSlotSubId.
>
> Sawada-san, does that mean that on the back branches, even for the
> case where the concerned subtransaction is committing while the slot
> is still held (a scenario we don't know can happen), we would release
> the slot and clean up MyReplicationSlotSubId? Is my understanding
> correct?
I don't think we should release the slot at subxact commit. I think
it's better to leave it to the caller as it might release the slot
afterward. Another problem is that nothing tests this case.
Please refer to the attached patch that can be applied on v16 patch
and implements my idea. It adds additional regression tests too.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
diff --git a/contrib/test_decoding/expected/slot.out b/contrib/test_decoding/expected/slot.out
index e30778e7e99..f7906f1e4c4 100644
--- a/contrib/test_decoding/expected/slot.out
+++ b/contrib/test_decoding/expected/slot.out
@@ -530,3 +530,31 @@ SELECT pg_drop_replication_slot('regress_subxact_slot');
(1 row)
+-- Unlike a top-level error, an error caught in a subtransaction releases the
+-- slot but leaves the session's temporary slots in place.
+SELECT 'init' FROM pg_create_logical_replication_slot('regress_subxact_temp_slot', 'test_decoding', true);
+ ?column?
+----------
+ init
+(1 row)
+
+DO $$
+BEGIN
+ PERFORM pg_replication_slot_advance('regress_subxact_temp_slot', '0/1');
+EXCEPTION WHEN OTHERS THEN
+ RAISE NOTICE 'caught SQLSTATE %', SQLSTATE;
+END $$;
+NOTICE: caught SQLSTATE 55000
+SELECT count(*) = 1 AS temp_slot_kept
+ FROM pg_replication_slots WHERE slot_name = 'regress_subxact_temp_slot';
+ temp_slot_kept
+----------------
+ t
+(1 row)
+
+SELECT pg_drop_replication_slot('regress_subxact_temp_slot');
+ pg_drop_replication_slot
+--------------------------
+
+(1 row)
+
diff --git a/contrib/test_decoding/sql/slot.sql b/contrib/test_decoding/sql/slot.sql
index 8f133c1faf4..bd292c41e9b 100644
--- a/contrib/test_decoding/sql/slot.sql
+++ b/contrib/test_decoding/sql/slot.sql
@@ -225,3 +225,16 @@ SELECT active FROM pg_replication_slots WHERE slot_name = 'regress_subxact_slot'
SELECT count(*) >= 0 AS peek_ok
FROM pg_logical_slot_peek_changes('regress_subxact_slot', NULL, NULL);
SELECT pg_drop_replication_slot('regress_subxact_slot');
+
+-- Unlike a top-level error, an error caught in a subtransaction releases the
+-- slot but leaves the session's temporary slots in place.
+SELECT 'init' FROM pg_create_logical_replication_slot('regress_subxact_temp_slot', 'test_decoding', true);
+DO $$
+BEGIN
+ PERFORM pg_replication_slot_advance('regress_subxact_temp_slot', '0/1');
+EXCEPTION WHEN OTHERS THEN
+ RAISE NOTICE 'caught SQLSTATE %', SQLSTATE;
+END $$;
+SELECT count(*) = 1 AS temp_slot_kept
+ FROM pg_replication_slots WHERE slot_name = 'regress_subxact_temp_slot';
+SELECT pg_drop_replication_slot('regress_subxact_temp_slot');
diff --git a/doc/src/sgml/func/func-admin.sgml b/doc/src/sgml/func/func-admin.sgml
index 64b0e7bb972..5b250f46ed2 100644
--- a/doc/src/sgml/func/func-admin.sgml
+++ b/doc/src/sgml/func/func-admin.sgml
@@ -1051,7 +1051,10 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
parameter, <parameter>temporary</parameter>, when set to true, specifies that
the slot should not be permanently stored to disk and is only meant
for use by the current session. Temporary slots are also
- released upon any error. This function corresponds
+ dropped on any error. An error raised and caught in a
+ subtransaction, for example by a
+ <application>PL/pgSQL</application> exception block, does not
+ drop them. This function corresponds
to the replication protocol command <literal>CREATE_REPLICATION_SLOT
... PHYSICAL</literal>.
</para></entry>
@@ -1091,7 +1094,10 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
parameter, <parameter>temporary</parameter>, when set to true, specifies that
the slot should not be permanently stored to disk and is only meant
for use by the current session. Temporary slots are also
- released upon any error. The optional fourth parameter,
+ dropped on any error. An error raised and caught in a
+ subtransaction, for example by a
+ <application>PL/pgSQL</application> exception block, does not
+ drop them. The optional fourth parameter,
<parameter>twophase</parameter>, when set to true, specifies
that the decoding of prepared transactions is enabled for this
slot. The optional fifth parameter,
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 2dd0bd9ce8d..6efb0c8f1ad 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -50,6 +50,7 @@
#include "replication/logicallauncher.h"
#include "replication/logicalworker.h"
#include "replication/origin.h"
+#include "replication/slot.h"
#include "replication/snapbuild.h"
#include "replication/syncrep.h"
#include "storage/aio_subsys.h"
@@ -5203,8 +5204,7 @@ CommitSubTransaction(void)
s->parent->curTransactionOwner);
AtEOSubXact_LargeObject(true, s->subTransactionId,
s->parent->subTransactionId);
- AtEOSubXact_ReplicationSlot(true, s->subTransactionId,
- s->parent->subTransactionId);
+ AtEOSubXact_ReplicationSlot(true, s->subTransactionId);
AtSubCommit_Notify();
CallSubXactCallbacks(SUBXACT_EVENT_COMMIT_SUB, s->subTransactionId,
@@ -5382,8 +5382,7 @@ AbortSubTransaction(void)
s->parent->curTransactionOwner);
AtEOSubXact_LargeObject(false, s->subTransactionId,
s->parent->subTransactionId);
- AtEOSubXact_ReplicationSlot(false, s->subTransactionId,
- s->parent->subTransactionId);
+ AtEOSubXact_ReplicationSlot(false, s->subTransactionId);
AtSubAbort_Notify();
/* Advertise the fact that we aborted in pg_xact. */
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index d67ec8db86d..adfa3152f71 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -40,6 +40,7 @@
#include <sys/stat.h>
#include "access/transam.h"
+#include "access/xact.h"
#include "access/xlog_internal.h"
#include "access/xlogrecovery.h"
#include "common/file_utils.h"
@@ -864,13 +865,11 @@ ReplicationSlotRelease(void)
}
/*
- * At subxact end, hand off or release MyReplicationSlot if it was acquired
- * in this subxact. On commit, ownership passes to the parent subxact; on
- * abort, the slot is released.
+ * At subxact end, release the replication slot if the subtransaction
+ * where the slot was acquired is aborted.
*/
void
-AtEOSubXact_ReplicationSlot(bool isCommit, SubTransactionId mySubid,
- SubTransactionId parentSubid)
+AtEOSubXact_ReplicationSlot(bool isCommit, SubTransactionId mySubid)
{
/* Nothing to do unless the slot was acquired in this subxact. */
if (MyReplicationSlotSubId != mySubid)
@@ -879,18 +878,12 @@ AtEOSubXact_ReplicationSlot(bool isCommit, SubTransactionId mySubid,
/*
* The subxact that acquired the slot is committing with the slot still
* held. No slot function in the core code does that today, though an
- * extension might. We don't know of a reason to keep a slot past the
- * commit of the subxact that acquired it, and the slot was most likely
- * left unreleased by mistake, so warn. If a real use case turns up, the
- * WARNING can be dropped. Either way, hand the slot to the parent so it
- * is still released if an ancestor aborts.
+ * extension might. A slot left unreleased by mistake cannot be told apart
+ * from one the caller means to release later. Leave it to the caller.
*/
if (isCommit)
{
- elog(WARNING, "replication slot \"%s\" acquired in a subtransaction is still held at its commit; ownership transferred to the parent",
- NameStr(MyReplicationSlot->data.name));
-
- MyReplicationSlotSubId = parentSubid;
+ MyReplicationSlotSubId = InvalidSubTransactionId;
return;
}
diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h
index 31f8755dc39..171ba6f2318 100644
--- a/src/include/replication/slot.h
+++ b/src/include/replication/slot.h
@@ -343,8 +343,7 @@ extern void ReplicationSlotAcquire(const char *name, bool nowait,
bool error_if_invalid);
extern void ReplicationSlotRelease(void);
extern void ReplicationSlotCleanup(bool synced_only);
-extern void AtEOSubXact_ReplicationSlot(bool isCommit, SubTransactionId mySubid,
- SubTransactionId parentSubid);
+extern void AtEOSubXact_ReplicationSlot(bool isCommit, SubTransactionId mySubid);
extern void ReplicationSlotSave(void);
extern void ReplicationSlotMarkDirty(void);
