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

Hi,

A brief ping on v4 before PG20-2 starts. The latest CFBot run is
green on all enabled jobs:

https://github.com/postgres/postgres-cfbot/actions/runs/32910623105/attempts/1

The point on which I would especially appreciate review is the
restart_lsn invariant described in section 2 of v4: a decoding pass
that does not know the subtransaction's parent cannot output-decode
that transaction's commit, so tuplecid entries left by the skipped
cleanup cannot reach ReorderBufferBuildTupleCidHash().

Could someone comment on whether that reasoning is sound, and whether
the O(N) abort-time cleanup is an acceptable backpatchable fix?

PostgreSQL 19 is currently at Beta 3. Should BUG #19555 also be
tracked as a PostgreSQL 19 open item?

CF entry:
https://commitfest.postgresql.org/patch/7137/

Thanks,
Bingshuai Li

Hello hackers,

27.08.2026 10:38, Bingshuai Li wrote:

A brief ping on v4 before PG20-2 starts. The latest CFBot run is
green on all enabled jobs:

https://github.com/postgres/postgres-cfbot/actions/runs/32910623105/attempts/1

....

PostgreSQL 19 is currently at Beta 3. Should BUG #19555 also be
tracked as a PostgreSQL 19 open item?

CF entry:
https://commitfest.postgresql.org/patch/7137/

FWIW, buildfarm animal prion produced a couple of failures recently due to
that assertion triggered, [1]https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&amp;dt=2026-09-15%2021%3A39%3A57, [2]https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&amp;dt=2026-09-15%2022%3A13%3A04:
TRAP: failed Assert("ent->cmin == change->data.tuplecid.cmin"), File:
"/home/ec2-user/bf/root/HEAD/pgsql.build/../pgsql/src/backend/replication/logical/reorderbuffer.c", Line: 1890, PID: 1211632
...
2026-09-15 21:58:17.665 UTC [1211335:63] LOG:  00000: client backend (PID 1211632) was terminated by signal 6: Aborted
2026-09-15 21:58:17.665 UTC [1211335:64] DETAIL:  Failed process was running: SELECT data FROM
pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');

[1]: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&amp;dt=2026-09-15%2021%3A39%3A57
[2]: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&amp;dt=2026-09-15%2022%3A13%3A04

Best regards,
Alexander

Hi Alexander,

Thanks for the data point. These failures are exactly the assertion
analyzed in this thread, and the failing statement is the decode call
right after the ddl test's own "DDL in aborted subtransactions"
section (contrib/test_decoding/sql/ddl.sql):

CREATE TABLE tr_sub_ddl(data int);
BEGIN;
SAVEPOINT a;
ALTER TABLE tr_sub_ddl ALTER COLUMN data TYPE text;
INSERT INTO tr_sub_ddl VALUES ('blah-blah');
ROLLBACK TO SAVEPOINT a;
ALTER TABLE tr_sub_ddl ALTER COLUMN data TYPE bigint;
INSERT INTO tr_sub_ddl VALUES(43);
COMMIT;

That is the same shape as the BUG #19555 reproducer, and the lines
missing from the ddl.out diff are everything after the backend died.

The failure is inherently layout-dependent: it needs on-access
pruning to free the rolled-back subtransaction's catalog line
pointer and a later catalog insert to reuse that tid. Presumably
prion's force-release configuration (RELCACHE_FORCE_RELEASE,
CATCACHE_FORCE_RELEASE) makes that far more likely than on other
animals -- two hits in one evening supports that.

The v4 patch posted on 2026-08-15 [1]/messages/by-id/SY8P300MB0570D533D4F6B47B804C7D57C6D92@SY8P300MB0570.AUSP300.PROD.OUTLOOK.COM fixes exactly this path: it
removes the aborted subtransaction's tuplecid entries in
ReorderBufferAbort(), so no stale (relfilelocator, tid) mapping
remains to collide with a reused tid. The regression test added by
v4 reproduces this crash within a handful of iterations on
unpatched assert builds and always passes with the patch.

I re-checked v4 against current master (be00f041a33) today: it
still applies cleanly, and no upstream commit has touched the
tuplecid code paths since it was posted, so the verification matrix
from the v4 mail (master and REL_19_STABLE, assert and non-assert,
both build systems) still stands.

