RE: Bug in logical decoding with DDL and subtransactions

Hi Hayato, Hi Tom,

Thanks, Hayato, for reviewing and confirming the issue, and Tom for
following up. Attached is v5, with the cleanup moved out of
ReorderBufferAbort() and a test for nested subtransaction rollback.

1. Hayato's comment

> The path you added might be redundant if the top-level transaction is
> being aborted, right? Can we export the cleanup part outside
> ReorderBufferAbort()? It does the actual task only if needed.

Agreed. The cleanup loop is now in
ReorderBufferCleanupSubTxnTupleCids(), and ReorderBufferAbort() is back
to its upstream implementation. DecodeAbort() calls the helper for
each aborted xid before removing its transaction state, passing the
abort record's primary xid as well.

The helper uses the known toplevel of the xid being cleaned. If that
toplevel's xid equals the primary xid, the whole transaction is being
aborted, so it returns without scanning the tuplecid list;
ReorderBufferCleanupTXN() will free it. Otherwise it removes the
tuplecid entries written by that subtransaction.

The primary xid's own association cannot be used to decide whether
cleanup is needed. An outer subtransaction that wrote no WAL of its
own can have no known association when its abort is decoded: the abort
record is written after entering TRANS_ABORT, so
IsSubxactTopXidLogPending() does not attach the toplevel xid. Released
inner subtransactions listed in that abort record can still have known
associations, and their tuplecids must be removed from the surviving
toplevel's list.

A new isolation test, tuplecid_nested, covers that shape: the toplevel
writes WAL first, an inner subtransaction inserts into a user catalog
table and is RELEASEd, and the outer subtransaction (no WAL of its own)
is then rolled back; using the page-fill and vacuum recipe of
tuplecid_restart, the toplevel's later insert deterministically reuses
the aborted subtransaction's line pointer. With cleanup gated on the
primary xid's known association, the test dies with the original cmin
assertion; with the attached version it passes. (The other test files
are unchanged from v4.)

2. Behavior vs v4, scenario by scenario

- Partial rollback, aborted subxact wrote its own WAL (the BUG #19555 /
ddl.sql shape): cleanup runs, as in v4. The tuplecid regression test
still trips the original assertion without the fix and passes with it.
- Partial rollback, aborted outer subxact without own WAL, released
inner subxact (the nested shape): the inner subxact's entries are
cleaned, as in v4, even if the outer subxact's association is unknown.
- Toplevel abort: no scan at all (early return), and
ReorderBufferCleanupTXN() frees the whole list.
- Association of the xid being cleaned unknown (a decoding pass that
started mid-transaction): cleanup skipped, as in v4; the restart-point
invariant from my v4 mail still applies unchanged (a pass that
output-decodes the toplevel commit has replayed the subtransaction's
first record and knows the association; later passes skip the commit
entirely), and tuplecid_restart still passes.
- Two-phase aborts and ReorderBufferAbortOld() are untouched.

Compared with v4, this adds one exported helper,
ReorderBufferCleanupSubTxnTupleCids(). ReorderBufferAbort() retains its
existing signature. The tuplecid subxid field and the additional subxid
argument to ReorderBufferAddNewTupleCids() are unchanged from v4.

3. Verification matrix (all --enable-cassert --enable-debug unless
noted; make check in contrib/test_decoding):

ENFORCE below denotes a build with
ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS defined.

branch base apply regress + isolation
master 6a93535798a clean (also non-assert and
ENFORCE builds) 21 + 16
REL_19_STABLE ab274a1e966 clean 21 + 16
REL_18_STABLE ea24eadda01 only the known trivial
test-list conflicts 20 + 15
REL_17_STABLE 583b414e511 branch version attached 20 + 15
REL_16_STABLE b8d50e14252 branch version attached 20 + 15
REL_15_STABLE 38e07ce9847 branch version attached 20 + 15
REL_14_STABLE 42ebc601fec branch version attached 20 + 14

tuplecid, tuplecid_restart and tuplecid_nested pass everywhere.

4. Tom's question about the recent buildfarm failures

I have some possible leads, but have not isolated the change that made
the failure more frequent. In the prion history I inspected, the first
failure with this signature was on September 15 at 21:39, at
862092932c9; the preceding green run was at 09:13, at ff39a858b984.
There are seven commits between those revisions.

Alexander's candidate a4b26b8f7 removes 30 lines of ddl.sql before the
tr_sub_ddl section, although the failing section itself is unchanged.
Another commit in that interval, ddce1da5b1b, adds a pg_opclass.dat
entry. Both could affect catalog layout, on which this bug's TID
collision depends. That is a possible explanation for why the revert
could matter, but I have not established that either change caused the
increase in failures.

The failures are intermittent on prion and skink. Locally, 50 runs at
prion's first failing revision with the same configure flags produced
no crashes. Michael's report of frequent failures under concurrent
check-world builds provides a useful lead about the role of the
environment, but these observations do not rule out a recent code
change as the trigger.

Thanks to Michael for the reproducer. I also tried it, adding two
pg_log_standby_snapshot() calls to ddl.sql, under CPU load:

- An unfixed tree at 862092932c9, built with cassert and
RELCACHE_FORCE_RELEASE, CATCACHE_FORCE_RELEASE and
REALLOCATE_BITMAPSETS, hit the original cmin assertion in all five
runs, at the tr_sub_ddl get_changes call.
- The attached v5 on 6a93535798a, built with cassert and debug but
without those three defines, completed ddl.sql without the assertion
in all five runs. make check still reported a failure because the
expected output lacked the two added queries' output; the remaining
decoded output matched.

Those runs used different baselines and build configurations, so they
do not isolate the patch's effect. They show that I can reproduce the
reported failure and that the v5 build completes the reproducer. The
nested test's failure with the association-gated variant and success
with v5 provide separate evidence for the nested rollback fix.

The patch removes the aborted subtransaction's tuplecids before the
surviving toplevel commit is processed. If prion or skink still hits
the assertion after the fix, I'll follow up with that evidence.

Does this address your comment, Hayato? Happy to adjust further.

Thanks,
Bingshuai Li