Bug in logical decoding with DDL and subtransactions

Hi Mark,

A follow-up on the v3 patch. While re-reviewing it I found a flaw in
the reasoning I gave for the abort-time cleanup. The v4 patch below
fixes the explanation and adds test coverage for the case in point;
the code logic is identical to v3.

1. The flaw in v3's reasoning

v3 claimed the sub->top association is always known in
ReorderBufferAbort(), because any subtransaction with tuplecid entries
must have written WAL, and records written inside a subtransaction
carry the toplevel xid. That argument misses one case: the toplevel
xid is included in a subtransaction's WAL records only once (the
one-time topxid mechanism), and an abort record never carries it. A
decoding pass that starts after the subtransaction's first record --
e.g. when decoding restarts in the middle of a transaction -- never
learns the association, so the v3 cleanup is indeed skipped there. I
confirmed this at runtime with temporary instrumentation:
ReorderBufferAbort() runs with known_subxact = false and the stale
tuplecid entries survive that pass.

2. Why the skipped cleanup stays unobservable

The surviving stale entries still cannot reach
ReorderBufferBuildTupleCidHash(), because of how a slot's restart
point advances:

- SnapBuildProcessRunningXacts() cannot move the restart point past
the oldest in-progress transaction, so a pass that output-decodes
the toplevel commit must have replayed the subtransaction's first
(toplevel-xid-bearing) record; hence it knows the association and
runs the cleanup.
- Conversely, once the restart point has advanced past that record,
the commit was necessarily consumed by an earlier pass. Later
passes skip it via SnapBuildXactNeedsSkip(), and the transaction
state, stale tuplecids included, is dropped without ever building
the hash.
- Starting to decode mid-transaction is unreachable via slot creation
as well: creation waits for a consistent point, which an open
transaction blocks until it ends.

I verified both arms at runtime (cleanup skipped with the commit not
decoded, and association rebuilt with the cleanup running before the
commit decode), and the new test in 3. locks the behavior in. I also
considered recording the tuplecid owner's toplevel xid on the writing
subtransaction's reorder-buffer entry, so the cleanup could always
run; given the invariant above it is not needed, so I left it out to
keep the patch minimal for backpatching.

3. What v4 changes

- reorderbuffer.c: the cleanup itself is byte-for-byte v3; the comment
now states the invariant from 2. instead of the incorrect "always
known" claim.
- New isolation test tuplecid_restart: a transaction with a
rolled-back catalog-modifying subtransaction stays open across two
checkpoints; a concurrent VACUUM deterministically reclaims the
aborted row's line pointer, so the toplevel's later catalog insert
reuses the same tid with a different cmin; the final
pg_logical_slot_get_changes() both replays the transaction across a
restart and output-decodes its commit. On unfixed assert builds it
dies deterministically with the original BUG #19555 assertion in
ReorderBufferBuildTupleCidHash(); with the fix it passes. A second
permutation locks in that slot creation cannot start decoding in the
middle of a transaction.
- The tuplecid regression test is unchanged in behavior; its expected
output is de-noised by hiding the \gexec loop behind
\set ECHO none/all (in-tree convention), 484 -> 199 lines.

Tested on current master (7e6e294e4e4) and REL_19_STABLE
(16742849a3d): contrib/test_decoding passes 21 regression + 15
isolation tests under both make and meson, with asserts on and off,
and with ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS; the core
regression suite passes (245 tests).

PG19 is still in beta, and I'd like to see this fixed there. Review
would be much appreciated -- in particular of the invariant in 2.,
since that is the piece v3 argued incorrectly (even though the skipped
cleanup turns out to be unobservable).

I've Cc'd Amit and Sawada: section 2 leans on the NEW_CID restart
semantics from 64ff0fe4e8c, so your eyes on it would be especially
valuable.

Thanks,
Bingshuai Li