Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

> On Sep 16, 2026, at 14:34, shveta malik  wrote:
> 
> On Wed, Sep 16, 2026 at 11:25 AM Chao Li  wrote:
>> 
>> 
>> 
>>> On Sep 10, 2026, at 06:21, Bharath Rupireddy  wrote:
>>> 
>>> Hi,
>>> 
>>> On Tue, Sep 8, 2026 at 9:24 PM shveta malik  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, ...);
>>>>> 
>>>>> ReleaseCurrentSubTransaction();
>>>>> 
>>>>> 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. 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 used similar wording to the above for the WARNING.
>>> 
>>> Please find the attached v16 patches prepared for all the supported branches.
>>> 
>>> --
>>> Bharath Rupireddy
>>> Amazon Web Services: https://aws.amazon.com
>>> 
>> 
>> I just reviewed v16 and have one concern.
>> 
>> The comment explicitly says that temporary slots are left in place. For already-created temporary slots, that sounds reasonable. But what if the creation of a temporary slot fails within the subtransaction? For example:
>> ```
>> evantest=# DO $$
>> evantest$# BEGIN
>> evantest$#     PERFORM pg_create_logical_replication_slot(
>> evantest$#         'tmp_bad',
>> evantest$#         'definitely_not_allowed',
>> evantest$#         true
>> evantest$#     );
>> evantest$# EXCEPTION WHEN OTHERS THEN
>> evantest$#     RAISE NOTICE 'caught SQLSTATE %', SQLSTATE;
>> evantest$# END
>> evantest$# $$;
>> NOTICE:  caught SQLSTATE 42501
>> DO
>> evantest=#
>> evantest=# SELECT slot_name,
>> evantest-#        plugin,
>> evantest-#        temporary,
>> evantest-#        active,
>> evantest-#        active_pid,
>> evantest-#        restart_lsn,
>> evantest-#        confirmed_flush_lsn,
>> evantest-#        catalog_xmin
>> evantest-# FROM pg_replication_slots
>> evantest-# WHERE slot_name = 'tmp_bad';
>> slot_name |         plugin         | temporary | active | active_pid | restart_lsn | confirmed_flush_lsn | catalog_xmin
>> -----------+------------------------+-----------+--------+------------+-------------+---------------------+--------------
>> tmp_bad   | definitely_not_allowed | t         | t      |       9668 | 0/01BFA5A8  |                     |          665
>> (1 row)
>> ```
>> 
>> With a bad plugin, creation of the temporary slot fails, but the partially initialized slot remains after the error is caught. It remains until the session terminates, or it’s dropped explicitly. For a long-lived or pooled session, its restart_lsn continues to participate in ReplicationSlotsComputeRequiredLSN(), potentially causing unnecessary WAL retention.
>> 
>> Therefore, should we distinguish a successfully created temporary slot from one whose creation is still in progress when the sub-transaction aborts, and drop the latter?
> 
> We had discussed this already, please see the email at [1] and the
> responses to it. Since this issue is not new (it exists for other
> slots too), it was decided to consider it separately on HEAD.
> 
> [1]: https://www.postgresql.org/message-id/CAJpy0uAwKM%3DLbnNp0rMevtCD9ub8zcADE9X1Z-PLwTmqFadgCQ%40mail.gmail.com
> 
> Thanks
> Shveta

Thanks for the explanation. Then would it make sense to add a brief description for that in the commit message?

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

> On Sep 10, 2026, at 06:21, Bharath Rupireddy  wrote:
> 
> Hi,
> 
> On Tue, Sep 8, 2026 at 9:24 PM shveta malik  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, ...);
>>> 
>>> ReleaseCurrentSubTransaction();
>>> 
>>> 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. 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 used similar wording to the above for the WARNING.
> 
> Please find the attached v16 patches prepared for all the supported branches.
> 
> -- 
> Bharath Rupireddy
> Amazon Web Services: https://aws.amazon.com
> 

I just reviewed v16 and have one concern.

The comment explicitly says that temporary slots are left in place. For already-created temporary slots, that sounds reasonable. But what if the creation of a temporary slot fails within the subtransaction? For example:
```
evantest=# DO $$
evantest$# BEGIN
evantest$#     PERFORM pg_create_logical_replication_slot(
evantest$#         'tmp_bad',
evantest$#         'definitely_not_allowed',
evantest$#         true
evantest$#     );
evantest$# EXCEPTION WHEN OTHERS THEN
evantest$#     RAISE NOTICE 'caught SQLSTATE %', SQLSTATE;
evantest$# END
evantest$# $$;
NOTICE:  caught SQLSTATE 42501
DO
evantest=#
evantest=# SELECT slot_name,
evantest-#        plugin,
evantest-#        temporary,
evantest-#        active,
evantest-#        active_pid,
evantest-#        restart_lsn,
evantest-#        confirmed_flush_lsn,
evantest-#        catalog_xmin
evantest-# FROM pg_replication_slots
evantest-# WHERE slot_name = 'tmp_bad';
 slot_name |         plugin         | temporary | active | active_pid | restart_lsn | confirmed_flush_lsn | catalog_xmin
-----------+------------------------+-----------+--------+------------+-------------+---------------------+--------------
 tmp_bad   | definitely_not_allowed | t         | t      |       9668 | 0/01BFA5A8  |                     |          665
(1 row)
```

With a bad plugin, creation of the temporary slot fails, but the partially initialized slot remains after the error is caught. It remains until the session terminates, or it’s dropped explicitly. For a long-lived or pooled session, its restart_lsn continues to participate in ReplicationSlotsComputeRequiredLSN(), potentially causing unnecessary WAL retention.

Therefore, should we distinguish a successfully created temporary slot from one whose creation is still in progress when the sub-transaction aborts, and drop the latter?

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Sorry for the late response.

I may be misunderstanding the patch, but it looks to me as if it is
trying to handle several different classes of problems uniformly by
wrapping them in exception handling.

ReplicationSlotAcquire() can exit with an ERROR after setting
MyReplicationSlot. I think it should restore that state itself.  Or
rather, perhaps it should not set MyReplicationSlot until it is
certain that the slot can be successfully acquired.  I think the same
kind of argument applies to ReplicationSlotCreate().

There is also a different issue around pgstat_create_replslot().  If
ReplicationSlotCreate() calls it and then an error is thrown later in
the caller, the caller needs to clean up that pgstat entry.  That
seems like a separate kind of cleanup from restoring
MyReplicationSlot.

In pg_logical_slot_get_changes_guts(), the patch moves the call to
ReplicationSlotAcquire() inside the PG_TRY block.  In practice the
code checks whether MyReplicationSlot is NULL before releasing it, so
this may work.  However, semantically, it looks as if the caller is
also responsible for releasing the slot even when
ReplicationSlotAcquire() itself fails.

More generally, I am not sure that all of these cleanup actions belong
at the same level.

Restoring MyReplicationSlot, cleaning up pgstat entry, and releasing
an acquired slot seem to be different kinds of
responsibilities. Handling them all through the same PG_TRY/PG_CATCH
blocks causes those cleanup responsibilities to cross component
boundaries. That, in turn, makes it harder to tell whether all
resources affected by an error are actually being cleaned up.

Regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Wed, Sep 16, 2026 at 5:43 AM Masahiko Sawada  wrote:
>
> 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. For (2), we can prepare a separate patch that
> implements the handoff mechanism (possibly with a WARNING or DEBUG
> message) with the regression tests, if we want to support these cases.
>
> It seems confusing and I might be too pessimistic as this is all about
> hypothetical cases that might not exist, but I'd like to keep the
> back-branch fix to the smallest thing that fixes only the reported
> problem while not changing other current behaviors.
>

Yeah, we can do a minimal fix for back-branches on the lines you are
suggesting but OTOH, I think we are over worried about the
hypothetical cases. I feel there is no harm in keeping the HEAD and
back-branches code/behavior same, in the worst case, if we get any
report, we can address keeping the actual usage in mind.

-- 
With Regards,
Amit Kapila.

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Thu, May 28, 2026 at 10:11 PM SATYANARAYANA NARLAPURAM
 wrote:
>
> Hi
>
> On Thu, May 28, 2026 at 9:17 PM Fujii Masao  wrote:
>>
>> On Thu, May 28, 2026 at 10:11 AM SATYANARAYANA NARLAPURAM
>>  wrote:
>> > Thanks for the patches, I combined these changes in my latest patch. Please find the v5.
>>
>> Thanks for updating the patch! But, v5 patch caused a compilation failure.
>>
>> slotfuncs.c:119:32: error: too few arguments to function call, single
>> argument 'try_disable' was not specified
>>   119 |                         ReplicationSlotDropAcquired();
>>       |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
>> ../../../src/include/replication/slot.h:338:13: note:
>> 'ReplicationSlotDropAcquired' declared here
>>   338 | extern void ReplicationSlotDropAcquired(bool try_disable);
>>       |             ^                           ~~~~~~~~~~~~~~~~
>> slotfuncs.c:207:32: error: too few arguments to function call, single
>> argument 'try_disable' was not specified
>>   207 |                         ReplicationSlotDropAcquired();
>>       |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
>> ../../../src/include/replication/slot.h:338:13: note:
>> 'ReplicationSlotDropAcquired' declared here
>>   338 | extern void ReplicationSlotDropAcquired(bool try_disable);
>>       |             ^                           ~~~~~~~~~~~~~~~~
>> slotfuncs.c:922:32: error: too few arguments to function call, single
>> argument 'try_disable' was not specified
>>   922 |                         ReplicationSlotDropAcquired();
>>       |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
>> ../../../src/include/replication/slot.h:338:13: note:
>> 'ReplicationSlotDropAcquired' declared here
>>   338 | extern void ReplicationSlotDropAcquired(bool try_disable);
>>       |             ^                           ~~~~~~~~~~~~~~~~
>> 3 errors generated.
>
>
> Please see the v6 patch. Upstream commit 2af1dc89282 changed the ReplicationSlotDropAcquired signature since the patch generated.
>

I've reviewed the v6 patch, and here are some comments:

Maybe pg_sync_replication_slots() has the same problem if it's called
inside an exception block?

---
The patch adds PG_TRY()/PG_CATCH() to each replication slot function,
but there is no comment explaining why we need them even though we
call ReplicationSlotRelease() and ReplicationSlotCleanup() in error
paths. Also, while probably the proposed idea works for back branches,
I guess we might want to consider more comprehensive approach for HEAD
to deal with this issue as it seems to me very error-prone,
especially when adding a new replication slot function.

This problem stems from the fact that when replication slots are used
within an exception block, we don't reach the error path even if an
error occurs, and we cannot simply call ReplicationSlotRelease()
during subtransaction abort as we need to start and abort transaction
while holding a slot. But I guess that we can release the slot when
aborting the subtransaction or above (sub)transaction where we
created/acquired the slot. I've drafted the patch for this idea and
confirmed it passes the regression tests, I still need to verify its
feasibility though.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Wed, Aug 19, 2026 at 11:26 PM Bharath Rupireddy
 wrote:
>
> Hi,
>
> On Wed, Aug 19, 2026 at 4:55 PM Masahiko Sawada  wrote:
> >
> > Thank you for updating the patch!
> >
> > I reviewed the v12 patch and here are some review comments:
>
> Thanks for reviewing it.
>
> > +static SubTransactionId acquiredInSubId = InvalidSubTransactionId;
> >
> > I'm not sure this variable name is ideal, since "acquired..." can be
> > read as a boolean. How about something like MyReplicationSlotSubid or
> > slotAcquireSubid?
>
> MyReplicationSlotSubId looks better, so I used that.
>
> > +/*
> > + * Release the replication slot at subxact end if it was acquired here.
> > + *
> > + * handled. The subxact id is used rather than a nesting level because levels
> > + * are reused across subxacts while ids are not.
> > + */
> >
> > I don't think it's the right place to explain the bug in detail, and
> > mentioning AtEOSubXact_LargeObject() seems unnecessary. How about
> > rewriting it to something like:
> >
> > /*
> >  * 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 (a dnthe sessions' temp slots dropped).
> > */
>
> WFM. Used the above comment.
>
> > +   /*
> > +    * The aborting subxact is the one that acquired the slot, so the slot is
> > +    * still held and must be released. acquiredInSubId is set only when a
> > +    * slot is held and cleared when it is released, so a matching subxact id
> > +    * means the slot is ours.
> > +    */
> > +   ReplicationSlotRelease();
> >
> > We should add an assertion that MyReplicationSlot is not NULL before this call.
>
> The slot release function already has an assertion. We discussed this
> upthread and agreed on the comment wording and not to have an
> additional assertion here. Does that work for you?

