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

Hi,

On Thu, Aug 6, 2026 at 1:33 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Wed, Aug 5, 2026 at 8:46 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> 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.
>
> Please find the attached v9 patch.
>

The patch looks good overall - just a few quick comments:

--

+ if (MyReplicationSlot != NULL)
+ ReplicationSlotRelease();

After releasing the slot in AtEOSubXact_ReplicationSlot(), I'd suggest
adding these assertions:

Assert(MyReplicationSlot == NULL);
Assert(acquiredInSubId == InvalidSubTransactionId);

--

+ acquiredInSubId = GetCurrentSubTransactionId();

Since the entire cleanup logic relies on the value of acquiredInSubId,
should we make this more robust, something like:

acquiredInSubId = IsTransactionState()
? GetCurrentSubTransactionId()
: InvalidSubTransactionId;

--

+ if (isCommit)
+ {
+ acquiredInSubId = parentSubid;
+ return;
+ }

Is there a test case covering this subtransaction handoff scenario? For example:

DO $$
BEGIN
BEGIN
PERFORM pg_replication_slot_advance('slot', some_valid_lsn);
EXCEPTION WHEN division_by_zero THEN
NULL;
END;

RAISE EXCEPTION 'outer failure';
EXCEPTION WHEN OTHERS THEN
NULL;
END $$;

--
With Regards,
Ashutosh Sharma.