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

On Wed, Aug 12, 2026 at 8:31 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Thu, Aug 13, 2026 at 7:01 AM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> >
> > Hi,
> >
> > On Mon, Aug 10, 2026 at 11:16 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> 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