Yes, I agree.

>
> > AtEOSubXact_ReplicationSlot() performs the same slot cleanup (release
> > + drop temporary slots) that the error path in PostgresMain() does. It
> > would be good to add a note around the
> > ReplicationSlotRelease()/ReplicationSlotCleanup() calls in postgres.c
> > so that any future change there is also considered for
> > AtEOSubXact_ReplicationSlot() (and vice versa).
>
> Sounds good. Reworded these comments.
>
> Please find the attached v13 patch. I verified that the same issue
> exists all the way back to PG14. I want to backport it with the
> reproducers, since it is a bug that can be reproduced with simple SQL
> queries and can cause crashes, slot leaks and vacuum issues. If v13
> looks good, I can prepare patches for back branches and send them.
> Thoughts?

I think we need to carefully think about whether we drop all temp
slots at subxact abort and when we do that.

+   /*
+    * Also drop this session's temporary slots, as the top-level error
+    * handler in PostgresMain() does (keep the two in sync). Otherwise a
+    * temporary slot could be left behind holding back WAL removal and the
+    * catalog xmin after an error.

If we drop temp slots here at all, I agree it should be all of them
rather than just the held one. But I'm not us re we want to drop them
here at all.

+    *
+    * Note that this only runs when the aborting subxact held a slot. A
+    * caught error that held no slot (for example an unrelated error caught
+    * by a PL/pgSQL EXCEPTION clause) does not drop the session's temporary
+    * slots, unlike a top-level error, which always does.

I'm concerned that this is confusing from the user perspective: temp
slots are cleaned up when an error happens during slot manipulation
but not when a non-slot-related error (like PERFORM 1/0) is caught.
Further, even the former is ambiguous. If
pg_replication_slot_advance() is called with a non-existent slot name,
the function raises an error but doesn't clean up temp slots because
we don't acquire any slots. So whether the session's temp slot
survives depends on which error a slot function happens to raise,
which the user cannot predict.

+    * slots, unlike a top-level error, which always does. Covering that too
+    * would mean running the cleanup on every aborting subxact, which is
+    * harder to reason about, so it is kept here after the release.

True.

+    *
+    * Note also that we could instead keep the temporary slots and treat the
+    * error as recoverable, since the subxact was caught and the session goes
+    * on. But the error may be in the slot handling itself, leaving the slot
+    * in a doubtful state, so dropping it is the safer choice.

I don't think it could be the reason, because we leave a persistent
slot alone even when an error happens while handling it.

+    */
+   ReplicationSlotCleanup(false);

Also, is calling ReplicationSlotCleanup() (and possibly
ReplicationSlotRelease()) during sub-transaction abort really safe in
the first place? That function could raise errors and we call it out
of transactions elsewhere.

I'm inclined to think AtEOSubXact_ReplicationSlot() should only
release the slot and not call ReplicationSlotCleanup() at all. That
said, I'm not fully convinced of this either as it might be
inconsistent with top-level error cases in a sense.

Regards,

--
Masahiko Sawada

Amazon Web Services: https://aws.amazon.com

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Wed, Sep 9, 2026 at 3:21 PM Bharath Rupireddy
 wrote:
>
> Hi,
>
> On Tue, Sep 8, 2026 at 9:24 PM shveta malik  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, ...);
> > > 
> > > ReleaseCurrentSubTransaction();
> > > 
> > > 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. For (2), we can prepare a separate patch that
implements the handoff mechanism (possibly with a WARNING or DEBUG
message) with the regression tests, if we want to support these cases.

It seems confusing and I might be too pessimistic as this is all about
hypothetical cases that might not exist, but I'd like to keep the
back-branch fix to the smallest thing that fixes only the reported
problem while not changing other current behaviors.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Wed, Sep 16, 2026 at 2:07 AM shveta malik  wrote:
>
> On Wed, Sep 16, 2026 at 5:43 AM Masahiko Sawada  wrote:
> >
> > On Wed, Sep 9, 2026 at 3:21 PM Bharath Rupireddy
> >  wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Sep 8, 2026 at 9:24 PM shveta malik  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, ...);
> > > > > 
> > > > > ReleaseCurrentSubTransaction();
> > > > > 
> > > > > 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

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Mon, Aug 31, 2026 at 1:47 PM Bharath Rupireddy
 wrote:
>
> Hi,
>
> On Sun, Aug 30, 2026 at 9:40 PM Amit Kapila  wrote:
> >
> > > Hi Amit, By restricting in the code, does that mean adding an Assert,
> > > or a WARNING, or a WARNING plus slot release (not an error), in the
> > > replication slot subxact callback on the commit path, instead of
> > > handing the slot off to the parent across the subtransaction boundary?
> >
> > Yes, I would prefer WARNING similar to existing cases for resource
> > leaks in commit paths. One example of a similar existing case is:
> > ------
> > /* Complain if any allocated files remain open at commit. */
> > if (isCommit && numAllocatedDescs > 0)
> > elog(WARNING, "%d temporary files and directories not closed at
> > end-of-transaction",
> > numAllocatedDescs);
> > -------
> >
> > Based on above, I am imagining a check/WARNING on lines of:
>
> Thanks, Amit. That works for me. Please find the attached v15 patch.
> If it looks good, I can prepare patches for the back branches.
>

Thank you for updating the patch! Here are some review comments:

