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

Hi,

On Mon, Aug 10, 2026 at 12:00 AM Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> wrote:
>
> > On Fri, Aug 7, 2026 at 4:31 AM Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> wrote:
> > >
> > > + if (MyReplicationSlot != NULL)
> > > + ReplicationSlotRelease();
> > >
> > > From this if-condition in AtEOSubXact_ReplicationSlot(), it appears
> > > that even when 'acquiredInSubId == mySubid', 'MyReplicationSlot' could
> > > be NULL and if that ever happens we may/will return from this function
> > > without clearing 'acquiredInSubId'. That matters because
> > > 'currentSubTransactionId' is reset to TopSubTransactionId at each
> > > StartTransaction(), so subxact ids are reused across top-level
> > > transactions; a stale id left behind here could later match an
> > > unrelated subxact.
> > >
> > > AFAIU, in practice this should be unreachable: 'acquiredInSubId' is
> > > only ever set together with 'MyReplicationSlot', and both
> > > ReplicationSlotRelease() and ReplicationSlotDropAcquired() clear it,
> > > so "MyReplicationSlot == NULL" implies "acquiredInSubId ==
> > > InvalidSubTransactionId", which can never equal a real `mySubid`. But
> > > the guard's existence suggests you think "MyReplicationSlot == NULL"
> > > is possible.
> > >
> > > So either the reasoning above deserves a comment atop the
> > > if-condition, or, if the NULL case really is impossible, an
> > > 'Assert(MyReplicationSlot != NULL)' would document it more directly
> > > than a silent 'if'.
> >
> > I get your point. Would something like the below work?
> >
> > + /*
> > + * The aborting subxact is the one that acquired the slot, and its id is
> > + * never invalid, so acquiredInSubId is valid here. It is set only when a
> > + * slot is held, and cleared when the slot is released, so the slot must
> > + * still be held.
> > + */
> > + Assert(acquiredInSubId != InvalidSubTransactionId);
> > + Assert(MyReplicationSlot != NULL);
> > + ReplicationSlotRelease();
>
> Thanks for the updated patch.
>
> I feel "Assert(MyReplicationSlot != NULL);" is redundant here, because
> ReplicationSlotRelease() already asserts this at the very beginning of
> its function body, so it isn't strictly required.

That's a valid point. Thinking about it more, I don't think we need
the acquiredInSubId assertion either. We never reach this code with an
invalid mySubid, since we don't get here when no subtransaction is
involved, and the early return already covers it. If acquiredInSubId
were invalid it couldn't match a valid mySubid, so we would return
early and never reach the slot release. 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();

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