Re: Bug in logical decoding with DDL and subtransactions

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")