+   if (isCommit)
+   {
+       Assert(MyReplicationSlot != NULL);
+       ereport(WARNING,
+               (errcode(ERRCODE_WARNING),
+                errmsg("subtransaction left replication slot \"%s\" acquired",
+                       NameStr(MyReplicationSlot->data.name)),
+                errhint("Check for missing \"ReplicationSlotRelease\"
calls.")));

The hint message "Check for missing ReplicationSlotRelease call" seems
to be for us (PostgreSQL hackers) but not users. I think such messages
should be left as a comment instead of in errhint.

Also, we don't prohibit external extensions or functions to commit a
subtransaction while holding a replication slot. If there are such
extensions, users would get WARNING messages. Which seems to be
something I'd like to avoid in minor releases.

---
+-- Error raised inside a PL/pgSQL block with an EXCEPTION clause is caught in a
+-- subtransaction; the slot must still be released.
+SELECT 'init' FROM
pg_create_logical_replication_slot('regress_subxact_slot',
'test_decoding');
+DO $$
+BEGIN
+    PERFORM pg_replication_slot_advance('regress_subxact_slot', '0/1');
+EXCEPTION WHEN OTHERS THEN
+    RAISE NOTICE 'caught SQLSTATE %', SQLSTATE;
+END $$;
+SELECT count(*) >= 0 AS peek_ok
+    FROM pg_logical_slot_peek_changes('regress_subxact_slot', NULL, NULL);

The last sentence is the comment doesn't match the test well and this
test doesn't fail on non-assertion builds. I think we can check the
active column in pg_replication_slots instead or before
slot_peek_changes() call.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Thu, Aug 27, 2026 at 9:19 PM shveta malik  wrote:
>
> On Thu, Aug 27, 2026 at 6:00 PM Amit Kapila  wrote:
> >
> > On Thu, Aug 27, 2026 at 11:58 AM Masahiko Sawada  wrote:
> > >
> > > On Mon, Aug 24, 2026 at 3:29 PM Bharath Rupireddy
> > >  wrote:
> > > >
> > > >
> > > > In short, having just the slot release in the subxact path gives the
> > > > same error behavior, is simple to reason about, and fixes the crash
> > > > reported in this thread.
> > >
> > > One thing I'm a bit concerned about is that this would be the first
> > > caller to invoke ReplicationSlotRelease() from inside the transaction
> > > machinery.
> > >
> >
> > True, but OTOH, won't we already clean up resources not directly
> > associated with subxact in AtEOSubXact_LargeObject() or
> > AtEOSubXact_Files()? I don't see any problem as far as the current
> > pattern of usage for slots.
>
> I agree.
>
> > The new restriction this patch will add is
> > "a slot acquired in a subxact does not survive that subxact being
> > unwound." which should be okay because of its similarity with
> > top-level xact behavior. I feel if possible we should restrict such
> > usage explicitly in code in some way rather than one finding out this
> > as a surprise.
> >
> > *
> > An error raised and caught in a
> > +          subtransaction, for example by a
> > +          PL/pgSQL exception block, does not drop
> > +          them.
> >
> > Based on above, something like below won't clean up temp slots and end
> > up holding xmin.
> >   DO $$ BEGIN
> >     PERFORM pg_create_logical_replication_slot('s', 'nonexistent_plugin', true);
> >   EXCEPTION WHEN OTHERS THEN RAISE NOTICE '%', SQLERRM;
> >   END $$;
>
> Well, on rethinking, I feel that if we encounter an error while
> creating a slot, whether persistent or temporary, the slot should be
> dropped right there.
>
> This already works correctly for persistent slots: by the time the
> slot reaches ReplicationSlotRelease, it is still in RS_EPHEMERAL state
> and is therefore dropped by release. OTIOH, a temporary slot is left
> behind. I think the temporary slot should also be dropped because the
> caller never received a reference to it. I don't see a legitimate use
> case where a temp slot should survive specifically because its
> creation call failed.

While I agree that it would be an ideal behavior and the analysis
holds for logical slots, I want to note that persistent physical
replication slots are created with RS_PERSISTENT so if an error
happens during the slot creation the slot is left behind. Also,
logical persistent slots actually have the same gap: if
ReplicationSlotPersist() raises an error it leaves a persistent slot
behind as well. Given that slot creation and drop are not
transactional operations, and that leaving a slot behind on a failure
is not a new behavior, I'm inclined toward only releasing the slot at
the subxact abort. We can discuss the better behavior on HEAD
separately.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Wed, Aug 12, 2026 at 8:31 PM shveta malik  wrote:
>
> On Thu, Aug 13, 2026 at 7:01 AM Bharath Rupireddy
>  wrote:
> >
> > Hi,
> >
> > On Mon, Aug 10, 2026 at 11:16 PM shveta malik  wrote:
> > >
> > > > I will drop both asserts and
> > > > keep a short comment explaining why the slot is still held here. The
> > > > existing AtEOSubXact_LargeObject() and AtEOSubXact_Files() don't check
> > > > the passed-in mySubid for invalid either.
> > > >
> > > > Does the following work for you?
> > > >
> > > > /*
> > > > * The aborting subxact is the one that acquired the slot, so the slot is
> > > > * still held and must be released. acquiredInSubId is set only when a slot
> > > > * is held and cleared when it is released, so a matching subxact id means
> > > > * the slot is ours.
> > > > */
> > > > ReplicationSlotRelease();
> > >
> > > I am okay with this comment. No 'MyReplicationSlot-null' check and no assert.
> >
> > Thanks. Done so in the attached v12 patch. Please have a look.
> >
>
> Thanks. Looks good. I have no further comments.

Thank you for updating the patch!

I reviewed the v12 patch and here are some review comments:

+static SubTransactionId acquiredInSubId = InvalidSubTransactionId;

I'm not sure this variable name is ideal, since "acquired..." can be
read as a boolean. How about something like MyReplicationSlotSubid or
slotAcquireSubid?

---
+/*
+ * Release the replication slot at subxact end if it was acquired here.
+ *
+ * A slot function acquires a slot and releases it before returning. On error
+ * the top-level error handler releases it. But PL/pgSQL, PL/Perl,
PL/Python and
+ * PL/Tcl run an error-handling block in an internal subxact, and when an error
+ * there is caught the top-level handler is never reached, so the slot would
+ * otherwise stay acquired. Release it when the subxact that acquired
it aborts,
+ * the same way AtEOSubXact_LargeObject() and other subxact-scoped
resources are
+ * handled. The subxact id is used rather than a nesting level because levels
+ * are reused across subxacts while ids are not.
+ */

I don't think it's the right place to explain the bug in detail, and
mentioning AtEOSubXact_LargeObject() seems unnecessary. How about
rewriting it to something like:

/*
 * 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 (a dnthe sessions' temp slots dropped).
*/

---
+   /*
+    * The aborting subxact is the one that acquired the slot, so the slot is
+    * still held and must be released. acquiredInSubId is set only when a
+    * slot is held and cleared when it is released, so a matching subxact id
+    * means the slot is ours.
+    */
+   ReplicationSlotRelease();

We should add an assertion that MyReplicationSlot is not NULL before this call.

---
AtEOSubXact_ReplicationSlot() performs the same slot cleanup (release
+ drop temporary slots) that the error path in PostgresMain() does. It
would be good to add a note around the
ReplicationSlotRelease()/ReplicationSlotCleanup() calls in postgres.c
so that any future change there is also considered for
AtEOSubXact_ReplicationSlot() (and vice versa).

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Fri, Sep 18, 2026 at 9:49 AM Bharath Rupireddy
 wrote:
>
> Hi,
>
> On Fri, Sep 18, 2026 at 1:31 AM kedar anavardekar
>  wrote:
> >
> > Two minor naming suggestions: (please take the suggestions if you
> > think the points are valid)
>
> Thanks for taking a look.
>
> > 1. Could MyReplicationSlotSubId be renamed to
> > MyReplicationSlotSubXactId (or MyReplicationSlotSubTransactionId)
> >
> > SubId may be read as a subscription ID, whereas this variable stores
> > the SubTransactionId of the subtransaction that acquired
> > MyReplicationSlot. The more explicit name would make its purpose
> > clearer and avoid confusion with logical replication subscriptions.
>
> Subscription and its related replication slot on the publisher are on
> two different database instances, and one has the context when reading
> the code around this. Also, "SubId" is used across the code base and I
> want to keep it consistent and short, so MyReplicationSlotSubId looks
> fine to me.

+1

> > 2., could the comment above AtEOSubXact_ReplicationSlot() be revised from:
> > /*
> >  * At subxact end, release the replication slot if the subtransaction
> >  * where the slot was acquired is aborted.
> >  */
> > to:
> > /*
> >  * At subxact end, release the replication slot if the subtransaction
> >  * in which the slot was acquired is aborted.
> >  */
> > “In which” is more precise here because the slot is acquired during
> > that subtransaction.
>
> I believe "where the slot was acquired" is grammatically correct as
> well, so I'm fine with the existing wording.

I reviewed the v17 patch and here are some comments:

         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
+        PL/pgSQL exception block, does not
+        drop them. This function corresponds

ISTM what the following sentence says seems to contradict with what
the first sentence says. How about rephrasing it to:

         for use by the current session. Temporary slots are also
-        released upon any error. This function corresponds
+        dropped when an error is reported to the client. An error
caught inside a
+        subtransaction, for example by a PL/pgSQL
+        exception block, does not drop them. This function corresponds

---
+-- A slot function that errors out must still release the slot, otherwise the
+-- next slot operation in the session fails an assertion or leaks the slot.
+-- Advancing a freshly created slot to a low LSN always errors.
+SELECT 'init' FROM
pg_create_logical_replication_slot('regress_subxact_slot',
'test_decoding');

The comment seems not to be in the right place; it's in right before
the pg_create_logical_replication_slot() call but not related. Given
that we have the comments for subsequent tests, we can remove it.

I've attached the updated patch that incorporated the above comments.
I'm going to push it early next week, barring any objections.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Wed, May 27, 2026 at 8:00 PM shveta malik  wrote:
> pg_copy_physical_replication_slot() should not have it as the common
> 'copy_replication_slot' is already fixed in the patch.

copy_replication_slot() calls create_physical_replication_slot() before
entering the PG_TRY/PG_CATCH block. So if create_physical_replication_slot()
throws an error, wouldn't the same issue still occur?

>  I will review
> the others.

Thanks!

Regards,

-- 
Fujii Masao

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Wed, May 27, 2026 at 1:31 PM SATYANARAYANA NARLAPURAM
 wrote:
> Thank you for the changes and review.

When I applied the v4 patch together with Shveta's diff patch and
ran the regression tests, the tests failed.

Could pg_create_physical_replication_slot() still have the same issue
if it throws an error after ReplicationSlotCreate() and that error is
caught by a PL/pgSQL EXCEPTION block?

Also, do maybe pg_copy_physical_replication_slot(), pg_drop_replication_slot(),
and ALTER_REPLICATION_SLOT potentially have the same issue as well?

Regards,

-- 
Fujii Masao

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Thu, May 28, 2026 at 10:11 AM SATYANARAYANA NARLAPURAM
 wrote:
> Thanks for the patches, I combined these changes in my latest patch. Please find the v5.

Thanks for updating the patch! But, v5 patch caused a compilation failure.

slotfuncs.c:119:32: error: too few arguments to function call, single
argument 'try_disable' was not specified
  119 |                         ReplicationSlotDropAcquired();
      |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
../../../src/include/replication/slot.h:338:13: note:
'ReplicationSlotDropAcquired' declared here
  338 | extern void ReplicationSlotDropAcquired(bool try_disable);
      |             ^                           ~~~~~~~~~~~~~~~~
slotfuncs.c:207:32: error: too few arguments to function call, single
argument 'try_disable' was not specified
  207 |                         ReplicationSlotDropAcquired();
      |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
../../../src/include/replication/slot.h:338:13: note:
'ReplicationSlotDropAcquired' declared here
  338 | extern void ReplicationSlotDropAcquired(bool try_disable);
      |             ^                           ~~~~~~~~~~~~~~~~
slotfuncs.c:922:32: error: too few arguments to function call, single
argument 'try_disable' was not specified
  922 |                         ReplicationSlotDropAcquired();
      |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
../../../src/include/replication/slot.h:338:13: note:
'ReplicationSlotDropAcquired' declared here
  338 | extern void ReplicationSlotDropAcquired(bool try_disable);
      |             ^                           ~~~~~~~~~~~~~~~~
3 errors generated.

Regards,

-- 
Fujii Masao

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Thu, Aug 6, 2026 at 1:32 PM Bharath Rupireddy
 wrote:
>
> Hi,
>
> On Wed, Aug 5, 2026 at 8:46 PM shveta malik  wrote:
> >
> > Thanks Bharath. A few trivial comments:
> >
> > 1)
> > + * We must not get here while decoding is running. Decoding starts and
> > + * aborts an internal (sub)transaction while holding the slot, for each
> > + * decoded transaction (ReorderBufferProcessTXN()) and when executing
> > + * invalidations (ReorderBufferImmediateInvalidation()), but that always
> > + * happens below the acquiring subxact, so those aborts have a different
> > + * (deeper) id and do not match here. Decoding also runs with a historic
> > + * snapshot set up, so assert that it is not.
> >
> > It is slightly difficult to understand this comment. What does 'below'
> > mean? Shall we rephrase 'but that always...' to:
> >
> > However, those subtransactions are always nested below the subtransaction that
> > acquired the slot, so their subtransaction IDs are deeper and therefore do not
> > match here. Decoding also ....
> >
> > (I hope your comment meant this, else let me know)
>
> That's right. TXN -> SUBTXN1 (acquires the slot) -> SUBTXN2 (internal
> subxact started during decoding in ReorderBufferProcessTXN()), and
> while in SUBTXN2, the historic snapshot is held. Your wording looks
> fine to me.
>
> > 2)
> > +-- Test 3: same as Test 1 for a temporary slot. Releasing a temporary slot on
> > +-- error does not drop it, so it would keep holding back WAL removal and the
> > +-- catalog xmin. The session's temporary slots are dropped as well, so none is
> > +-- left behind.
> >
> > The comment is slightly confusing. We are intititally saying 'it does
> > not drop temp-slot' and then saying 'it is dropped'. Do we want to
> > distinguish the sentences as old and post-patch behaviour somehow?
>
> I wanted to say the difference between slot release and cleanup there.
> I simplified it as follows, and the comments in
> AtEOSubXact_ReplicationSlot() have a detailed explanation anyway.
>
> +-- Test 3: same as Test 1 for a temporary slot, which is dropped rather than
> +-- just released, so it is not left behind after the error.
>

Okay, I get it now. Thanks for the making the change.

v10 compiles without these inclusions. Can you please check?

xact.c:
+#include "replication/slot.h"

slot.c:
+#include "access/xact.h"


thanks
Shveta

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Wed, May 27, 2026 at 1:42 PM Fujii Masao  wrote:
>
> On Wed, May 27, 2026 at 1:31 PM SATYANARAYANA NARLAPURAM
>  wrote:
> > Thank you for the changes and review.
>
> When I applied the v4 patch together with Shveta's diff patch and
> ran the regression tests, the tests failed.

That is because my top-up patch lacks slot.out changes, I wanted Satya
to first confirm if the changes are acceptable to him. Attached
another top-up patch for test-output correction.

> Could pg_create_physical_replication_slot() still have the same issue
> if it throws an error after ReplicationSlotCreate() and that error is
> caught by a PL/pgSQL EXCEPTION block?
>
> Also, do maybe pg_copy_physical_replication_slot(), pg_drop_replication_slot(),
> and ALTER_REPLICATION_SLOT potentially have the same issue as well?
>

pg_copy_physical_replication_slot() should not have it as the common
'copy_replication_slot' is already fixed in the patch.  I will review
the others.

thanks
Shveta

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Sat, Aug 29, 2026 at 4:42 AM Bharath Rupireddy
 wrote:
>
> Hi,
>
> On Fri, Aug 28, 2026 at 3:49 PM Masahiko Sawada  wrote:
> >
> > On Thu, Aug 27, 2026 at 9:19 PM shveta malik  wrote:
> > >
> > > On Thu, Aug 27, 2026 at 6:00 PM Amit Kapila  wrote:
> > > >
> > > > On Thu, Aug 27, 2026 at 11:58 AM Masahiko Sawada  wrote:
> > > > >
> > > > > On Mon, Aug 24, 2026 at 3:29 PM Bharath Rupireddy
> > > > >  wrote:
> > > > > >
> > > > > >
> > > > > > In short, having just the slot release in the subxact path gives the
> > > > > > same error behavior, is simple to reason about, and fixes the crash
> > > > > > reported in this thread.
> > > > >
> > > > > One thing I'm a bit concerned about is that this would be the first
> > > > > caller to invoke ReplicationSlotRelease() from inside the transaction
> > > > > machinery.
> > > > >
> > > >
> > > > True, but OTOH, won't we already clean up resources not directly
> > > > associated with subxact in AtEOSubXact_LargeObject() or
> > > > AtEOSubXact_Files()? I don't see any problem as far as the current
> > > > pattern of usage for slots.
> > >
> > > I agree.
> > >
> > > > The new restriction this patch will add is
> > > > "a slot acquired in a subxact does not survive that subxact being
> > > > unwound." which should be okay because of its similarity with
> > > > top-level xact behavior. I feel if possible we should restrict such
> > > > usage explicitly in code in some way rather than one finding out this
> > > > as a surprise.
> > > >
> > > > *
> > > > An error raised and caught in a
> > > > +          subtransaction, for example by a
> > > > +          PL/pgSQL exception block, does not drop
> > > > +          them.
> > > >
> > > > Based on above, something like below won't clean up temp slots and end
> > > > up holding xmin.
> > > >   DO $$ BEGIN
> > > >     PERFORM pg_create_logical_replication_slot('s', 'nonexistent_plugin', true);
> > > >   EXCEPTION WHEN OTHERS THEN RAISE NOTICE '%', SQLERRM;
> > > >   END $$;
> > >
> > > Well, on rethinking, I feel that if we encounter an error while
> > > creating a slot, whether persistent or temporary, the slot should be
> > > dropped right there.
> > >
> > > This already works correctly for persistent slots: by the time the
> > > slot reaches ReplicationSlotRelease, it is still in RS_EPHEMERAL state
> > > and is therefore dropped by release. OTIOH, a temporary slot is left
> > > behind. I think the temporary slot should also be dropped because the
> > > caller never received a reference to it. I don't see a legitimate use
> > > case where a temp slot should survive specifically because its
> > > creation call failed.
> >
> > While I agree that it would be an ideal behavior and the analysis
> > holds for logical slots, I want to note that persistent physical
> > replication slots are created with RS_PERSISTENT so if an error
> > happens during the slot creation the slot is left behind. Also,
> > logical persistent slots actually have the same gap: if
> > ReplicationSlotPersist() raises an error it leaves a persistent slot
> > behind as well. Given that slot creation and drop are not
> > transactional operations, and that leaving a slot behind on a failure
> > is not a new behavior, I'm inclined toward only releasing the slot at
> > the subxact abort. We can discuss the better behavior on HEAD
> > separately.
>
> Yes, I realised the same. The ephemeral state only applies to logical
> slots, not to physical slots or temporary slots.
>
> I agree to keep the back-branch fix simple and solve the slot leak and
> crash reported in this thread. However, I think the creation failure
> on temporary slots inside a subxact also needs to be fixed in the back
> branches (perhaps separately), because one can hit the issue with
> direct SQL.

Do you mean one "cannot" hit the issue with direct SQL?

> A temporary slot whose creation fails needs to be dropped,
> to avoid leaking resources for a slot the caller never got a reference
> to.
>
> In the replication slot subxact callback, on the abort path, we need
> to know whether the slot's creation failed. Ephemeral slots already
> handle that, but only for persistent logical slots. A temporary slot
> stays RS_TEMPORARY throughout. So there are a few ways to solve this:
>
> 1/ Also mark temporary slots as ephemeral initially and transition
> them to RS_TEMPORARY once creation succeeds. A quick check shows this
> needs changes in many places.
> 2/ Introduce a new state to represent a temporary slot still in
> creation (RS_TEMPORARY_EPHEMERAL or such).
> 3/ Use a boolean in the ReplicationSlot structure
> (is_create_in_progress or such), and in the subxact callback, when the
> slot is temporary and is_create_in_progress is set, drop just that
> temporary slot and leave the others alone.
>
> I prefer option 3, to keep it simple without adding a new state, and
> because it is back-branch friendly. The new boolean lives only in
> memory and is not written to disk. To drop a single temporary slot,
> I'm thinking of moving the single-slot drop code out of
> ReplicationSlotCleanup() into an internal helper function.

My preference will be option 1 but it needs careful checking for
existing usages.

> Thoughts?
>
> On Thu, Aug 27, 2026 at 5:30 AM Amit Kapila  wrote:
> >
> > True, but OTOH, won't we already clean up resources not directly
> > associated with subxact in AtEOSubXact_LargeObject() or
> > AtEOSubXact_Files()? I don't see any problem as far as the current
> > pattern of usage for slots. The new restriction this patch will add is
> > "a slot acquired in a subxact does not survive that subxact being
> > unwound." which should be okay because of its similarity with
> > top-level xact behavior. I feel if possible we should restrict such
> > usage explicitly in code in some way rather than one finding out this
> > as a surprise.
>
> Hi Amit, By restricting in the code, does that mean adding an Assert,
> or a WARNING, or a WARNING plus slot release (not an error), in the
> replication slot subxact callback on the commit path, instead of
> handing the slot off to the parent across the subtransaction boundary?
>
> --
> Bharath Rupireddy
> Amazon Web Services: https://aws.amazon.com

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Sat, Aug 29, 2026 at 4:19 AM Masahiko Sawada  wrote:
>
> On Thu, Aug 27, 2026 at 9:19 PM shveta malik  wrote:
> >
> > On Thu, Aug 27, 2026 at 6:00 PM Amit Kapila  wrote:
> > >
> > > On Thu, Aug 27, 2026 at 11:58 AM Masahiko Sawada  wrote:
> > > >
> > > > On Mon, Aug 24, 2026 at 3:29 PM Bharath Rupireddy
> > > >  wrote:
> > > > >
> > > > >
> > > > > In short, having just the slot release in the subxact path gives the
> > > > > same error behavior, is simple to reason about, and fixes the crash
> > > > > reported in this thread.
> > > >
> > > > One thing I'm a bit concerned about is that this would be the first
> > > > caller to invoke ReplicationSlotRelease() from inside the transaction
> > > > machinery.
> > > >
> > >
> > > True, but OTOH, won't we already clean up resources not directly
> > > associated with subxact in AtEOSubXact_LargeObject() or
> > > AtEOSubXact_Files()? I don't see any problem as far as the current
> > > pattern of usage for slots.
> >
> > I agree.
> >
> > > The new restriction this patch will add is
> > > "a slot acquired in a subxact does not survive that subxact being
> > > unwound." which should be okay because of its similarity with
> > > top-level xact behavior. I feel if possible we should restrict such
> > > usage explicitly in code in some way rather than one finding out this
> > > as a surprise.
> > >
> > > *
> > > An error raised and caught in a
> > > +          subtransaction, for example by a
> > > +          PL/pgSQL exception block, does not drop
> > > +          them.
> > >
> > > Based on above, something like below won't clean up temp slots and end
> > > up holding xmin.
> > >   DO $$ BEGIN
> > >     PERFORM pg_create_logical_replication_slot('s', 'nonexistent_plugin', true);
> > >   EXCEPTION WHEN OTHERS THEN RAISE NOTICE '%', SQLERRM;
> > >   END $$;
> >
> > Well, on rethinking, I feel that if we encounter an error while
> > creating a slot, whether persistent or temporary, the slot should be
> > dropped right there.
> >
> > This already works correctly for persistent slots: by the time the
> > slot reaches ReplicationSlotRelease, it is still in RS_EPHEMERAL state
> > and is therefore dropped by release. OTIOH, a temporary slot is left
> > behind. I think the temporary slot should also be dropped because the
> > caller never received a reference to it. I don't see a legitimate use
> > case where a temp slot should survive specifically because its
> > creation call failed.
>
> While I agree that it would be an ideal behavior and the analysis
> holds for logical slots, I want to note that persistent physical
> replication slots are created with RS_PERSISTENT so if an error
> happens during the slot creation the slot is left behind. Also,
> logical persistent slots actually have the same gap: if
> ReplicationSlotPersist() raises an error it leaves a persistent slot
> behind as well. Given that slot creation and drop are not
> transactional operations, and that leaving a slot behind on a failure
> is not a new behavior, I'm inclined toward only releasing the slot at
> the subxact abort. We can discuss the better behavior on HEAD
> separately.
>

Okay, fair enough. I agree.

thanks
Shveta

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Tue, May 26, 2026 at 1:41 PM SATYANARAYANA NARLAPURAM
 wrote:
>
> Hi,
>
> On Mon, May 25, 2026 at 10:50 PM shveta malik  wrote:
>>
>> On Tue, May 26, 2026 at 10:06 AM shveta malik  wrote:
>> >
>> > On Tue, May 26, 2026 at 12:31 AM SATYANARAYANA NARLAPURAM
>> >  wrote:
>> > >
>> > > Hi,
>> > >
>> > > On Mon, May 25, 2026 at 2:58 AM shveta malik  wrote:
>> > >>
>> > >> On Mon, May 25, 2026 at 12:42 PM SATYANARAYANA NARLAPURAM
>> > >>  wrote:
>> > >> >
>> > >> > Hi
>> > >> >
>> > >> > On Fri, May 22, 2026 at 2:16 AM shveta malik  wrote:
>> > >> >>
>> > >> >> Thanks for reporting the issue. I could reproduce the same issue with
>> > >> >> all these as well:
>> > >> >>
>> > >> >> pg_logical_slot_peek_changes
>> > >> >> pg_logical_slot_get_binary_changes
>> > >> >> pg_logical_slot_peek_binary_changes
>> > >> >
>> > >> >
>> > >> > Please find the attached v2 patch that addressed these three cases as well.
>> > >> >
>> > >>
>> > >> Thank You for addressuing these cases. A few comments:
>> > >>
>> > >> 1)
>> > >>
>> > >> +-- Test 2: session remains usable after the error (MyReplicationSlot cleared)
>> > >>
>> > >> It shoudl be part of 'Test 1' itself and thus should not be named as 'Test 2'
>> > >>
>> > >> 2)
>> > >> --------
>> > >> +-- Test 4: copy_replication_slot with max_replication_slots exceeded.
>> > >> +-- We reduce max_replication_slots artificially by filling all remaining slots.
>> > >> +-- Instead, trigger an error by copying to an already-existing name.
>> > >> +DO $$
>> > >> +BEGIN
>> > >> +    PERFORM pg_copy_logical_replication_slot('regression_slot_t3',
>> > >> 'regression_slot_t3');
>> > >> +EXCEPTION WHEN OTHERS THEN
>> > >> +    RAISE NOTICE 'caught: %', SQLERRM;
>> > >> +END;
>> > >> +$$;
>> > >> +-- The original slot must still exist and be usable
>> > >> +SELECT count(*) = 1 AS orig_slot_ok FROM pg_replication_slots
>> > >> +    WHERE slot_name = 'regression_slot_t3';
>> > >> -----------
>> > >>
>> > >> I don't think we can hit the Assert with above test (at-least I could
>> > >> not). Since creation of slot itself will fail as the slot with
>> > >> same-name already exists, MyReplicationSlot will never be set and thus
>> > >> Assert will not be hit.  A better testcase will be below which fails
>> > >> during LoadOutputPlugin() after slot-creation and MyReplicationSlot is
>> > >> set already.
>> > >>
>> > >> SELECT pg_create_logical_replication_slot('src_slot', 'test_decoding');
>> > >>
>> > >> DO $$
>> > >> BEGIN
>> > >> PERFORM pg_copy_logical_replication_slot('src_slot', 'dst_slot',
>> > >> false, 'nonexistent_plugin');
>> > >> EXCEPTION WHEN others THEN
>> > >> RAISE NOTICE 'caught: %', SQLERRM;
>> > >> END $$;
>> > >>
>> > >> SELECT count(*) FROM pg_logical_slot_get_changes('src_slot', NULL, NULL);
>> > >>
>> > >> 3)
>> > >> So overall these are the problematic APIs:
>> > >>
>> > >> pg_create_logical_replication_slot
>> > >> pg_replication_slot_advance
>> > >> pg_copy_logical_replication_slot
>> > >> pg_logical_slot_peek_binary_changes
>> > >> pg_logical_slot_peek_changes
>> > >> pg_logical_slot_get_changes
>> > >> pg_logical_slot_get_binary_changes
>> > >>
>> > >> First 3 are are mutually exclusive fixes fow which we have added
>> > >> testcases. Last 4 are addressed by fixing common function
>> > >> pg_logical_slot_get_changes_guts(). I think we should add a test case
>> > >> for at-least any one of these APIs to cover
>> > >> pg_logical_slot_get_changes_guts().
>> > >
>> > >
>> > > Thanks for reviewing. Please review the attached v3 patch.
>> > >
>> >
>> > A few trivial things:
>> >
>> > 1)
>> > pg_replication_slot_advance:
>> > + PG_TRY();
>> > + {
>> > + /* Acquire the slot so we "own" it */
>> > + ReplicationSlotAcquire(NameStr(*slotname), true, true);
>> > + /* A slot whose restart_lsn has never been reserved cannot be advanced */
>> > + if (!XLogRecPtrIsValid(MyReplicationSlot->data.restart_lsn))
>> >
>> >
>> > We can have a blank line after ReplicationSlotAcquire for better readability.
>> >
>> > 2)
>> >
>> > +SELECT 'init' FROM
>> > pg_create_logical_replication_slot('regression_slot_t3',
>> > 'test_decoding', true);
>> > +SELECT count(*) = 1 AS slot_exists FROM pg_replication_slots
>> > +    WHERE slot_name = 'regression_slot_t3';
>> >
>> > The intent is not clear why are we checking existence of
>> > regression_slot_t3? I think we can skip it (or else add a comment if
>> > really needed). The success of previous
>> > pg_create_logical_replication_slot is enough to confirm that session
>> > is healthy to run other slot related queries.
>> >
>> > 3)
>> > +SELECT pg_drop_replication_slot('regression_slot_phy');
>> > +
>> > +-- cleanup
>> > +SELECT pg_drop_replication_slot('regression_slot_t3');
>> >
>> > We can move drop of 'regression_slot_phy' too under '-- cleanup'
>> >
>> > ~~
>> >
>> > I have no further comments other than the trivial things mentioned above.
>> >
>>
>> Missed to inform this earlier, I am not able to apply any version of
>> the patches shared so far with 'git am'. It gives error, 'patch -p1'
>> works.
>>
>> git am v3-0001-Release-replication-slot-on-error-in-slot-SQL-functions.patch
>> Patch format detection failed.
>
> Thanks , Shveta! Please find the attached v4 patch that addressed your comments.
>