With the buildfarm now red on HEAD and PostgreSQL 19 GA
approaching, could a committer please take a look at v4? The two
points where I would most appreciate judgement are unchanged from
my August 27 mail: the restart_lsn invariant (section 2 of the v4
mail), and whether the O(N) abort-time cleanup is acceptable to
backpatch. I also believe this strengthens the case for tracking
BUG #19555 as a PostgreSQL 19 open item. I'm happy to prepare a
REL_19_STABLE version of the patch if that would help.

[1]: /messages/by-id/SY8P300MB0570D533D4F6B47B804C7D57C6D92@SY8P300MB0570.AUSP300.PROD.OUTLOOK.COM

Thanks,
Bingshuai Li

On 2026-Sep-17, Bingshuai Li wrote:

With the buildfarm now red on HEAD and PostgreSQL 19 GA
approaching, could a committer please take a look at v4? The two
points where I would most appreciate judgement are unchanged from
my August 27 mail: the restart_lsn invariant (section 2 of the v4
mail), and whether the O(N) abort-time cleanup is acceptable to
backpatch. I also believe this strengthens the case for tracking
BUG #19555 as a PostgreSQL 19 open item.

What's the reason to consider this as a worse problem in 19 than in
previous releases? In my mind it's pretty clear that this bug needs to
be fixed in all stable branches. Moreover, I think the real deadline
here is the next set of minors to be released in November, rather than
the 19.0 release. The November minors would also include 19.1.

I'm happy to prepare a REL_19_STABLE version of the patch if that
would help.

If the proposed patch has any conflicts when applied on any previous
branches, then by all means please do provide backpatches as
appropriate. However, if the same patch applies cleanly or only has
trivial conflicts, then there's no need. (I see there's a trivial
conflict in 18's test schedule files for instance, for which IMO you
don't need to provide a patch. Didn't try further back.)

The patch does change the layout of ReorderBufferChange, which is
apparently an exported struct, so some analysis on whether that can
break anything would also be useful. It might be innocuous, if there
are no external users, but better make sure ...

--- /tmp/ReorderBufferChange.unpatched  2026-09-17 13:09:09.517276936 +0200
+++ /tmp/ReorderBufferChange.patched    2026-09-17 13:09:35.505580268 +0200
@@ -37,27 +37,29 @@
        CommandId          command_id;           /*    32     4 */
        struct {
            RelFileLocator locator;          /*    32    12 */
            ItemPointerData tid __attribute__((__aligned__(2))); /*    44     6 */

/* XXX 2 bytes hole, try to pack */

            CommandId  cmin;                 /*    52     4 */
            CommandId  cmax;                 /*    56     4 */
            CommandId  combocid;             /*    60     4 */
-       } __attribute__((__aligned__(4))) tuplecid __attribute__((__aligned__(4)));  /*    32    32 */
+           /* --- cacheline 1 boundary (64 bytes) --- */
+           TransactionId subxid;            /*    64     4 */
+       } __attribute__((__aligned__(4))) tuplecid __attribute__((__aligned__(4)));  /*    32    36 */
        struct {
            uint32     ninvalidations;       /*    32     4 */

/* XXX 4 bytes hole, try to pack */

            SharedInvalidationMessage * invalidations; /*    40     8 */
        } inval;                                 /*    32    16 */
-   } data __attribute__((__aligned__(8)));          /*    32    32 */
-   /* --- cacheline 1 boundary (64 bytes) --- */
-   dlist_node                 node;                 /*    64    16 */
+   } data __attribute__((__aligned__(8)));          /*    32    40 */
+   /* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */
+   dlist_node                 node;                 /*    72    16 */

-   /* size: 80, cachelines: 2, members: 6 */
-   /* sum members: 70, holes: 2, sum holes: 10 */
+   /* size: 88, cachelines: 2, members: 6 */
+   /* sum members: 78, holes: 2, sum holes: 10 */
    /* forced alignments: 1, forced holes: 1, sum forced holes: 6 */
