RE: Bug in logical decoding with DDL and subtransactions

Hi Hayato,

On why some platforms passed: the two failing jobs are the only ones in
that run built with UBSan (CFLAGS ... -fno-sanitize-recover=all
-fsanitize=alignment,undefined, UBSAN_OPTIONS with abort_on_error=1).
glibc's <stdlib.h> declares bsearch()'s key, base and compar __nonnull,
and UBSan's nonnull-attribute check validates call arguments against
that declaration even when nmemb is zero. In the glibc build I probed,
the NULL/0 input returns NULL without touching base, so without the
sanitizer the invalid call can go unnoticed; built with those flags,
the same program reports the same "argument 2" error as the cfbot
logs. With the guard, bsearch() is never reached.

The crash showed up in the stream tests rather than only the new ones:
those tests reach the invalid call while scanning the toplevel's
tuplecid list -- the abort has no subcommitted children, and an entry's
subxid differs from the primary XID. That shape can occur in savepoint
rollbacks with catalog changes. tuplecid_restart's entries match the
primary XID (the bsearch is short-circuited) and tuplecid_nested has a
nonempty array, so neither of them trips the NULL base.

On rbtxn_is_known_subxact(): fair point. My earlier test showed that
an unassociated child entry is reachable, not that the check is
required for correctness. rbtxn_get_toptxn() returns the transaction
itself when it has no toplevel, and tuplecid changes are always queued
on the entry of the NEW_CID record's top_xid, so an unassociated
child's own list is always empty -- selecting it just walks nothing,
and the worst case is a missed cleanup, not a wrong removal. I
compared the child-lookup loop with and without the check (leaving the
primary-XID early return unchanged) on current master plus v6 and the
guard, with three schedules including a mixed case that has one
unassociated and one associated child: the output was identical
either way. In the mixed case, with the check the loop settles on the
associated child and the cleanup does remove its entries; without it
the first child found is selected and nothing is removed -- harmless
in these schedules, because that pass skips the toplevel commit
anyway. So my earlier example is not a reason to insist on keeping
the child-lookup check; I have no objection to removing it.

On skipped transactions: agreed for the already-consumed transaction
in this example. DecodeCommit() takes its skip branch, and
ReorderBufferForget() frees the remaining tuplecids through
ReorderBufferCleanupTXN(), without building the tuplecid hash on that
path. I'd just keep the scope at "this pass will skip the commit":
an abort record can precede start_decoding_at while the later toplevel
commit still needs to be decoded, so the abort record's own skip
decision cannot be used to omit the cleanup.

And yes, your shorter comment is better. I would only keep one line
saying that the cleanup runs before the ReorderBufferAbort() loop
tears down the transactions and their associations. The helper's
child-lookup comment should also distinguish missing associations from
an absence of queued tuplecids on the actual toplevel.

No attachment here, so cfbot keeps testing the v6 series as posted;
the guard and the comment updates can go into the next version as
Álvaro prefers.

Best regards,
Bingshuai Li