Thank You for the patch.
I noticed that we are creating regression_slot_t3 as a a temporary
slot, is that intentional? I think creating a permanent slot here will
be a better testcase.

I have made a few cosmetic changes for better readability along with
creating the 'permanent' regression_slot_t3 slot. Please incorporate
what you think is okay. I have no more comments.

thanks
Shveta

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Tue, Aug 4, 2026 at 7:51 AM Bharath Rupireddy
 wrote:
>
> Hi,
>
> On Sun, Aug 2, 2026 at 10:28 PM shveta malik  wrote:
> >
> > Bharath, I could not find any issue in my basic testing,
>
> Thanks, Shveta, for reviewing!
>
> > although I
> > would like to understand this part a bit better:
> >
> > + if (isCommit)
> > + {
> > + acquiredInSubId = parentSubid;
> > + return;
> > + }
> >
> > How can we reach this block? In a non-error scenario, it seems that by
> > the time AtEOSubXact_ReplicationSlot() is invoked, the slot has
> > already been released, and we return earlier from 'if (acquiredInSubId
> > != mySubid)' block. I could not find a case where:
> >
> > a) the slot is acquired in the current subtransaction, b) the
> > subtransaction commits (isCommit == true), and c) the slot is still
> > held when AtEOSubXact_ReplicationSlot() is invoked.
> >
> > Could you please explain what I am missing?
>
> Yes, no caller hits this today. Each slot function releases the slot
> before returning, so we take the early return and never reach the
> commit branch with a slot still held. It is there for a future slot
> function that acquires the slot but never releases it (rare case).