-   /* last cacheline: 16 bytes */
+   /* last cacheline: 24 bytes */
 } __attribute__((__aligned__(8)));

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Debido a que la velocidad de la luz es mucho mayor que la del sonido,
algunas personas nos parecen brillantes un minuto antes
de escuchar las pelotudeces que dicen." (Roberto Fontanarrosa)

Hi Álvaro,

Thanks for picking this up.

What's the reason to consider this as a worse problem in 19 than in
previous releases?

There isn't one -- I asked because 19 GA was approaching and the bug
was reported against 19. Fixing it in all stable branches for the
November minors sounds right.

If the proposed patch has any conflicts when applied on any previous
branches, then by all means please do provide backpatches as
appropriate.

I ran v4 across all supported branches (--enable-cassert build plus
the full test_decoding suite on each):

branch apply tests (regress+isolation)
------ ------------------------------- -------------------------
master clean (be00f041a33) 21+15, CFBot green
19 clean (a203d201900) 21+15
18 trivial test-list conflicts 20+14
17 branch version attached 20+14
16 branch version attached 20+14
15 branch version attached 20+14
14 branch version attached 20+13

(The counts follow each branch's existing test_decoding suite;
tuplecid and tuplecid_restart pass everywhere.)

On 18 the conflicts are exactly the trivial ones you saw: master's
test lists mention repack and parallel_session_origin, which don't
exist there. All code applies cleanly, so per your advice I have not
prepared an 18 version.

The attached branch versions differ from v4 as follows:

- 17, 16: only the test-list context, plus one rename -- the cleanup
calls ReorderBufferReturnChange(), the pre-PG18 name of
ReorderBufferFreeChange() (renamed by ac4494646da; same 3-argument
signature and body). The 17 and 16 versions differ from each other
only in the test-list context, too.
- 15, 14: additionally drop the meson.build hunk (meson arrived in
16) and use the pre-PG16 names: RelFileNode, xlrec->target_node,
and txn->toptxn instead of rbtxn_get_toptxn(). The 14 version also
trims the new test's expected output, because 14's psql prints only
the last result of a multi-statement string; the decode work is
unchanged, so the test exercises the bug just the same.

rbtxn_is_known_subxact() exists natively in all of these branches, so
the fix itself is identical everywhere.

The patch does change the layout of ReorderBufferChange, which is
apparently an exported struct, so some analysis on whether that can
break anything would also be useful.

I checked who can observe the layout change:

- the tuplecid variant is only ever accessed from reorderbuffer.c
itself (no data.tuplecid users anywhere else in tree);
- only reorderbuffer.c touches change->node, and only reorderbuffer.c
allocates, frees or serializes ReorderBufferChange (the spill files
embed the struct, but they are transient within a single decoding
session);
- output plugins (pgoutput, pgrepack, test_decoding, and external
ones using the same callback API) only read change->action, ->txn,
->data.tp.{old,new}tuple and ->data.truncate.*, all at offsets
unchanged by the patch: the union only grows at the tail of the
tuplecid variant, and the struct stays within two cachelines, as
your pahole output shows.

So the layout change is innocuous for any plugin I can think of.

There is one more exported-API change next to it:
ReorderBufferAddNewTupleCids() gains a subxid parameter. Its only
caller is the NEW_CID decode path in snapbuild.c; output plugins have
no reason to call it. If you'd rather keep the exported signature
stable on the back branches, I can keep the old one as a wrapper
passing InvalidTransactionId -- it is about five lines.

The part of v4 where I would still most value a second opinion is the
restart_lsn invariant in section 2 of the v4 mail (why the skipped
cleanup on the restart path cannot reach the tuplecid hash); I believe
everything else is covered above.

Thanks,
Bingshuai Li

Hi Álvaro,

Apologies -- the branch versions did not make it onto that mail.
Here they are.

Thanks,
Bingshuai Li

Hi,

Housekeeping for the commitfest entry: CFBot picked up the
REL_14_STABLE back-branch version from my previous mail and now
reports "needs rebase" against master. Re-attaching the master
version of v4 (unchanged; still applies cleanly to current master
e1d8f81d496), so the entry points at the right patch again. The
branch versions from my previous mail are for the back branches
only.

Thanks,
Bingshuai Li

Hi Bingshuai,

Thanks for working on it. Confirmed it's a real issue.

