RE: Bug in logical decoding with DDL and subtransactions - Mailing list pgsql-hackers

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