Okay, so we are preparing for a future scenario where a slot may live
across subtransaction boundaries, although that is not possible at the
moment. I'm not sure whether we really need to handle that case right
now; perhaps this should simply be an Assert() for the time being (not
a strong opinion though). At the very least, we should update the
comment to mention this rationale. And let's see what others think
about it.

> On
> commit we pass the slot to the parent so that if an ancestor later
> aborts, the slot still gets released, the same way
> AtEOSubXact_LargeObject() and AtEOSubXact_Files() do.
>
> I checked this locally by making the pg_replication_slot_advance()
> return while still holding the slot and running it through nested
> PL/pgSQL exception blocks. With the handoff the outer abort releases
> the slot, and dropping the handoff makes the slot leak and hit the
> same issue reported in this thread. I can either enhance the comment
> there to say it's currently unreachable and why we keep it, or turn it
> into an assertion.
>
> Thoughts?
>
> --
> Bharath Rupireddy
> Amazon Web Services: https://aws.amazon.com

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Mon, May 25, 2026 at 12:42 PM SATYANARAYANA NARLAPURAM
 wrote:
>
> Hi
>
> On Fri, May 22, 2026 at 2:16 AM shveta malik  wrote:
>>
>> Thanks for reporting the issue. I could reproduce the same issue with
>> all these as well:
>>
>> pg_logical_slot_peek_changes
>> pg_logical_slot_get_binary_changes
>> pg_logical_slot_peek_binary_changes
>
>
> Please find the attached v2 patch that addressed these three cases as well.
>

Thank You for addressuing these cases. A few comments:

1)

+-- Test 2: session remains usable after the error (MyReplicationSlot cleared)

It shoudl be part of 'Test 1' itself and thus should not be named as 'Test 2'

2)
--------
+-- Test 4: copy_replication_slot with max_replication_slots exceeded.
+-- We reduce max_replication_slots artificially by filling all remaining slots.
+-- Instead, trigger an error by copying to an already-existing name.
+DO $$
+BEGIN
+    PERFORM pg_copy_logical_replication_slot('regression_slot_t3',
'regression_slot_t3');
+EXCEPTION WHEN OTHERS THEN
+    RAISE NOTICE 'caught: %', SQLERRM;
+END;
+$$;
+-- The original slot must still exist and be usable
+SELECT count(*) = 1 AS orig_slot_ok FROM pg_replication_slots
+    WHERE slot_name = 'regression_slot_t3';
-----------

I don't think we can hit the Assert with above test (at-least I could
not). Since creation of slot itself will fail as the slot with
same-name already exists, MyReplicationSlot will never be set and thus
Assert will not be hit.  A better testcase will be below which fails
during LoadOutputPlugin() after slot-creation and MyReplicationSlot is
set already.

SELECT pg_create_logical_replication_slot('src_slot', 'test_decoding');

DO $$
BEGIN
PERFORM pg_copy_logical_replication_slot('src_slot', 'dst_slot',
false, 'nonexistent_plugin');
EXCEPTION WHEN others THEN
RAISE NOTICE 'caught: %', SQLERRM;
END $$;

SELECT count(*) FROM pg_logical_slot_get_changes('src_slot', NULL, NULL);

3)
So overall these are the problematic APIs:

pg_create_logical_replication_slot
pg_replication_slot_advance
pg_copy_logical_replication_slot
pg_logical_slot_peek_binary_changes
pg_logical_slot_peek_changes
pg_logical_slot_get_changes
pg_logical_slot_get_binary_changes

First 3 are are mutually exclusive fixes fow which we have added
testcases. Last 4 are addressed by fixing common function
pg_logical_slot_get_changes_guts(). I think we should add a test case
for at-least any one of these APIs to cover
pg_logical_slot_get_changes_guts().

Thanks.
Shveta

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Wed, Sep 16, 2026 at 11:25 AM Chao Li  wrote:
>
>
>
> > On Sep 10, 2026, at 06:21, Bharath Rupireddy  wrote:
> >
> > Hi,
> >
> > On Tue, Sep 8, 2026 at 9:24 PM shveta malik  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, ...);
> >>> 
> >>> ReleaseCurrentSubTransaction();
> >>> 
> >>> 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. 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 used similar wording to the above for the WARNING.
> >
> > Please find the attached v16 patches prepared for all the supported branches.
> >
> > --
> > Bharath Rupireddy
> > Amazon Web Services: https://aws.amazon.com
> > 
>
> I just reviewed v16 and have one concern.
>
> The comment explicitly says that temporary slots are left in place. For already-created temporary slots, that sounds reasonable. But what if the creation of a temporary slot fails within the subtransaction? For example:
> ```
> evantest=# DO $$
> evantest$# BEGIN
> evantest$#     PERFORM pg_create_logical_replication_slot(
> evantest$#         'tmp_bad',
> evantest$#         'definitely_not_allowed',
> evantest$#         true
> evantest$#     );
> evantest$# EXCEPTION WHEN OTHERS THEN
> evantest$#     RAISE NOTICE 'caught SQLSTATE %', SQLSTATE;
> evantest$# END
> evantest$# $$;
> NOTICE:  caught SQLSTATE 42501
> DO
> evantest=#
> evantest=# SELECT slot_name,
> evantest-#        plugin,
> evantest-#        temporary,
> evantest-#        active,
> evantest-#        active_pid,
> evantest-#        restart_lsn,
> evantest-#        confirmed_flush_lsn,
> evantest-#        catalog_xmin
> evantest-# FROM pg_replication_slots
> evantest-# WHERE slot_name = 'tmp_bad';
>  slot_name |         plugin         | temporary | active | active_pid | restart_lsn | confirmed_flush_lsn | catalog_xmin
> -----------+------------------------+-----------+--------+------------+-------------+---------------------+--------------
>  tmp_bad   | definitely_not_allowed | t         | t      |       9668 | 0/01BFA5A8  |                     |          665
> (1 row)
> ```
>
> With a bad plugin, creation of the temporary slot fails, but the partially initialized slot remains after the error is caught. It remains until the session terminates, or it’s dropped explicitly. For a long-lived or pooled session, its restart_lsn continues to participate in ReplicationSlotsComputeRequiredLSN(), potentially causing unnecessary WAL retention.
>
> Therefore, should we distinguish a successfully created temporary slot from one whose creation is still in progress when the sub-transaction aborts, and drop the latter?

We had discussed this already, please see the email at [1] and the
responses to it. Since this issue is not new (it exists for other
slots too), it was decided to consider it separately on HEAD.

[1]: https://www.postgresql.org/message-id/CAJpy0uAwKM%3DLbnNp0rMevtCD9ub8zcADE9X1Z-PLwTmqFadgCQ%40mail.gmail.com

thanks
Shveta

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Tue, May 26, 2026 at 12:31 AM SATYANARAYANA NARLAPURAM
 wrote:
>
> Hi,
>
> On Mon, May 25, 2026 at 2:58 AM shveta malik  wrote:
>>
>> On Mon, May 25, 2026 at 12:42 PM SATYANARAYANA NARLAPURAM
>>  wrote:
>> >
>> > Hi
>> >
>> > On Fri, May 22, 2026 at 2:16 AM shveta malik  wrote:
>> >>
>> >> Thanks for reporting the issue. I could reproduce the same issue with
>> >> all these as well:
>> >>
>> >> pg_logical_slot_peek_changes
>> >> pg_logical_slot_get_binary_changes
>> >> pg_logical_slot_peek_binary_changes
>> >
>> >
>> > Please find the attached v2 patch that addressed these three cases as well.
>> >
>>
>> Thank You for addressuing these cases. A few comments:
>>
>> 1)
>>
>> +-- Test 2: session remains usable after the error (MyReplicationSlot cleared)
>>
>> It shoudl be part of 'Test 1' itself and thus should not be named as 'Test 2'
>>
>> 2)
>> --------
>> +-- Test 4: copy_replication_slot with max_replication_slots exceeded.
>> +-- We reduce max_replication_slots artificially by filling all remaining slots.
>> +-- Instead, trigger an error by copying to an already-existing name.
>> +DO $$
>> +BEGIN
>> +    PERFORM pg_copy_logical_replication_slot('regression_slot_t3',
>> 'regression_slot_t3');
>> +EXCEPTION WHEN OTHERS THEN
>> +    RAISE NOTICE 'caught: %', SQLERRM;
>> +END;
>> +$$;
>> +-- The original slot must still exist and be usable
>> +SELECT count(*) = 1 AS orig_slot_ok FROM pg_replication_slots
>> +    WHERE slot_name = 'regression_slot_t3';
>> -----------
>>
>> I don't think we can hit the Assert with above test (at-least I could
>> not). Since creation of slot itself will fail as the slot with
>> same-name already exists, MyReplicationSlot will never be set and thus
>> Assert will not be hit.  A better testcase will be below which fails
>> during LoadOutputPlugin() after slot-creation and MyReplicationSlot is
>> set already.
>>
>> SELECT pg_create_logical_replication_slot('src_slot', 'test_decoding');
>>
>> DO $$
>> BEGIN
>> PERFORM pg_copy_logical_replication_slot('src_slot', 'dst_slot',
>> false, 'nonexistent_plugin');
>> EXCEPTION WHEN others THEN
>> RAISE NOTICE 'caught: %', SQLERRM;
>> END $$;
>>
>> SELECT count(*) FROM pg_logical_slot_get_changes('src_slot', NULL, NULL);
>>
>> 3)
>> So overall these are the problematic APIs:
>>
>> pg_create_logical_replication_slot
>> pg_replication_slot_advance
>> pg_copy_logical_replication_slot
>> pg_logical_slot_peek_binary_changes
>> pg_logical_slot_peek_changes
>> pg_logical_slot_get_changes
>> pg_logical_slot_get_binary_changes
>>
>> First 3 are are mutually exclusive fixes fow which we have added
>> testcases. Last 4 are addressed by fixing common function
>> pg_logical_slot_get_changes_guts(). I think we should add a test case
>> for at-least any one of these APIs to cover
>> pg_logical_slot_get_changes_guts().
>
>
> Thanks for reviewing. Please review the attached v3 patch.
>

