Re: Bug in logical decoding with DDL and subtransactions

Hi Ayush,

> Could we skip that scan when none of the aborting xids has catalog
> changes? [...] I can prepare a patch if the direction looks right.

The direction looks right. I built your guard on current master
(26bd08fb5c, which has beff3cd0098) and measured it:

- Correctness: across 60,000 random transactions (DDL plus DML, nested
savepoints with RELEASE, and a DML-only savepoint rolled back after the
DDL), the build with your guard decoded byte-for-byte the same stream as
master. I also instrumented the skip path to walk the list anyway and
count what it would have removed: in 50,287 skipped aborts, 0 had any
tuplecid to remove.

- The loop over the subxacts is necessary, as you noted. Without it, a
tuplecid_nested permutation with an INSERT at the outer savepoint (the
outer subxact has no catalog changes of its own; the entries belong to a
released inner subxact) fails on the cmin Assert, and the same thing
crashes the differential fuzzer in every shard. With the loop, all of it
passes. (ReorderBufferBuildTupleCidHash() already gates on
rbtxn_has_catalog_changes() the same way; the flag is set in
ReorderBufferXidSetCatalogChanges() and cleared nowhere, so it is a sound
gate here too.)

- Saving: it grows as (tuplecid list length) x (DML-only aborts). For short
lists it is in the noise -- around 0.1 ms at 1k tuplecids and 100 aborts.
It becomes real higher up: 10k/1k is 19 ms -> 3.6 ms, 50k/10k is
1067 ms -> 18 ms, and 100k/10k is 5213 ms -> 36 ms.

One thing worth documenting: the guard relies on the catalog-changes flag
never being cleared once set. That holds today, but a short comment or an
Assert at the skip would make the dependency explicit for whoever adds a
flag path later.

Happy to share the extra test permutation and the fuzz/benchmark scripts if
they are useful for the patch.

Regards,
Manu