From 2a1db38d716cc917e8787e300e122b7931971226 Mon Sep 17 00:00:00 2001 From: Andrey Borodin Date: Sun, 9 Aug 2026 17:36:03 +0500 Subject: [PATCH] Demonstrate the data corruption from a subtransaction abort after subcommit A subtransaction that aborts after AtSubCommit_childXids() has handed its XID to the parent leaves that XID in the parent's committed-children list. The parent's commit then marks the aborted XID committed, so rows the subtransaction rolled back come back to life. This adds an injection point right after the transfer, and a TAP test that turns the resurrection into visible corruption of a primary key. The rolled-back row is produced by a HOT update, so it has no index entry of its own and nobody examines its xmin while the XID is still aborted -- otherwise the HEAP_XMIN_INVALID hint bit would mask the resurrection. A second session then supersedes the version it was chained behind. After the culprit commits, a sequential scan finds two rows under the key while an index scan finds one, amcheck reports the orphaned heap-only tuple, and REINDEX fails. The test asserts correct behaviour, so it fails on unpatched code and prints the corruption. It is a demonstration rather than a proposed regression test: a build with --enable-cassert trips the assertion in clog.c long before reaching any of this. --- src/backend/access/transam/xact.c | 10 ++ src/test/modules/test_misc/Makefile | 2 + src/test/modules/test_misc/meson.build | 1 + .../test_misc/t/015_subxact_resurrect_pk.pl | 149 ++++++++++++++++++ 4 files changed, 162 insertions(+) create mode 100644 src/test/modules/test_misc/t/015_subxact_resurrect_pk.pl diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 3a89149016f..5c6addd2d9b 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -65,6 +65,7 @@ #include "utils/builtins.h" #include "utils/combocid.h" #include "utils/guc.h" +#include "utils/injection_point.h" #include "utils/inval.h" #include "utils/memutils.h" #include "utils/relmapper.h" @@ -5197,6 +5198,15 @@ CommitSubTransaction(void) /* Post-commit cleanup */ if (FullTransactionIdIsValid(s->fullTransactionId)) AtSubCommit_childXids(); + + /* + * Injection point to model a failure occurring after the subtransaction's + * XID has been transferred into the parent's committed-children array but + * before the subcommit finishes. Any error here longjmps into + * AbortSubTransaction() with the XID already in the parent's list. + */ + INJECTION_POINT("subxact-after-childxids-transfer", NULL); + AfterTriggerEndSubXact(true); AtSubCommit_Portals(s->subTransactionId, s->parent->subTransactionId, diff --git a/src/test/modules/test_misc/Makefile b/src/test/modules/test_misc/Makefile index fedbef071ef..6a3c749d69a 100644 --- a/src/test/modules/test_misc/Makefile +++ b/src/test/modules/test_misc/Makefile @@ -3,6 +3,8 @@ TAP_TESTS = 1 EXTRA_INSTALL=src/test/modules/injection_points \ + contrib/amcheck \ + contrib/pageinspect \ contrib/test_decoding # The injection points are cluster-wide, so disable installcheck diff --git a/src/test/modules/test_misc/meson.build b/src/test/modules/test_misc/meson.build index ee290698b31..d71174c9f23 100644 --- a/src/test/modules/test_misc/meson.build +++ b/src/test/modules/test_misc/meson.build @@ -23,6 +23,7 @@ tests += { 't/012_ddlutils.pl', 't/013_temp_obj_multisession.pl', 't/014_log_statement_max_length.pl', + 't/015_subxact_resurrect_pk.pl', ], # The injection points are cluster-wide, so disable installcheck 'runningcheck': false, diff --git a/src/test/modules/test_misc/t/015_subxact_resurrect_pk.pl b/src/test/modules/test_misc/t/015_subxact_resurrect_pk.pl new file mode 100644 index 00000000000..a3c80df38c0 --- /dev/null +++ b/src/test/modules/test_misc/t/015_subxact_resurrect_pk.pl @@ -0,0 +1,149 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group +# +# Demonstrate that a subtransaction which aborts after it has already +# subcommitted corrupts a primary key. +# +# AtSubCommit_childXids() copies the subtransaction's XID into the parent's +# committed-children array before CommitSubTransaction() is finished. If a +# later step throws, control longjmps into AbortSubTransaction(), which records +# the XID aborted but leaves it in the parent's array. The parent's commit then +# marks that aborted XID COMMITTED, so a row the subtransaction rolled back +# becomes live. +# +# To make that reach the index we use a HOT update. The rolled-back row sits in +# the HOT chain behind a live one, so it has no index entry of its own and +# nobody looks at its xmin while the XID is still aborted -- had anyone done so, +# the HEAP_XMIN_INVALID hint bit would have masked the resurrection. A second +# session then updates the row, which retires the version the rolled-back one +# was chained behind. Once the culprit commits, both the resurrected row and +# the second session's row are live under the same key. +# +# The checks below assert correct behaviour, so on unpatched code they fail and +# print the corruption. Note that a build with --enable-cassert trips the +# assertion in clog.c instead of reaching the corruption. + +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +my $node = PostgreSQL::Test::Cluster->new('subxact_resurrect_pk'); +$node->init; +$node->append_conf('postgresql.conf', 'autovacuum = off'); +$node->start; + +if (!$node->check_extension('injection_points')) +{ + plan skip_all => 'Extension injection_points not installed'; +} + +$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;'); +$node->safe_psql('postgres', 'CREATE EXTENSION amcheck;'); +$node->safe_psql('postgres', 'CREATE EXTENSION pageinspect;'); +$node->safe_psql('postgres', q[ +CREATE TABLE t (id int PRIMARY KEY, note text); +INSERT INTO t VALUES (1, 'original'); +]); + +# The offending session. The injection point fires inside +# CommitSubTransaction() right after the XID has been handed to the parent, so +# the subtransaction aborts while already in TRANS_COMMIT state. The PL/pgSQL +# EXCEPTION block swallows that error and the surrounding transaction lives on. +my $culprit = $node->background_psql('postgres', on_error_stop => 0); + +$culprit->query_safe(q[SELECT injection_points_set_local()]); +$culprit->query_safe( + q[SELECT injection_points_attach('subxact-after-childxids-transfer', 'error')] +); +$culprit->query_safe(q[BEGIN]); + +# note is not indexed and the page has room, so this is a HOT update. +$culprit->query(q[ +DO $$ +BEGIN + BEGIN + UPDATE t SET note = 'rolled-back-subxact' WHERE id = 1; + EXCEPTION WHEN OTHERS THEN + RAISE NOTICE 'subxact aborted: %', SQLERRM; + END; +END $$; +]); + +# The WARNING and NOTICE above arrive on stderr; take them and clear it, so that +# the later query_safe() calls do not mistake them for a failure. +my $subxact_err = $culprit->{stderr}; +$culprit->{stderr} = ''; + +like( + $subxact_err, + qr/AbortSubTransaction while in COMMIT state/, + 'subtransaction aborted after it had already subcommitted'); + +# A second session updates the same row. It sees the original version as live, +# since the subtransaction that tried to supersede it is aborted, and never +# examines the rolled-back version behind it. +$node->safe_psql('postgres', "UPDATE t SET note = 'other-session' WHERE id = 1;"); + +# Releasing the culprit is what does the damage: its commit record carries the +# aborted subtransaction in the child list, so that XID is marked COMMITTED. +$culprit->query_safe(q[COMMIT]); +$culprit->quit; + +diag("heap page after commit:\n" + . $node->safe_psql('postgres', q[ +SELECT lp, t_xmin, t_xmax, t_ctid, + (t_infomask & 256) <> 0 AS xmin_committed, + (t_infomask & 512) <> 0 AS xmin_invalid, + (t_infomask & 1024) <> 0 AS xmax_committed, + (t_infomask & 2048) <> 0 AS xmax_invalid +FROM heap_page_items(get_raw_page('t', 0)) ORDER BY lp;])); + +# A sequential scan and an index scan must agree on how many rows carry the key. +my $seqscan = $node->safe_psql('postgres', q[ +SET enable_indexscan = off; SET enable_bitmapscan = off; SET enable_indexonlyscan = off; +SELECT count(*) FROM t WHERE id = 1;]); +my $idxscan = $node->safe_psql('postgres', q[ +SET enable_seqscan = off; +SELECT count(*) FROM t WHERE id = 1;]); + +diag("rows with id = 1: seqscan $seqscan, index scan $idxscan"); + +is($seqscan, '1', 'primary key must hold a single row for the key'); +is($seqscan, $idxscan, 'sequential and index scan must agree'); + +# amcheck compares the index against the heap under a snapshot, so a live heap +# tuple with no index entry, or two live entries under a unique key, is +# reported as corruption. +my ($rc, $stdout, $stderr) = $node->psql('postgres', + "SELECT bt_index_check(index => 't_pkey'::regclass, heapallindexed => true, checkunique => true);" +); + +diag("amcheck says: $stderr") if $stderr ne ''; +is($rc, 0, 'amcheck must find the primary key intact'); + +# And rebuilding the index must not stumble over a duplicate. +my ($rc2, $stdout2, $stderr2) = + $node->psql('postgres', 'REINDEX TABLE t;'); + +diag("reindex says: $stderr2") if $stderr2 ne ''; +is($rc2, 0, 'the primary key must be rebuildable'); + +# The damage reached WAL in the parent's commit record, so recovery applies +# the same wrong status and the table comes back just as broken. +$node->stop('immediate'); +$node->start; + +my $seqscan_after = $node->safe_psql('postgres', q[ +SET enable_indexscan = off; SET enable_bitmapscan = off; SET enable_indexonlyscan = off; +SELECT count(*) FROM t WHERE id = 1;]); + +is($seqscan_after, '1', 'the key must still hold a single row after recovery'); + +done_testing(); -- 2.50.1 (Apple Git-155)