A few trivial things:

1)
pg_replication_slot_advance:
+ PG_TRY();
+ {
+ /* Acquire the slot so we "own" it */
+ ReplicationSlotAcquire(NameStr(*slotname), true, true);
+ /* A slot whose restart_lsn has never been reserved cannot be advanced */
+ if (!XLogRecPtrIsValid(MyReplicationSlot->data.restart_lsn))


We can have a blank line after ReplicationSlotAcquire for better readability.

2)

+SELECT 'init' FROM
pg_create_logical_replication_slot('regression_slot_t3',
'test_decoding', true);
+SELECT count(*) = 1 AS slot_exists FROM pg_replication_slots
+    WHERE slot_name = 'regression_slot_t3';

The intent is not clear why are we checking existence of
regression_slot_t3? I think we can skip it (or else add a comment if
really needed). The success of previous
pg_create_logical_replication_slot is enough to confirm that session
is healthy to run other slot related queries.

3)
+SELECT pg_drop_replication_slot('regression_slot_phy');
+
+-- cleanup
+SELECT pg_drop_replication_slot('regression_slot_t3');

We can move drop of 'regression_slot_phy' too under '-- cleanup'

~~

I have no further comments other than the trivial things mentioned above.

thanks
Shveta

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Thanks for reporting the issue. I could reproduce the same issue with
all these as well:

pg_logical_slot_peek_changes
pg_logical_slot_get_binary_changes
pg_logical_slot_peek_binary_changes

thanks
Shveta

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Wed, May 27, 2026 at 5:40 PM Fujii Masao  wrote:
>
> On Wed, May 27, 2026 at 8:00 PM shveta malik  wrote:
> > pg_copy_physical_replication_slot() should not have it as the common
> > 'copy_replication_slot' is already fixed in the patch.
>
> copy_replication_slot() calls create_physical_replication_slot() before
> entering the PG_TRY/PG_CATCH block. So if create_physical_replication_slot()
> throws an error, wouldn't the same issue still occur?
>


You are right. Using v5, if I force create_physical_replication_slot()
to fail while executing pg_copy_physical_replication_slot() (through
debugging), I can see that the next slot-related call hits an Assert.

1)
I also noticed that for pg_copy_logical_replication_slot(), we do not
hit the CATCH block of copy_replication_slot(), but instead the one in
create_logical_replication_slot(). That behavior seems fine.

However, I noticed some inconsistencies in the implementation.

For create_physical_replication_slot(), the caller
pg_create_physical_replication_slot() contains the TRY-CATCH block,
whereas create_logical_replication_slot() contains its own TRY-CATCH
block internally.

I think it makes more sense to keep the TRY-CATCH handling inside the
internal functions, i.e. create_logical_replication_slot() and
create_physical_replication_slot(), since that would automatically
cover all callers. For example, create_logical_replication_slot() is
invoked from multiple places, so callers need not worry about cleanup
handling themselves. Similarly, for
create_physical_replication_slot(), we could move the TRY-CATCH block
inside the function instead of having it in
pg_create_physical_replication_slot(). Doing so would also resolve the
issue with pg_copy_physical_replication_slot().


2)
I also feel that ReplicationSlotCreate() should be moved inside the
TRY block in create_logical_replication_slot().

Currently, if in the future ReplicationSlotCreate() gains any
post-slot-creation implementation that could throw an error, we may
end up leaving the system in an unsafe state. Keeping it inside the
TRY block would make the code more robust against such future changes.
~~

Reveiwign further. Need to review few more things on v5/v6.

thanks
Shveta

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Thu, Sep 17, 2026 at 2:32 AM Masahiko Sawada  wrote:
>
> On Wed, Sep 16, 2026 at 2:07 AM shveta malik  wrote:
> >
> > On Wed, Sep 16, 2026 at 5:43 AM Masahiko Sawada  wrote:
> > >
> > > On Wed, Sep 9, 2026 at 3:21 PM Bharath Rupireddy
> > >  wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tue, Sep 8, 2026 at 9:24 PM shveta malik  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, ...);
> > > > > > 
> > > > > > ReleaseCurrentSubTransaction();
> > > > > > 
> > > > > > 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 agree. I was a bit surprised by what I understood, so I wanted to confirm.

> 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.
>

Right. I agree.

> Please refer to the attached patch that can be applied on v16 patch
> and implements my idea. It adds additional regression tests too.

The changes looks good.

> Regards,
>
> --
> Masahiko Sawada
> Amazon Web Services: https://aws.amazon.com

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Wed, Sep 16, 2026 at 5:43 AM Masahiko Sawada  wrote:
>
> On Wed, Sep 9, 2026 at 3:21 PM Bharath Rupireddy
>  wrote:
> >
> > Hi,
> >
> > On Tue, Sep 8, 2026 at 9:24 PM shveta malik  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, ...);
> > > > 
> > > > ReleaseCurrentSubTransaction();
> > > > 
> > > > 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?

> For (2), we can prepare a separate patch that
> implements the handoff mechanism (possibly with a WARNING or DEBUG
> message) with the regression tests, if we want to support these cases.
>
> It seems confusing and I might be too pessimistic as this is all about
> hypothetical cases that might not exist, but I'd like to keep the
> back-branch fix to the smallest thing that fixes only the reported
> problem while not changing other current behaviors.
>
> Regards,
>
> --
> Masahiko Sawada
> Amazon Web Services: https://aws.amazon.com

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Tue, May 26, 2026 at 10:06 AM shveta malik  wrote:
>
> On Tue, May 26, 2026 at 12:31 AM SATYANARAYANA NARLAPURAM
>  wrote:
> >
> > Hi,
> >
> > On Mon, May 25, 2026 at 2:58 AM shveta malik  wrote:
> >>
> >> On Mon, May 25, 2026 at 12:42 PM SATYANARAYANA NARLAPURAM
> >>  wrote:
> >> >
> >> > Hi
> >> >
> >> > On Fri, May 22, 2026 at 2:16 AM shveta malik  wrote:
> >> >>
> >> >> Thanks for reporting the issue. I could reproduce the same issue with
> >> >> all these as well:
> >> >>
> >> >> pg_logical_slot_peek_changes
> >> >> pg_logical_slot_get_binary_changes
> >> >> pg_logical_slot_peek_binary_changes
> >> >
> >> >
> >> > Please find the attached v2 patch that addressed these three cases as well.
> >> >
> >>
> >> Thank You for addressuing these cases. A few comments:
> >>
> >> 1)
> >>
> >> +-- Test 2: session remains usable after the error (MyReplicationSlot cleared)
> >>
> >> It shoudl be part of 'Test 1' itself and thus should not be named as 'Test 2'
> >>
> >> 2)
> >> --------
> >> +-- Test 4: copy_replication_slot with max_replication_slots exceeded.
> >> +-- We reduce max_replication_slots artificially by filling all remaining slots.
> >> +-- Instead, trigger an error by copying to an already-existing name.
> >> +DO $$
> >> +BEGIN
> >> +    PERFORM pg_copy_logical_replication_slot('regression_slot_t3',
> >> 'regression_slot_t3');
> >> +EXCEPTION WHEN OTHERS THEN
> >> +    RAISE NOTICE 'caught: %', SQLERRM;
> >> +END;
> >> +$$;
> >> +-- The original slot must still exist and be usable
> >> +SELECT count(*) = 1 AS orig_slot_ok FROM pg_replication_slots
> >> +    WHERE slot_name = 'regression_slot_t3';
> >> -----------
> >>
> >> I don't think we can hit the Assert with above test (at-least I could
> >> not). Since creation of slot itself will fail as the slot with
> >> same-name already exists, MyReplicationSlot will never be set and thus
> >> Assert will not be hit.  A better testcase will be below which fails
> >> during LoadOutputPlugin() after slot-creation and MyReplicationSlot is
> >> set already.
> >>
> >> SELECT pg_create_logical_replication_slot('src_slot', 'test_decoding');
> >>
> >> DO $$
> >> BEGIN
> >> PERFORM pg_copy_logical_replication_slot('src_slot', 'dst_slot',
> >> false, 'nonexistent_plugin');
> >> EXCEPTION WHEN others THEN
> >> RAISE NOTICE 'caught: %', SQLERRM;
> >> END $$;
> >>
> >> SELECT count(*) FROM pg_logical_slot_get_changes('src_slot', NULL, NULL);
> >>
> >> 3)
> >> So overall these are the problematic APIs:
> >>
> >> pg_create_logical_replication_slot
> >> pg_replication_slot_advance
> >> pg_copy_logical_replication_slot
> >> pg_logical_slot_peek_binary_changes
> >> pg_logical_slot_peek_changes
> >> pg_logical_slot_get_changes
> >> pg_logical_slot_get_binary_changes
> >>
> >> First 3 are are mutually exclusive fixes fow which we have added
> >> testcases. Last 4 are addressed by fixing common function
> >> pg_logical_slot_get_changes_guts(). I think we should add a test case
> >> for at-least any one of these APIs to cover
> >> pg_logical_slot_get_changes_guts().
> >
> >
> > Thanks for reviewing. Please review the attached v3 patch.
> >
>
> A few trivial things:
>
> 1)
> pg_replication_slot_advance:
> + PG_TRY();
> + {
> + /* Acquire the slot so we "own" it */
> + ReplicationSlotAcquire(NameStr(*slotname), true, true);
> + /* A slot whose restart_lsn has never been reserved cannot be advanced */
> + if (!XLogRecPtrIsValid(MyReplicationSlot->data.restart_lsn))
>
>
> We can have a blank line after ReplicationSlotAcquire for better readability.
>
> 2)
>
> +SELECT 'init' FROM
> pg_create_logical_replication_slot('regression_slot_t3',
> 'test_decoding', true);
> +SELECT count(*) = 1 AS slot_exists FROM pg_replication_slots
> +    WHERE slot_name = 'regression_slot_t3';
>
> The intent is not clear why are we checking existence of
> regression_slot_t3? I think we can skip it (or else add a comment if
> really needed). The success of previous
> pg_create_logical_replication_slot is enough to confirm that session
> is healthy to run other slot related queries.
>
> 3)
> +SELECT pg_drop_replication_slot('regression_slot_phy');
> +
> +-- cleanup
> +SELECT pg_drop_replication_slot('regression_slot_t3');
>
> We can move drop of 'regression_slot_phy' too under '-- cleanup'
>
> ~~
>
> I have no further comments other than the trivial things mentioned above.
>

Missed to inform this earlier, I am not able to apply any version of
the patches shared so far with 'git am'. It gives error, 'patch -p1'
works.

git am v3-0001-Release-replication-slot-on-error-in-slot-SQL-functions.patch
Patch format detection failed.

thanks
Shveta

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Hi Bharath,

Two minor naming suggestions: (please take the suggestions if you
think the points are valid)
1. Could MyReplicationSlotSubId be renamed to
MyReplicationSlotSubXactId (or MyReplicationSlotSubTransactionId)

SubId may be read as a subscription ID, whereas this variable stores
the SubTransactionId of the subtransaction that acquired
MyReplicationSlot. The more explicit name would make its purpose
clearer and avoid confusion with logical replication subscriptions.

2., could the comment above AtEOSubXact_ReplicationSlot() be revised from:
/*
 * At subxact end, release the replication slot if the subtransaction
 * where the slot was acquired is aborted.
 */
to:
/*
 * At subxact end, release the replication slot if the subtransaction
 * in which the slot was acquired is aborted.
 */
“In which” is more precise here because the slot is acquired during
that subtransaction.


--
Thanks & Regards,
Kedar


