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

Hi,

On Sun, Aug 9, 2026 at 2:48 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> Hi,
>
> 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.

--
With Regards,
Ashutosh Sharma.