Bug in logical decoding with DDL and subtransactions

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