Hi, On Fri, Aug 7, 2026 at 12:50 AM Bharath Rupireddy <[email protected]> wrote: > > Hi, > > On Thu, Aug 6, 2026 at 5:26 AM Ashutosh Sharma <[email protected]> wrote: > > > > > Please find the attached v9 patch. > > > > The patch looks good overall - just a few quick comments: > > Thanks for taking a look at it. > > > After releasing the slot in AtEOSubXact_ReplicationSlot(), I'd suggest > > adding these assertions: > > > > Assert(MyReplicationSlot == NULL); > > Assert(acquiredInSubId == InvalidSubTransactionId); > > The slot release function sets MyReplicationSlot to NULL in both the > ephemeral and the other path, and clears acquiredInSubId with no early > return, so both conditions already hold there. I would prefer not to > add asserts that re-check what the release just above guarantees. >
+ 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'. -- With Regards, Ashutosh Sharma.
