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

On Tue, Aug 11, 2026 at 11:17 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> 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 have the same understanding here and feels that assertion on invalid
acquiredInSubId is not needed.
Also assertion on 'MyReplicationSlot' is not needed because
ReplicationSlotRelease already covers it (as suggested by Ashutosh
above).

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