On Thu, Aug 6, 2026 at 1:32 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. >
Okay, I get it now. Thanks for the making the change. v10 compiles without these inclusions. Can you please check? xact.c: +#include "replication/slot.h" slot.c: +#include "access/xact.h" thanks Shveta
