Hi,
On Thu, Aug 6, 2026 at 1:33 PM Bharath Rupireddy
<[email protected]> wrote:
>
> Hi,
>
> On Wed, Aug 5, 2026 at 8:46 PM shveta malik <[email protected]> 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.
