Hi Álvaro,
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.
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().
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.
And thanks for the terminology fixes; "decoding pass" was indeed not
the codebase's language.
Best regards,
Bingshuai Li