I've read your patch and one comment:
The path you added might be redundant if the top-level transaction is being aborted,
right? Can we export the cleanup part outside ReorderBufferAbort()? It does the
actual task only if needed.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Bingshuai Li <lucian1412@outlook.com> writes:

What's the reason to consider this as a worse problem in 19 than in
previous releases?

There isn't one -- I asked because 19 GA was approaching and the bug
was reported against 19. Fixing it in all stable branches for the
November minors sounds right.

While it's not any worse for v19 than older branches, it would be sad
if this isn't fixed for v19 GA, since we have that scheduled ahead of
the November minors.

Also, of late the test_decoding/sql/ddl.sql test has been failing
often enough in the buildfarm to be quite annoying. So I'd like to
see this fixed sooner not later. (It's not very clear to me why
we are suddenly able to see this old bug in the regression tests.
The part of ddl.sql that's crashing hasn't changed in years, but
BF member "prion" has failed multiple times in the past two weeks.
Do we have a theory about that that's better than hand-wavy
"some change in timing"?)

regards, tom lane

Hello Tom,

29.09.2026 21:31, Tom Lane wrote:

Bingshuai Li<lucian1412@outlook.com> writes:

What's the reason to consider this as a worse problem in 19 than in
previous releases?

There isn't one -- I asked because 19 GA was approaching and the bug
was reported against 19. Fixing it in all stable branches for the
November minors sounds right.

While it's not any worse for v19 than older branches, it would be sad
if this isn't fixed for v19 GA, since we have that scheduled ahead of
the November minors.

Also, of late the test_decoding/sql/ddl.sql test has been failing
often enough in the buildfarm to be quite annoying. So I'd like to
see this fixed sooner not later. (It's not very clear to me why
we are suddenly able to see this old bug in the regression tests.
The part of ddl.sql that's crashing hasn't changed in years, but
BF member "prion" has failed multiple times in the past two weeks.
Do we have a theory about that that's better than hand-wavy
"some change in timing"?)

