Bug in logical decoding with DDL and subtransactions

On 4/30/2026 12:59 PM, Mark Dilger wrote:

There is a bug in logical decoding with CREATE and subtransactions. 
If a CREATE statement creates a row in a catalog during a
subtransaction, but that subtransaction gets rolled back to the
savepoint, and other things happen which trigger page pruning on the
catalog page, and the original transaction (perhaps in a new
subtransaction) then does another CREATE operation, a new row can get
inserted into the same catalog at the same TID.

Hi Mark,

Bug #19555 [1]/messages/by-id/19555-3c700f40a13045cd@postgresql.org appears to be another instance of this issue: an
assertion failure in ReorderBufferBuildTupleCidHash
("ent->cmin == change->data.tuplecid.cmin") when DDL runs inside a
savepoint that is then rolled back.  I reproduced it on REL_19_STABLE
(64542957b44) using the script from the bug report, and on current
master (36f7330b8b2) with a slightly different table shape:

    CREATE TABLE zz(data int, pad1 text, pad2 int, pad3 text);
    BEGIN;
    SAVEPOINT a;
    ALTER TABLE zz ALTER COLUMN data TYPE text;
    ROLLBACK TO SAVEPOINT a;
    ALTER TABLE zz ALTER COLUMN data TYPE bigint;
    COMMIT;
    SELECT data FROM pg_logical_slot_get_changes('regression_slot',
        NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');

The crash is layout-dependent: it needs on-access pruning to free the
aborted subtransaction's line pointer and a later catalog insert to
reuse it, so the catalog churn from the bug report's prelude matters.
With the 4-column table above it crashed on the first attempt.

I traced the WAL and can confirm your diagnosis:

- the rolled-back subtransaction's NEW_CID records stay queued under
  the top-level xid, and ReorderBufferAbort() never removes them;
- its HOT-updated catalog tuple becomes a dead heap-only tuple, which
  on-access pruning marks LP_UNUSED (no vacuum needed);
- the next ALTER's catalog insert reuses that TID, producing a second
  NEW_CID for the same (relfilelocator, tid) with a different cmin.

With your patch applied to master, the previously-crashing case passes
(20 iterations, no crash, no "cmin mismatch" warning), and the
test_decoding regression suite passes.

A few review comments:

1. The patch fixes the crashing path, but the mirror case remains: a
   catalog tuple *deleted* only by an aborted subtransaction keeps a
   stale cmax in the hash, so historic snapshots could wrongly treat a
   live tuple as deleted.  Did you consider removing the aborted
   subtransaction's tuplecid entries at ReorderBufferAbort() time,
   instead of resolving conflicts lazily at hash-build time?  That
   would cover both cases and avoid the TransactionIdDidAbort() clog
   lookup during decode.  I'm happy to write that variant if you think
   it is worth comparing.

2. For the "neither subtransaction aborted" branch, would an Assert
   be better than elog(WARNING)?  A warning from the decoding path
   seems odd, and if the case is truly impossible an assertion would
   document that.

3. The patch has no regression test.  The scenario above is the
   smallest reproducer I know of, though its layout dependence makes
   it fragile for the test suite.  Any suggestions for making it
   deterministic?

It would be great to get this fixed for 19: the bug report shows it
is being hit by fuzzers in the wild, and the cmax half of this
problem was already fixed in 8c67d29fd51 back in 2019.

[1]: /messages/by-id/19555-3c700f40a13045cd@postgresql.org
/messages/by-id/19555-3c700f40a13045cd@postgresql.org

Thanks,
<Bingshuai Li>

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

Hi Mark,

Here is a v2 of the abort-time cleanup patch. The code changes are
identical to v1; there are two test-related fixes:

- the new regression test is now also registered in
contrib/test_decoding/meson.build -- v1 added it to the Makefile
only, so Meson builds would not have run it;
- the test no longer hardcodes the regression database name when
reconnecting to drop its scratch database (make and Meson use
different names, which broke the test's final reconnect under
Meson).

Re-verified on master (36f7330b8b2, --enable-cassert): the
test_decoding suite (21 regression + 14 isolation tests, including
tuplecid) passes under both build systems, and the core regression
suite passes (245/245).

One trade-off worth stating explicitly: ReorderBufferAbort() now
scans the toplevel transaction's tuplecids list, an O(N) walk per
aborted subtransaction. I believe that is acceptable for a
backpatchable fix -- catalog-modifying subtransaction aborts are
rare, and each removal shortens the list. The cleaner long-term
design (queue tuplecids on the writing subtransaction and merge at
commit) is a much larger change and should not block this fix.

The test considerations from my previous mail still apply: it is
probabilistic in principle, and it cannot observe the silent
corruption on non-assert builds. And as before, I'm not attached to
either approach -- the goal is to get this fixed for 19.

Thanks,
Bingshuai Li

Hi Mark,

Here is v3. The only change from v2 is in the regression test: its
scratch database is now named regression_tuplecid instead of
tuplecid_db. cfbot's autoconf task compiles with
-DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS, and the old name tripped
the "should have names including regression" WARNING, which broke the
expected output on that task (all other cfbot tasks were green).
No code changes.

Re-verified under that exact flag locally
(CPPFLAGS=-DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS): the
test_decoding suite passes, 21 regression + 14 isolation tests.

Thanks,
Bingshuai Li

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