RE: Bug in logical decoding with DDL and subtransactions

Hi,

Thanks Álvaro for putting together v6. In cfbot run 37698593816, the two
UBSan jobs, "Linux - Meson (32-bit)" and "Linux - Autoconf", fail with
the same error:

reorderbuffer.c:3281:9: runtime error: null pointer passed as argument
2, which is declared to never be null
#0 TransactionIdInSubxactArray reorderbuffer.c:3281
#1 ReorderBufferCleanupAbortedSubTxnTupleCids reorderbuffer.c:3258
#2 DecodeAbort decode.c:912

An abort record without a subxacts block -- for example, ROLLBACK TO a
savepoint with no subcommitted children -- leaves ParseAbortRecord()'s
subxacts == NULL and nsubxacts == 0. In 0002, any tuplecid entry not
matching the primary XID then reaches bsearch(&xid, NULL, 0, ...).
That violates the nonnull contract and aborts the backend under UBSan.
The crashes are in stream and t/019_stream_subxact_ddl_abort.pl; the
backend crash also explains the concurrent regression failures.

The guard belongs in TransactionIdInSubxactArray():

if (nsubxacts == 0)
return false;

We still need to remove the primary XID's entries when there are no
children. The isolation tests avoid this UB for different reasons:
tuplecid_restart's entries match the primary XID and short-circuit the
bsearch, whereas tuplecid_nested has a nonempty subxacts array.

With that guard, +1 on 0002: it preserves the cleanup needed before
decoding a commit for output, including finding the toplevel through a
known child when the primary XID is unknown, while scanning the tuplecid
list once. The sortedness precondition holds: AssignTransactionId()
assigns ancestors their XIDs first, AtSubCommit_childXids() relies on
that ordering, and RecordTransactionAbort() logs the array as-is.

On Zhijie's comments:

1. I think the check is needed on a pass that restarts after a child's
first WAL record. Later records can create a ReorderBuffer entry without
replaying the record that established its toplevel association. The
existing catalog_change_snapshot test describes this for NEW_CID records.
I reproduced it with a nested variant on current master plus v6 and the
guard above: at the outer abort, the primary XID was unknown, while a
child in the subxacts array had an entry but was not a known subxact.
The toplevel commit was skipped in that pass. So an entry's presence
does not establish its association; I'd keep rbtxn_is_known_subxact()
and explain that reason in the comment.

2. Agreed, the sortedness comment belongs at the call site.

3. Agreed. 0002 already rewrote the helper's comment. The decode.c
comment could just explain why cleanup precedes the ReorderBufferAbort()
loop, leaving the association reasoning in the helper.

4. Yes, the skipped-cleanup case is reachable on a pass that restarts
after the subtransaction's first, toplevel-XID-bearing WAL record.
Later NEW_CID records can still queue tuplecids on the toplevel without
establishing the writer's association. I reproduced this by consuming
the toplevel commit and then replaying from a checkpoint between the
child's first WAL record and its catalog write. The abort cleanup was
skipped, and so was that already-consumed commit.

For a small example using catalog_change_snapshot.spec, add this step
to session s0 and run only the following permutation:

step "s0_rollback" { ROLLBACK TO SAVEPOINT sp1; }

permutation "s0_init" "s0_begin" "s0_savepoint" "s0_insert"
"s1_checkpoint" "s1_get_changes" "s0_insert2"
"s0_rollback" "s0_commit" "s0_begin" "s0_insert"
"s1_checkpoint" "s1_get_changes" "s0_commit"
"s1_get_changes"

The final get_changes replays the abort without the association and
skips the earlier commit. To exercise (1), nest another savepoint inside
sp1 before the first insert, and release it just before s0_rollback.

The invariant rules out output-decoding that commit in the same pass,
rather than ruling out missing associations: a slot's restart point
cannot advance past the oldest in-progress transaction. The comment
should distinguish those cases. tuplecid_nested covers the separate
case where a known child lets us do the cleanup despite an unknown
primary XID; tuplecid_restart covers replay of the association before
a commit that is decoded for output.

On dropping tuplecid.sql: agreed. The two isolation tests construct the
collision deterministically using a user_catalog_table, page filler,
VACUUM to LP_UNUSED and tid reuse. tuplecid.sql depends on incidental
catalog page layout. Together with your coverage comparison, that seems
like a good reason to keep just the isolation tests.

No attachment here, so cfbot keeps testing the v6 series as posted.
Happy to provide a respin with the guard and comment updates if useful;
otherwise please fold them in as you see fit. The earlier backbranch
versions use v5's per-XID cleanup; I can refresh those once master settles.

Best regards,
Bingshuai Li