On Fri, Sep 18, 2026 at 1:43 PM Bharath Rupireddy
 wrote:
>
> Hi,
>
> On Wed, Sep 16, 2026 at 6:37 PM Bharath Rupireddy
>  wrote:
> >
> > Please find the attached patch for HEAD. I dropped the two unnecessary
> > header file inclusions added in the diff but otherwise took it as-is.
> > If it looks good, I can prepare the patches for all supported
> > branches.
>
> Please find the attached patches for HEAD and all supported branches.
> Thanks to all for reviewing and sharing thoughts.
>
> --
> Bharath Rupireddy
> Amazon Web Services: https://aws.amazon.com

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

On Mon, 11 May 2026 at 08:31, Fujii Masao  wrote:
>
> On Sun, May 10, 2026 at 5:45 AM SATYANARAYANA NARLAPURAM
>  wrote:
> >
> > Hi Hackers,
> >
> > SQL-callable replication slot functions acquire a slot (setting
> > the process-global MyReplicationSlot) but can then ERROR before reaching
> > ReplicationSlotRelease(). If such an error is caught by a PL/pgSQL
> > EXCEPTION block (which uses a subtransaction), MyReplicationSlot remains
> > set because there is no subtransaction-level cleanup hook for replication
> > slots.
> >
> > Any subsequent slot operation in the same session then hits
> > Assert(MyReplicationSlot == NULL) and crashes the backend on assert
> > enabled builds. In release builds the stale MyReplicationSlot is silently overwritten,
> > permanently orphaning the old slot as "active." The orphaned slot blocks any other
> > session from acquiring it, vacuum and WAL deletion.
> >
> > Repro:
> >
> > SELECT pg_create_logical_replication_slot('adv_test', 'test_decoding');
> >
> > DO $$ BEGIN
> >     PERFORM pg_replication_slot_advance('adv_test', '0/1'::pg_lsn);
> > EXCEPTION WHEN others THEN
> >     RAISE NOTICE 'caught: %', SQLERRM;
> > END $$;
> >
> > SELECT count(*) FROM pg_logical_slot_get_changes('adv_test', NULL, NULL);
> >
> > 2026-05-09 19:45:06.619 UTC [1096805] STATEMENT:  SELECT pg_create_logical_replication_slot('adv_test', 'test_decoding');
> > TRAP: failed Assert("MyReplicationSlot == NULL"), File: "slot.c", Line: 638, PID: 1096805
> >
> >
> > Attached a patch to address this by wrapping error-prone paths in PG_TRY/PG_CATCH blocks
> > and call ReplicationSlotRelease().
>
> Thanks for the report and the patch!
>
> I think wrapping the slot-processing code with PG_TRY()/PG_CATCH() seems
> a good direction for addressing the issue you reported.
>
>
> +   PG_CATCH();
> +   {
> +       ReplicationSlotRelease();
>
> When create_logical_replication_slot() is called with temporary = true,
> the created logical replication slot has RS_TEMPORARY persistency. Such a slot
> is not dropped by ReplicationSlotRelease(), whereas an RS_EPHEMERAL slot is
> dropped via ReplicationSlotDropAcquired().
>
> So even with the v1 patch, a temporary logical replication slot can remain
> unexpectedly if pg_create_logical_replication_slot() throws an error.
> In this case, should create_logical_replication_slot() explicitly drop the slot
> with ReplicationSlotDropAcquired(), or temporarily change the slot persistency
> to RS_EPHEMERAL before calling ReplicationSlotRelease()?
>
>
> Does a newly created logical replication slot created by
> pg_copy_logical_replication_slot() have the same issue?

Additionally pg_logical_slot_get_changes also has the same issue, it
can be reproduced by the following:
SELECT pg_create_logical_replication_slot('test_slot_1', 'test_decoding');

DO $$
BEGIN
    -- This will ERROR if the slot_get changes fails for the slot.
    PERFORM 1 FROM pg_logical_slot_get_changes('test_slot_1', NULL,
NULL, 'nonexistent-option', 'val');
EXCEPTION WHEN others THEN
    RAISE NOTICE 'caught: %', SQLERRM;
END $$;

SELECT count(*) FROM pg_logical_slot_get_changes('test_slot_1', NULL, NULL);

TRAP: failed Assert("MyReplicationSlot == NULL"), File: "slot.c",
Line: 638, PID: 80308
postgres: vignesh postgres [local] SELECT(ExceptionalCondition+0xba)
[0x642e7b2ebae1]
postgres: vignesh postgres [local] SELECT(ReplicationSlotAcquire+0x6e)
[0x642e7b00d732]

Regards,
Vignesh

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Hi,

On Wed, Sep 16, 2026 at 2:02 PM Masahiko Sawada  wrote:
>
> > 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.

Thanks for sharing the diff. I apologize for going back and forth on
the subtransaction commit hand-off. After looking at it, here is what
I have.

The slot acquired in a subtransaction gets cleaned up only if that
subtransaction is aborted, which is the reported bug and easily
reachable from SQL today.

MyReplicationSlotSubId is not a stale value within the transaction but
the owner's subtransaction id. However, across transactions it becomes
stale and can get reused, so it is reset at the subtransaction commit
path, preventing MyReplicationSlotSubId from going stale for users
carrying the slot across transactions (if any). In core code, the
owning subtransaction always releases the slot before it ends, and the
release clears the id. No hand-off, no warning on subtransaction
commit.

Please find the attached patch for HEAD. I dropped the two unnecessary
header file inclusions added in the diff but otherwise took it as-is.
If it looks good, I can prepare the patches for all supported
branches.

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Hi,

On Sun, Aug 2, 2026 at 10:28 PM shveta malik  wrote:
>
> Bharath, I could not find any issue in my basic testing,

Thanks, Shveta, for reviewing!

> although I
> would like to understand this part a bit better:
>
> + if (isCommit)
> + {
> + acquiredInSubId = parentSubid;
> + return;
> + }
>
> How can we reach this block? In a non-error scenario, it seems that by
> the time AtEOSubXact_ReplicationSlot() is invoked, the slot has
> already been released, and we return earlier from 'if (acquiredInSubId
> != mySubid)' block. I could not find a case where:
>
> a) the slot is acquired in the current subtransaction, b) the
> subtransaction commits (isCommit == true), and c) the slot is still
> held when AtEOSubXact_ReplicationSlot() is invoked.
>
> Could you please explain what I am missing?

Yes, no caller hits this today. Each slot function releases the slot
before returning, so we take the early return and never reach the
commit branch with a slot still held. It is there for a future slot
function that acquires the slot but never releases it (rare case). On
commit we pass the slot to the parent so that if an ancestor later
aborts, the slot still gets released, the same way
AtEOSubXact_LargeObject() and AtEOSubXact_Files() do.

I checked this locally by making the pg_replication_slot_advance()
return while still holding the slot and running it through nested
PL/pgSQL exception blocks. With the handoff the outer abort releases
the slot, and dropping the handoff makes the slot leak and hit the
same issue reported in this thread. I can either enhance the comment
there to say it's currently unreachable and why we keep it, or turn it
into an assertion.

Thoughts?

-- 
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Hi,

On Fri, Sep 18, 2026 at 1:31 AM kedar anavardekar
 wrote:
>
> Two minor naming suggestions: (please take the suggestions if you
> think the points are valid)

Thanks for taking a look.

> 1. Could MyReplicationSlotSubId be renamed to
> MyReplicationSlotSubXactId (or MyReplicationSlotSubTransactionId)
>
> SubId may be read as a subscription ID, whereas this variable stores
> the SubTransactionId of the subtransaction that acquired
> MyReplicationSlot. The more explicit name would make its purpose
> clearer and avoid confusion with logical replication subscriptions.

Subscription and its related replication slot on the publisher are on
two different database instances, and one has the context when reading
the code around this. Also, "SubId" is used across the code base and I
want to keep it consistent and short, so MyReplicationSlotSubId looks
fine to me.

> 2., could the comment above AtEOSubXact_ReplicationSlot() be revised from:
> /*
>  * At subxact end, release the replication slot if the subtransaction
>  * where the slot was acquired is aborted.
>  */
> to:
> /*
>  * At subxact end, release the replication slot if the subtransaction
>  * in which the slot was acquired is aborted.
>  */
> “In which” is more precise here because the slot is acquired during
> that subtransaction.

I believe "where the slot was acquired" is grammatically correct as
well, so I'm fine with the existing wording.

-- 
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Hi,

On Fri, Sep 18, 2026 at 11:37 AM Masahiko Sawada  wrote:
>
> I reviewed the v17 patch and here are some comments:

Thanks for taking a look at it.

>          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
> +        PL/pgSQL exception block, does not
> +        drop them. This function corresponds
>
> ISTM what the following sentence says seems to contradict with what
> the first sentence says. How about rephrasing it to:
>
>          for use by the current session. Temporary slots are also
> -        released upon any error. This function corresponds
> +        dropped when an error is reported to the client. An error
> caught inside a
> +        subtransaction, for example by a PL/pgSQL
> +        exception block, does not drop them. This function corresponds

I think "when an error is reported to the client" may not be apt for
all cases here. For example, a temporary slot could be created and
errored out by a worker internally using SPI interface, not
necessarily by a client. I don't want that confusion, so I dropped the
client part.

> ---
> +-- A slot function that errors out must still release the slot, otherwise the
> +-- next slot operation in the session fails an assertion or leaks the slot.
> +-- Advancing a freshly created slot to a low LSN always errors.
> +SELECT 'init' FROM
> pg_create_logical_replication_slot('regress_subxact_slot',
> 'test_decoding');
>
> The comment seems not to be in the right place; it's in right before
> the pg_create_logical_replication_slot() call but not related. Given
> that we have the comments for subsequent tests, we can remove it.

My intention was to have a comment describing the group of tests, like
the other groups in this test file have. I slightly adjusted it to
match those.

> I've attached the updated patch that incorporated the above comments.
> I'm going to push it early next week, barring any objections.

Please find the attached v18 patches (incl. backbranches) with the
above two slight adjustments. I fixed a typo in the commit message but
otherwise retained it as-is.

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Hi,

On Fri, Aug 28, 2026 at 3:49 PM Masahiko Sawada  wrote:
>
> On Thu, Aug 27, 2026 at 9:19 PM shveta malik  wrote:
> >
> > On Thu, Aug 27, 2026 at 6:00 PM Amit Kapila  wrote:
> > >
> > > On Thu, Aug 27, 2026 at 11:58 AM Masahiko Sawada  wrote:
> > > >
> > > > On Mon, Aug 24, 2026 at 3:29 PM Bharath Rupireddy
> > > >  wrote:
> > > > >
> > > > >
> > > > > In short, having just the slot release in the subxact path gives the
> > > > > same error behavior, is simple to reason about, and fixes the crash
> > > > > reported in this thread.
> > > >
> > > > One thing I'm a bit concerned about is that this would be the first
> > > > caller to invoke ReplicationSlotRelease() from inside the transaction
> > > > machinery.
> > > >
> > >
> > > True, but OTOH, won't we already clean up resources not directly
> > > associated with subxact in AtEOSubXact_LargeObject() or
> > > AtEOSubXact_Files()? I don't see any problem as far as the current
> > > pattern of usage for slots.
> >
> > I agree.
> >
> > > The new restriction this patch will add is
> > > "a slot acquired in a subxact does not survive that subxact being
> > > unwound." which should be okay because of its similarity with
> > > top-level xact behavior. I feel if possible we should restrict such
> > > usage explicitly in code in some way rather than one finding out this
> > > as a surprise.
> > >
> > > *
> > > An error raised and caught in a
> > > +          subtransaction, for example by a
> > > +          PL/pgSQL exception block, does not drop
> > > +          them.
> > >
> > > Based on above, something like below won't clean up temp slots and end
> > > up holding xmin.
> > >   DO $$ BEGIN
> > >     PERFORM pg_create_logical_replication_slot('s', 'nonexistent_plugin', true);
> > >   EXCEPTION WHEN OTHERS THEN RAISE NOTICE '%', SQLERRM;
> > >   END $$;
> >
> > Well, on rethinking, I feel that if we encounter an error while
> > creating a slot, whether persistent or temporary, the slot should be
> > dropped right there.
> >
> > This already works correctly for persistent slots: by the time the
> > slot reaches ReplicationSlotRelease, it is still in RS_EPHEMERAL state
> > and is therefore dropped by release. OTIOH, a temporary slot is left
> > behind. I think the temporary slot should also be dropped because the
> > caller never received a reference to it. I don't see a legitimate use
> > case where a temp slot should survive specifically because its
> > creation call failed.
>
> While I agree that it would be an ideal behavior and the analysis
> holds for logical slots, I want to note that persistent physical
> replication slots are created with RS_PERSISTENT so if an error
> happens during the slot creation the slot is left behind. Also,
> logical persistent slots actually have the same gap: if
> ReplicationSlotPersist() raises an error it leaves a persistent slot
> behind as well. Given that slot creation and drop are not
> transactional operations, and that leaving a slot behind on a failure
> is not a new behavior, I'm inclined toward only releasing the slot at
> the subxact abort. We can discuss the better behavior on HEAD
> separately.

