Hi, On Sun, Aug 9, 2026 at 2:48 AM Bharath Rupireddy <[email protected]> wrote: > > Hi, > > On Fri, Aug 7, 2026 at 4:31 AM Ashutosh Sharma <[email protected]> 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.
