Hello, Many thanks for the reviews. I have pushed this to all branches.
On 2026-Oct-14, Bingshuai Li wrote: > Agreed on the guard (+1); for the record, I reproduced the cfbot > failure locally on master 36a56229765c plus v8 with the same sanitizer > flags -- the stream test loses its backend and the one-line return > makes the test_decoding and isolation cases pass. Right. > One correction on the comment, though, in the "not known to be a > subxact" paragraph: I don't think "there cannot possibly be TUPLECIDs > to clean up" holds. The NEW_CID gate is on the snapshot state, but > the association needed by this abort cleanup is established when the > subxact's first WAL record carrying its toplevel XID is replayed > (LogicalDecodingProcessRecord -> ReorderBufferAssignChild). After a > restart, a saved CONSISTENT snapshot can be restored at a checkpoint > past that record; later NEW_CID records still pass the FULL_SNAPSHOT > gate. The subxact then gets an entry via ReorderBufferProcessXid with > no association, while its TUPLECIDs are queued on the toplevel. That > is the state I reported upthread (entry present, not a known subxact, > tuplecids on the toplevel; the toplevel commit skipped). > > For this unassociated-subxact case, the early return need not mean an > empty list. If the toplevel commits, that commit was already consumed > before start_decoding_at; SnapBuildXactNeedsSkip() skips it and > ReorderBufferForget() frees the list, never reaching the tuplecid hash > -- as Zhijie's v7 comment had it. If the toplevel aborts, > ReorderBufferAbort() frees the list. > The existing parenthetical already covers the top-level abort case. > Minimal suggestion for that paragraph: > > * We return without cleanup if the aborting subxact is not known to > * this reorderbuffer. It may still have TUPLECIDs queued on the > * toplevel if this snapbuild missed its first WAL record carrying > * the toplevel XID. A commit that could use them was already > * consumed: SnapBuildXactNeedsSkip() skips it and ReorderBufferForget() > * frees the list. A toplevel abort frees it via ReorderBufferAbort(). Okay, I adopted this, but made still more changes to it, mostly because I don't think we care very much about the memory freeing; we're mostly concerned with whether the catalog tuple processing is correct. But I also changed how we talk about the subxact being "unknown" -- I mean, it's not that it's really unknown; as I understand it, the problem is that we don't know that the parent is the parent. I also think that talking about SnapBuildXactNeedsSkip() is a bit indirect, so I mentioned DecodeCommit instead. There's quite a bit of abstraction leakage in this whole thing, which is probably unavoidable. I mean the relationship between reorderbuffer.c and decode.c and snapbuild.c. It's probably not surprising that this patch had to touch all three. So I don't feel bad that the comment in reorderbuffer.c needs to mention the function in decode.c. BTW you mention Zhijie's v7, but I think that comment mostly comes from your version just prior to that ... > Also, in the inner comment, the clause "or that we entered FULL_SNAPSHOT > after the subxid was assigned as a child" doesn't work as an > explanation for finding no entry at all: ReorderBufferProcessXid creates > the entry before the snapshot-state gate, and a later transition to > FULL_SNAPSHOT does not itself remove it. Dropping that clause (as in > v8's wording) reads correct to me. Okay, done that way. > And thanks for the terminology fixes; "decoding pass" was indeed not > the codebase's language. Many thanks for double-checking. Feel free to propose more wording changes to these comments if something is still wrong. At least I think we're better than when we started ... -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ Officer Krupke, what are we to do? Gee, officer Krupke, Krup you! (West Side Story, "Gee, Officer Krupke")
