Hi Mark, There has been no feedback so far, so I went ahead and wrote the abort-time cleanup variant I offered in my first mail, so that both approaches can be compared concretely. Patch attached (including a regression test, more on that below).
The variant shares your approach of tagging each tuplecid record with the writing subtransaction's xid, but instead of resolving conflicts lazily at hash-build time via TransactionIdDidAbort(), it removes the aborted subtransaction's entries in ReorderBufferAbort(). Compared to your patch: - ReorderBufferBuildTupleCidHash() is left untouched: with the stale entries removed at abort time, the "same tid => same cmin" assumption holds again, so the original Assert can stay (and no WARNING needs to be raised from the decoding path). - No clog lookups during decode: the abort is known from the WAL stream itself. - The mirror case is covered too: a catalog tuple *deleted* only by an aborted subtransaction no longer leaves a stale cmax behind, so historic snapshots cannot wrongly treat a still-live tuple as deleted. One implementation note: no extra work is needed to learn the sub->top association. Any subtransaction with tuplecid entries must have written WAL (the xl_heap_new_cid record itself), and records written inside a subtransaction carry the top-level xid, which LogicalDecodingProcessRecord() uses to assign the subtransaction to its top-level transaction before dispatching the record. So ReorderBufferAbort() can simply walk the toplevel transaction's tuplecids list and drop the entries tagged with the aborted xid. Testing (master 36f7330b8b2, --enable-cassert): - A/B with my earlier reproducer in a loop: unpatched master dies at the 4th iteration (the original Assert at reorderbuffer.c:1890); with the patch, 300 iterations without a crash. - contrib/test_decoding "make check" passes, including the new test (21 regression + 14 isolation tests). - Same results on REL_19_STABLE (64542957b44): the reporter's original script crashes unpatched and passes with the patch. About the regression test: since the collision depends on the physical layout of catalog pages, I initially doubted a usable test was possible. What worked was running the trigger shape in a *freshly created database* (so prior tests cannot have changed the catalog layout), with some catalog churn first and the savepoint-DDL/rollback/DDL/decode shape repeated 25 times via \gexec. Without the fix, the test dies in ReorderBufferBuildTupleCidHash() within a handful of iterations on every fresh cluster I tried; with the fix it always passes. It is still probabilistic in principle -- an unusual initial catalog layout could in theory dodge all 25 iterations -- and on non-assert builds it cannot observe the silent corruption at all, but it is a good deal smaller than a fully deterministic reproducer. I'm not attached to either approach; the goal is to get this fixed for 19. If you prefer your variant, the test should serve it as well, since it exercises the crash that both patches fix. Thanks, Bingshuai Li
v1-0001-Fix-stale-tuplecid-records-left-behind-by-aborted-su.patch
Description: v1-0001-Fix-stale-tuplecid-records-left-behind-by-aborted-su.patch