Yes, I realised the same. The ephemeral state only applies to logical
slots, not to physical slots or temporary slots.

I agree to keep the back-branch fix simple and solve the slot leak and
crash reported in this thread. However, I think the creation failure
on temporary slots inside a subxact also needs to be fixed in the back
branches (perhaps separately), because one can hit the issue with
direct SQL. A temporary slot whose creation fails needs to be dropped,
to avoid leaking resources for a slot the caller never got a reference
to.

In the replication slot subxact callback, on the abort path, we need
to know whether the slot's creation failed. Ephemeral slots already
handle that, but only for persistent logical slots. A temporary slot
stays RS_TEMPORARY throughout. So there are a few ways to solve this:

1/ Also mark temporary slots as ephemeral initially and transition
them to RS_TEMPORARY once creation succeeds. A quick check shows this
needs changes in many places.
2/ Introduce a new state to represent a temporary slot still in
creation (RS_TEMPORARY_EPHEMERAL or such).
3/ Use a boolean in the ReplicationSlot structure
(is_create_in_progress or such), and in the subxact callback, when the
slot is temporary and is_create_in_progress is set, drop just that
temporary slot and leave the others alone.

I prefer option 3, to keep it simple without adding a new state, and
because it is back-branch friendly. The new boolean lives only in
memory and is not written to disk. To drop a single temporary slot,
I'm thinking of moving the single-slot drop code out of
ReplicationSlotCleanup() into an internal helper function.

Thoughts?

On Thu, Aug 27, 2026 at 5:30 AM Amit Kapila  wrote:
>
> True, but OTOH, won't we already clean up resources not directly
> associated with subxact in AtEOSubXact_LargeObject() or
> AtEOSubXact_Files()? I don't see any problem as far as the current
> pattern of usage for slots. The new restriction this patch will add is
> "a slot acquired in a subxact does not survive that subxact being
> unwound." which should be okay because of its similarity with
> top-level xact behavior. I feel if possible we should restrict such
> usage explicitly in code in some way rather than one finding out this
> as a surprise.

Hi Amit, By restricting in the code, does that mean adding an Assert,
or a WARNING, or a WARNING plus slot release (not an error), in the
replication slot subxact callback on the commit path, instead of
handing the slot off to the parent across the subtransaction boundary?

-- 
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Hi,

On Wed, Sep 16, 2026 at 6:37 PM Bharath Rupireddy
 wrote:
>
> Please find the attached patch for HEAD. I dropped the two unnecessary
> header file inclusions added in the diff but otherwise took it as-is.
> If it looks good, I can prepare the patches for all supported
> branches.

Please find the attached patches for HEAD and all supported branches.
Thanks to all for reviewing and sharing thoughts.

-- 
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Hi,

On Tue, Sep 15, 2026 at 5:13 PM Masahiko Sawada  wrote:
>
> 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. For (2), we can prepare a separate patch that
> implements the handoff mechanism (possibly with a WARNING or DEBUG
> message) with the regression tests, if we want to support these cases.
>
> It seems confusing and I might be too pessimistic as this is all about
> hypothetical cases that might not exist, but I'd like to keep the
> back-branch fix to the smallest thing that fixes only the reported
> problem while not changing other current behaviors.

Thanks for taking a look at it. Here is my thinking on this.

Within a transaction, subtransaction ids are not reset or reused, so
MyReplicationSlotSubId isn't stale. It is the id of the subtransaction
that owns the slot (ownership stays with the subtransaction that
acquired it until the slot is released, and the id gets reset at that
point). And if the owning subtransaction aborts, the slot is released,
which is what protects against the issue reported in this thread
(leftover slot or assertion failure).

Without the handoff on commit, the slot stops being protected at the
first subtransaction boundary. If the owning subtransaction commits
while still holding the slot, whether it holds the slot intentionally
or unknowingly forgets to release the slot, MyReplicationSlotSubId
keeps the id of an owner that is gone, so no later subtransaction in
the transaction can match it. If the parent subtransaction then
aborts, it is not the owner, so the slot is not released even though
the work it was acquired for is being rolled back, and that brings
back the issue reported in this thread.

With the handoff on commit, the parent becomes the owner. If the
parent aborts, it is now the owner, so the slot is released, and the
protection against the issue reported in this thread still holds. If
the parent commits, ownership moves up again, and it eventually
reaches TopSubTransactionId, which is the top-level transaction's own
id and is never assigned to a subtransaction, so no owner id is left
behind that could match an unrelated subtransaction later.

Therefore, handing the slot off to the parent on commit looks correct
to me on all branches, even when the slot is carried across a
subtransaction boundary, because ownership moves to a subtransaction
that is still in progress and the slot stays protected from the issue
reported in this thread for as long as it is held. And I am okay with
not emitting any warning on any branch, including HEAD. For the
legitimate usage, a function that intentionally holds the slot across
subtransaction boundaries, the warning would be reporting correct code
as a problem, and that is as wrong on HEAD as it is in the back
branches. With no warning on any branch, AtEOSubXact_ReplicationSlot()
ends up identical on all branches, so I don't think we need to split
this into a back-branch patch and a separate HEAD patch.

On adding AtEOXact_ReplicationSlot(), I don't think it is needed for
correctness, for the reason above, but I have no objection to adding
it on HEAD, similar to what AtEOXact_LargeObject() and
AtEOXact_Files() do today.

I previously tested the handoff in the commit path, by making
pg_replication_slot_advance() return while still holding the slot and
running it through nested PL/pgSQL exception blocks:
https://postgr.es/m/CALj2ACUD_K5zBgXD3ebYmhmouJx91fq+aiLeD8HSuC6xnYvj3g@mail.gmail.com.
If we want to add this test, I think I can add an injection point that
returns before the slot release (similar to the skip-log-running-xacts
test), and add the test on all the possible back branches.

Thoughts?

-- 
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Hi,

On Sat, Sep 19, 2026 at 9:54 PM Bharath Rupireddy
 wrote:
>
> Please find the attached v18 patches (incl. backbranches) with the
> above two slight adjustments. I fixed a typo in the commit message but
> otherwise retained it as-is.

I slightly adjusted the test comments to make the flow clearer. The
rest remains the same. Please have a look at the attached v19 patches.

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Hi,

On Tue, Aug 4, 2026 at 8:37 PM shveta malik  wrote:
>
> > Right, I've confirmed that this can't happen. I'm OK with keeping this code,
> > since it's future-proof - otherwise, others might raise the same concern I
> > imagined above.
> >
>
> Okay, works for me, let's retain it. But good to change the comment to
> indicate there is no such scenario at the moment, otherwise it may
> confuse readers.

Agreed.

Please find the attached v8 patch.

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

RE: [PATCH] Release replication slot on error in SQL-callable slot functions

On Tuesday, August 4, 2026 4:39 PM Zhijie Hou (Fujitsu)  wrote:
> On Friday, July 31, 2026 3:12 AM Bharath Rupireddy
>  wrote:
> >
> > I read the issue, patches and comments so far and here's my take on it.
> >
> > ...
> 
> Thanks for sharing the patch.
> 
> IIUC, the patch only handles releasing the slot when the subtransaction aborts
> after ReplicationSlotCreate(), but it doesn't address the original repro[1]

Sorry, I misread the code and missed the new logic in ReplicationSlotAcquire().
It does fix all the issues. Please ignore above.

The only thing I notice is that this new design seems to touch more scope than
the original PG_TRY/PG_CATCH approach, since it releases the slot not only on
ERROR but also on a manual transaction abort (a direct AbortCurrentTransaction()
call without an intervening ERROR). It also seems slightly inconsistent that we
do this for subtransactions but not for top-level transactions, but maybe it's
OK as it only targets to fix the PL/pgSQL EXCEPTION case.

One interesting case I thought of: we currently record
GetCurrentSubTransactionId() when creating or acquiring a slot, and that ID is a
logical subxid (starting from 1). So it looks possible for the following to
happen: the user acquires the slot in a subtransaction with subxid 2 and commits
the whole transaction; then, in a new transaction, the user starts a
subtransaction that also gets subxid 2 and aborts it. In that case the slot
would be released, even though the aborted subtransaction is a different one
from the subtransaction that originally acquired the slot. I think the HEAD
cannot create such a case using SQL APIs, so it might not be a serious issue,
but just share it for reference. Maybe some comments are needed to hint user
about the risk of this.

Best Regards,
Zhijie Hou

RE: [PATCH] Release replication slot on error in SQL-callable slot functions

On Friday, May 29, 2026 1:11 PM SATYANARAYANA NARLAPURAM   wrote:
> Thanks for the patches, I combined these changes in my latest patch. Please find the v5.

Thanks for updating the patch.

Few comments:

1.

The patch places PG_CATCH() inside create_logical_replication_slot(), but the
caller may still error out before releasing the slot. It would be better to
catch the error at the caller level instead. The same problem exists in
copy_replication_slot(), which does not include the slot persistence logic
within its CATCH block.

2.

I have a concern about the LWLock handling added within the PG_CATCH block, see [1].

[1] https://www.postgresql.org/message-id/TY4PR01MB17718F04D32E0073F0BC7EC0294082%40TY4PR01MB17718.jpnprd01.prod.outlook.com

Best Regards,
Hou zj

RE: [PATCH] Release replication slot on error in SQL-callable slot functions

On Wednesday, May 27, 2026 7:00 PM shveta malik  wrote:
> 
> On Wed, May 27, 2026 at 1:42 PM Fujii Masao 
> wrote:
> >
> > On Wed, May 27, 2026 at 1:31 PM SATYANARAYANA NARLAPURAM
> >  wrote:
> > > Thank you for the changes and review.
> >
> > Could pg_create_physical_replication_slot() still have the same issue
> > if it throws an error after ReplicationSlotCreate() and that error is
> > caught by a PL/pgSQL EXCEPTION block?
> >
> > Also, do maybe pg_copy_physical_replication_slot(),
> > pg_drop_replication_slot(), and ALTER_REPLICATION_SLOT potentially have
> the same issue as well?
> >
> 
> pg_copy_physical_replication_slot() should not have it as the common
> 'copy_replication_slot' is already fixed in the patch.  I will review the others.

I have one slight concern about the approach of releasing the slot within a
PG_CATCH() block in lots of functions. I'm not entirely sure if it's safe or
acceptable to do so before aborting the current transaction, so just to confirm
it once:

Since both ReplicationSlotRelease() and ReplicationSlotDropPtr() acquire
LWLocks, it's possible that a backend reports an ERROR while already holding
one of these locks, then enters the PG_CATCH() block and calls
ReplicationSlotRelease(), which attempts to acquire the same LWLock. However,
LWLocks do not distinguish between locks held by the same backend versus other
backends, so the backend could block forever and become uninterruptible.

I don't have a better alternative, but I think we can
evaluate once whether this is a real risk and if it's acceptable (perhaps the
scenario is rare enough to be acceptable). It may also be worth adding comments
to document this risk.

Best Regards,
Hou zj