Besides prion, skink managed to trigger that assert too: [1]https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&amp;dt=2026-09-28%2003%3A25%3A19. It didn't run
tests from 2026-09-08 to 2026-09-25 [2]https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=skink&amp;br=master, and never failed this test before,
so I guess the change which affected the test was on Sep 15: a4b26b8f7
(the very first prion's failure includes this commit [3]https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&amp;dt=2026-09-15%2021%3A39%3A57). I can reproduce
this failure under Valgrind locally, but not reliably enough to bisect it
quickly.

[1]: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&amp;dt=2026-09-28%2003%3A25%3A19
[2]: https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=skink&amp;br=master
[3]: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&amp;dt=2026-09-15%2021%3A39%3A57

Best regards,
Alexander

Alexander Lakhin <exclusion@gmail.com> writes:

29.09.2026 21:31, Tom Lane wrote:

Also, of late the test_decoding/sql/ddl.sql test has been failing
often enough in the buildfarm to be quite annoying. So I'd like to
see this fixed sooner not later. (It's not very clear to me why
we are suddenly able to see this old bug in the regression tests.
The part of ddl.sql that's crashing hasn't changed in years, but
BF member "prion" has failed multiple times in the past two weeks.
Do we have a theory about that that's better than hand-wavy
"some change in timing"?)

Besides prion, skink managed to trigger that assert too: [1]. It didn't run
tests from 2026-09-08 to 2026-09-25 [2], and never failed this test before,
so I guess the change which affected the test was on Sep 15: a4b26b8f7
(the very first prion's failure includes this commit [3]).

I doubt that theory, because a4b26b8f7 was "Revert UPDATE/DELETE FOR
PORTION OF", so to suppose that it caused this failure you'd have to
explain why we didn't see it before any of that went in.

Assuming that there is an identifiable commit that triggered this,
it must certainly be no later than 862092932 (the latest commit seen
by prion's first failure of this sort). But it could plausibly have
been anything within several days before that. ddl.sql itself wasn't
touched for nearly a month before that, so I'm inclined to bet that
the relevant change was somewhere in our code not in the test itself.

Given that the problem is expensive to reproduce, I doubt that
bisecting to identify this still-hypothetical commit is worth the
trouble; I was just curious to know if anyone already understood the
connection. Now, if it turns out that Bingshuai's patch doesn't make
prion happy, then we'll need to dig deeper.

regards, tom lane

On Tue, Sep 29, 2026 at 05:17:19PM -0400, Tom Lane wrote:

Given that the problem is expensive to reproduce, I doubt that
bisecting to identify this still-hypothetical commit is worth the
trouble; I was just curious to know if anyone already understood the
connection. Now, if it turns out that Bingshuai's patch doesn't make
prion happy, then we'll need to dig deeper.

It's not that hard to reach. If I run three check-world builds in
parallel to load my laptop CPU (say HEAD, v19 and v18 with a
worktree), HEAD is able to fail rather reliably like 30%~40% of the
time, on this test. The problem is very easy to reach for the last
two weeks.
--
Michael

On Wed, Sep 30, 2026 at 12:05:09PM +0900, Michael Paquier wrote:

On Tue, Sep 29, 2026 at 05:17:19PM -0400, Tom Lane wrote:

Given that the problem is expensive to reproduce, I doubt that
bisecting to identify this still-hypothetical commit is worth the
trouble; I was just curious to know if anyone already understood the
connection. Now, if it turns out that Bingshuai's patch doesn't make
prion happy, then we'll need to dig deeper.

It's not that hard to reach. If I run three check-world builds in
parallel to load my laptop CPU (say HEAD, v19 and v18 with a
worktree), HEAD is able to fail rather reliably like 30%~40% of the
time, on this test. The problem is very easy to reach for the last
two weeks.

There is cheap way to reproduce the issue, as of two
pg_log_standby_snapshot() in ddl.sql:
@@ -231,6 +231,7 @@ ROLLBACK TO SAVEPOINT b;
INSERT INTO tr_sub(path) VALUES ('3-top-2-#2');
COMMIT;

+SELECT pg_log_standby_snapshot() IS NOT NULL;
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');

 -- test whether a known, but not yet logged toplevel xact, followed by a
@@ -250,6 +251,7 @@ INSERT INTO tr_sub(path) VALUES ('5-top-1-#1');
 COMMIT;

+SELECT pg_log_standby_snapshot() IS NOT NULL;
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');

-- check that DDL in aborted subtransactions handled correctly

See also the attached. This stuff reproduces quickly the assertion
failure.
--
Michael

Hi Hayato, Hi Tom,

Thanks, Hayato, for reviewing and confirming the issue, and Tom for
following up. Attached is v5, with the cleanup moved out of
ReorderBufferAbort() and a test for nested subtransaction rollback.

1. Hayato's comment

The path you added might be redundant if the top-level transaction is
being aborted, right? Can we export the cleanup part outside
ReorderBufferAbort()? It does the actual task only if needed.

Agreed. The cleanup loop is now in
ReorderBufferCleanupSubTxnTupleCids(), and ReorderBufferAbort() is back
to its upstream implementation. DecodeAbort() calls the helper for
each aborted xid before removing its transaction state, passing the
abort record's primary xid as well.

The helper uses the known toplevel of the xid being cleaned. If that
toplevel's xid equals the primary xid, the whole transaction is being
aborted, so it returns without scanning the tuplecid list;
ReorderBufferCleanupTXN() will free it. Otherwise it removes the
tuplecid entries written by that subtransaction.

The primary xid's own association cannot be used to decide whether
cleanup is needed. An outer subtransaction that wrote no WAL of its
own can have no known association when its abort is decoded: the abort
record is written after entering TRANS_ABORT, so
IsSubxactTopXidLogPending() does not attach the toplevel xid. Released
inner subtransactions listed in that abort record can still have known
associations, and their tuplecids must be removed from the surviving
toplevel's list.

A new isolation test, tuplecid_nested, covers that shape: the toplevel
writes WAL first, an inner subtransaction inserts into a user catalog
table and is RELEASEd, and the outer subtransaction (no WAL of its own)
is then rolled back; using the page-fill and vacuum recipe of
tuplecid_restart, the toplevel's later insert deterministically reuses
the aborted subtransaction's line pointer. With cleanup gated on the
primary xid's known association, the test dies with the original cmin
assertion; with the attached version it passes. (The other test files
are unchanged from v4.)

2. Behavior vs v4, scenario by scenario

- Partial rollback, aborted subxact wrote its own WAL (the BUG #19555 /
ddl.sql shape): cleanup runs, as in v4. The tuplecid regression test
still trips the original assertion without the fix and passes with it.
- Partial rollback, aborted outer subxact without own WAL, released
inner subxact (the nested shape): the inner subxact's entries are
cleaned, as in v4, even if the outer subxact's association is unknown.
- Toplevel abort: no scan at all (early return), and
ReorderBufferCleanupTXN() frees the whole list.
- Association of the xid being cleaned unknown (a decoding pass that
started mid-transaction): cleanup skipped, as in v4; the restart-point
invariant from my v4 mail still applies unchanged (a pass that
output-decodes the toplevel commit has replayed the subtransaction's
first record and knows the association; later passes skip the commit
entirely), and tuplecid_restart still passes.
- Two-phase aborts and ReorderBufferAbortOld() are untouched.

Compared with v4, this adds one exported helper,
ReorderBufferCleanupSubTxnTupleCids(). ReorderBufferAbort() retains its
existing signature. The tuplecid subxid field and the additional subxid
argument to ReorderBufferAddNewTupleCids() are unchanged from v4.

3. Verification matrix (all --enable-cassert --enable-debug unless
noted; make check in contrib/test_decoding):

ENFORCE below denotes a build with
ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS defined.

branch base apply regress + isolation
master 6a93535798a clean (also non-assert and
ENFORCE builds) 21 + 16
REL_19_STABLE ab274a1e966 clean 21 + 16
REL_18_STABLE ea24eadda01 only the known trivial
test-list conflicts 20 + 15
REL_17_STABLE 583b414e511 branch version attached 20 + 15
REL_16_STABLE b8d50e14252 branch version attached 20 + 15
REL_15_STABLE 38e07ce9847 branch version attached 20 + 15
REL_14_STABLE 42ebc601fec branch version attached 20 + 14

tuplecid, tuplecid_restart and tuplecid_nested pass everywhere.

4. Tom's question about the recent buildfarm failures

I have some possible leads, but have not isolated the change that made
the failure more frequent. In the prion history I inspected, the first
failure with this signature was on September 15 at 21:39, at
862092932c9; the preceding green run was at 09:13, at ff39a858b984.
There are seven commits between those revisions.

Alexander's candidate a4b26b8f7 removes 30 lines of ddl.sql before the
tr_sub_ddl section, although the failing section itself is unchanged.
Another commit in that interval, ddce1da5b1b, adds a pg_opclass.dat
entry. Both could affect catalog layout, on which this bug's TID
collision depends. That is a possible explanation for why the revert
could matter, but I have not established that either change caused the
increase in failures.

The failures are intermittent on prion and skink. Locally, 50 runs at
prion's first failing revision with the same configure flags produced
no crashes. Michael's report of frequent failures under concurrent
check-world builds provides a useful lead about the role of the
environment, but these observations do not rule out a recent code
change as the trigger.

Thanks to Michael for the reproducer. I also tried it, adding two
pg_log_standby_snapshot() calls to ddl.sql, under CPU load:

- An unfixed tree at 862092932c9, built with cassert and
RELCACHE_FORCE_RELEASE, CATCACHE_FORCE_RELEASE and
REALLOCATE_BITMAPSETS, hit the original cmin assertion in all five
runs, at the tr_sub_ddl get_changes call.
- The attached v5 on 6a93535798a, built with cassert and debug but
without those three defines, completed ddl.sql without the assertion
in all five runs. make check still reported a failure because the
expected output lacked the two added queries' output; the remaining
decoded output matched.

Those runs used different baselines and build configurations, so they
do not isolate the patch's effect. They show that I can reproduce the
reported failure and that the v5 build completes the reproducer. The
nested test's failure with the association-gated variant and success
with v5 provide separate evidence for the nested rollback fix.

The patch removes the aborted subtransaction's tuplecids before the
surviving toplevel commit is processed. If prion or skink still hits
the assertion after the fix, I'll follow up with that evidence.

Does this address your comment, Hayato? Happy to adjust further.

Thanks,
Bingshuai Li