From: Andrey Rachitskiy Date: Wed, 12 Aug 2026 20:46:33 +0500 Subject: [PATCH v2] Fix pg_xact corruption from subtransaction abort after subcommit CommitSubTransaction() calls AtSubCommit_childXids() to copy this subtransaction's XID and its committed grandchildren into the parent's list of committed children. Several fallible steps run after that while the subtransaction is still in TRANS_COMMIT state. If one of them throws (for example, out of memory), the subtransaction aborts while in COMMIT state. RecordTransactionAbort() marks its XID aborted in pg_xact, but nothing removes that XID from the parent's committed-child list; AtSubAbort_childXids() only frees the child's own array. When the parent commits, TransactionIdSetTreeStatus() walks the list and tries to mark the aborted XID committed. That trips the assertion in TransactionIdSetStatusBit() in an assert build, and writes the wrong pg_xact status otherwise. The same list is stored in the parent's commit WAL record, so replay hits it too and recovery cannot complete. Have AtSubCommit_childXids() save the parent's child count before it appends, and have AbortSubTransaction() restore it when the subtransaction aborts after the transfer. The appended entries are at the tail of the parent's array, so restoring the saved length removes exactly this subtransaction's XID and its grandchildren; the grandchildren revert to implicitly aborted, which is correct because the whole subtree is rolling back. The restore runs ahead of the curTransactionOwner-guarded cleanup, so it happens whenever AtSubCommit_childXids() ran. Initialize savedParentNChildXids in StartTransaction() and PushTransaction(), per review. Add a small test module that raises from a SUBXACT_EVENT_COMMIT_SUB callback after the child-XID transfer. t/001_subxact_resurrect_pk.pl adapts Andrey Borodin's demonstration to that callback (HOT update, second session, amcheck, REINDEX, recovery) and asserts correct behaviour. No --enable-injection-points is required, so the same test approach can be used on back branches. Author: Bryan Green Co-authored-by: Andrey Borodin Co-authored-by: Andrey Rachitskiy --- src/backend/access/transam/xact.c | 28 ++++ src/test/modules/Makefile | 1 + src/test/modules/meson.build | 1 + .../modules/test_subxact_commit/.gitignore | 4 + src/test/modules/test_subxact_commit/Makefile | 25 +++ .../modules/test_subxact_commit/meson.build | 33 ++++ .../t/001_subxact_resurrect_pk.pl | 146 ++++++++++++++++++ .../test_subxact_commit--1.0.sql | 11 ++ .../test_subxact_commit/test_subxact_commit.c | 68 ++++++++ .../test_subxact_commit.control | 4 + 10 files changed, 321 insertions(+) create mode 100644 src/test/modules/test_subxact_commit/.gitignore create mode 100644 src/test/modules/test_subxact_commit/Makefile create mode 100644 src/test/modules/test_subxact_commit/meson.build create mode 100644 src/test/modules/test_subxact_commit/t/001_subxact_resurrect_pk.pl create mode 100644 src/test/modules/test_subxact_commit/test_subxact_commit--1.0.sql create mode 100644 src/test/modules/test_subxact_commit/test_subxact_commit.c create mode 100644 src/test/modules/test_subxact_commit/test_subxact_commit.control diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 3a89149016f..4cfc0b64b52 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -208,6 +208,8 @@ typedef struct TransactionStateData TransactionId *childXids; /* subcommitted child XIDs, in XID order */ int nChildXids; /* # of subcommitted child XIDs */ int maxChildXids; /* allocated size of childXids[] */ + int savedParentNChildXids; /* parent's nChildXids before + * subcommit transfer, or -1 */ Oid prevUser; /* previous CurrentUserId setting */ int prevSecContext; /* previous SecurityRestrictionContext */ bool prevXactReadOnly; /* entry-time xact r/o state */ @@ -1710,6 +1712,12 @@ AtSubCommit_childXids(void) Assert(s->parent != NULL); + /* + * Remember the parent's child count so a later abort can undo this + * transfer (see AbortSubTransaction). + */ + s->savedParentNChildXids = s->parent->nChildXids; + /* * The parent childXids array will need to hold my XID and all my * childXids, in addition to the XIDs already there. @@ -2143,6 +2151,7 @@ StartTransaction(void) s->childXids = NULL; s->nChildXids = 0; s->maxChildXids = 0; + s->savedParentNChildXids = -1; /* * Once the current user ID and the security context flags are fetched, @@ -5343,6 +5352,24 @@ AbortSubTransaction(void) s->state = TRANS_ABORT; + /* + * If AtSubCommit_childXids() moved our XID and childXids up to the + * parent, undo that here. An error escaping the later steps of + * CommitSubTransaction() aborts us in COMMIT state; leaving our aborted + * XID in the parent's committed-child list would make the parent's commit + * mark it committed and corrupt pg_xact. Our entries are the tail of the + * parent's array, so restoring the saved length drops exactly them. Done + * before the curTransactionOwner check below so it runs whenever + * AtSubCommit_childXids() did. + */ + if (s->savedParentNChildXids >= 0) + { + Assert(s->parent != NULL); + Assert(s->parent->nChildXids >= s->savedParentNChildXids); + s->parent->nChildXids = s->savedParentNChildXids; + s->savedParentNChildXids = -1; + } + /* * Reset user ID which might have been changed transiently. (See notes in * AbortTransaction.) @@ -5515,6 +5542,7 @@ PushTransaction(void) s->parallelModeLevel = 0; s->parallelChildXact = (p->parallelModeLevel != 0 || p->parallelChildXact); s->topXidLogged = false; + s->savedParentNChildXids = -1; CurrentTransactionState = s; diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile index 098bb8142ae..446c514b720 100644 --- a/src/test/modules/Makefile +++ b/src/test/modules/Makefile @@ -52,6 +52,7 @@ SUBDIRS = \ test_shmem \ test_shm_mq \ test_slru \ + test_subxact_commit \ test_tidstore \ unsafe_tests \ worker_spi \ diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build index 4bca42bb370..3bbe09c06d2 100644 --- a/src/test/modules/meson.build +++ b/src/test/modules/meson.build @@ -53,6 +53,7 @@ subdir('test_saslprep') subdir('test_shmem') subdir('test_shm_mq') subdir('test_slru') +subdir('test_subxact_commit') subdir('test_tidstore') subdir('typcache') subdir('unsafe_tests') diff --git a/src/test/modules/test_subxact_commit/.gitignore b/src/test/modules/test_subxact_commit/.gitignore new file mode 100644 index 00000000000..5dcb3ff9723 --- /dev/null +++ b/src/test/modules/test_subxact_commit/.gitignore @@ -0,0 +1,4 @@ +# Generated subdirectories +/log/ +/results/ +/tmp_check/ diff --git a/src/test/modules/test_subxact_commit/Makefile b/src/test/modules/test_subxact_commit/Makefile new file mode 100644 index 00000000000..b3db1bc52f8 --- /dev/null +++ b/src/test/modules/test_subxact_commit/Makefile @@ -0,0 +1,25 @@ +# src/test/modules/test_subxact_commit/Makefile + +MODULE_big = test_subxact_commit +OBJS = \ + $(WIN32RES) \ + test_subxact_commit.o +PGFILEDESC = "test_subxact_commit - force errors at subtransaction commit" + +EXTENSION = test_subxact_commit +DATA = test_subxact_commit--1.0.sql + +TAP_TESTS = 1 + +EXTRA_INSTALL = contrib/amcheck contrib/pageinspect + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = src/test/modules/test_subxact_commit +top_builddir = ../../../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif diff --git a/src/test/modules/test_subxact_commit/meson.build b/src/test/modules/test_subxact_commit/meson.build new file mode 100644 index 00000000000..b354452efc9 --- /dev/null +++ b/src/test/modules/test_subxact_commit/meson.build @@ -0,0 +1,33 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +test_subxact_commit_sources = files( + 'test_subxact_commit.c', +) + +if host_system == 'windows' + test_subxact_commit_sources += rc_lib_gen.process(win32ver_rc, extra_args: [ + '--NAME', 'test_subxact_commit', + '--FILEDESC', 'test_subxact_commit - force errors at subtransaction commit',]) +endif + +test_subxact_commit = shared_module('test_subxact_commit', + test_subxact_commit_sources, + kwargs: pg_test_mod_args, +) +test_install_libs += test_subxact_commit + +test_install_data += files( + 'test_subxact_commit.control', + 'test_subxact_commit--1.0.sql', +) + +tests += { + 'name': 'test_subxact_commit', + 'sd': meson.current_source_dir(), + 'bd': meson.current_build_dir(), + 'tap': { + 'tests': [ + 't/001_subxact_resurrect_pk.pl', + ], + }, +} diff --git a/src/test/modules/test_subxact_commit/t/001_subxact_resurrect_pk.pl b/src/test/modules/test_subxact_commit/t/001_subxact_resurrect_pk.pl new file mode 100644 index 00000000000..3d8c2e41b7d --- /dev/null +++ b/src/test/modules/test_subxact_commit/t/001_subxact_resurrect_pk.pl @@ -0,0 +1,146 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group +# +# Demonstrate that a subtransaction which aborts after it has already +# subcommitted would corrupt a primary key without the childXids restore +# in AbortSubTransaction(). +# +# AtSubCommit_childXids() copies the subtransaction's XID into the parent's +# committed-children array before CommitSubTransaction() is finished. The +# test_subxact_commit.force_error GUC makes the COMMIT_SUB callback raise +# after that transfer. Control longjmps into AbortSubTransaction(), which +# records the XID aborted but, without the fix, 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 would be live under the same +# key without the fix. +# +# The checks below assert correct behaviour, so on unpatched non-cassert +# builds they fail and print the corruption. A build with --enable-cassert +# trips the assertion in clog.c on the culprit COMMIT instead. + +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +my $node = PostgreSQL::Test::Cluster->new('subxact_resurrect_pk'); +$node->init; +$node->append_conf('postgresql.conf', 'autovacuum = off'); +$node->start; + +# Extension is installed with this test module. +$node->safe_psql('postgres', 'CREATE EXTENSION test_subxact_commit;'); +$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. force_error raises from COMMIT_SUB 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); + +# Load the module in this backend. SET of a custom GUC alone only creates a +# placeholder if the .so is not loaded, so the COMMIT_SUB callback would never +# run. +$culprit->query_safe(q[SELECT test_subxact_commit_init()]); + +$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 + SET LOCAL test_subxact_commit.force_error = on; + 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 without the fix: 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 would +# apply the same wrong status without the fix. +$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(); diff --git a/src/test/modules/test_subxact_commit/test_subxact_commit--1.0.sql b/src/test/modules/test_subxact_commit/test_subxact_commit--1.0.sql new file mode 100644 index 00000000000..165870a02ec --- /dev/null +++ b/src/test/modules/test_subxact_commit/test_subxact_commit--1.0.sql @@ -0,0 +1,11 @@ +/* src/test/modules/test_subxact_commit/test_subxact_commit--1.0.sql */ + +-- complain if script is sourced in psql, rather than via CREATE EXTENSION +\echo Use "CREATE EXTENSION test_subxact_commit" to load this file. \quit + +-- Forces the shared library to load so _PG_init can register the callback +-- and define test_subxact_commit.force_error. +CREATE FUNCTION test_subxact_commit_init() + RETURNS void + AS 'MODULE_PATHNAME', 'test_subxact_commit_init' + LANGUAGE C; diff --git a/src/test/modules/test_subxact_commit/test_subxact_commit.c b/src/test/modules/test_subxact_commit/test_subxact_commit.c new file mode 100644 index 00000000000..326f6884ce4 --- /dev/null +++ b/src/test/modules/test_subxact_commit/test_subxact_commit.c @@ -0,0 +1,68 @@ +/*------------------------------------------------------------------------- + * + * test_subxact_commit.c + * Helpers to test subtransaction commit failure handling. + * + * Loading this module registers a SubXactCallback. With + * test_subxact_commit.force_error = on, the COMMIT_SUB callback raises + * ERROR after AtSubCommit_childXids() has already published the subxact + * XID into the parent's childXids list. That is the window that used to + * corrupt pg_xact when the error was caught and the outer transaction + * later committed. See t/001_subxact_resurrect_pk.pl. + * + * Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * IDENTIFICATION + * src/test/modules/test_subxact_commit/test_subxact_commit.c + * + *------------------------------------------------------------------------- + */ + +#include "postgres.h" + +#include "access/xact.h" +#include "fmgr.h" +#include "utils/guc.h" + +PG_MODULE_MAGIC; + +static bool force_commit_error = false; + +static void +test_subxact_commit_cb(SubXactEvent event, + SubTransactionId mySubid, + SubTransactionId parentSubid, + void *arg) +{ + if (force_commit_error && event == SUBXACT_EVENT_COMMIT_SUB) + ereport(ERROR, + (errcode(ERRCODE_INTERNAL_ERROR), + errmsg("forced error on subtransaction commit"))); +} + +/* + * Dummy SQL entry point so CREATE EXTENSION loads the shared library + * (and thus runs _PG_init) without needing shared_preload_libraries. + */ +PG_FUNCTION_INFO_V1(test_subxact_commit_init); +Datum +test_subxact_commit_init(PG_FUNCTION_ARGS) +{ + PG_RETURN_VOID(); +} + +void +_PG_init(void) +{ + DefineCustomBoolVariable("test_subxact_commit.force_error", + "Raise ERROR from SUBXACT_EVENT_COMMIT_SUB callback.", + NULL, + &force_commit_error, + false, + PGC_USERSET, + 0, + NULL, NULL, NULL); + + RegisterSubXactCallback(test_subxact_commit_cb, NULL); +} diff --git a/src/test/modules/test_subxact_commit/test_subxact_commit.control b/src/test/modules/test_subxact_commit/test_subxact_commit.control new file mode 100644 index 00000000000..1c5c2792d4e --- /dev/null +++ b/src/test/modules/test_subxact_commit/test_subxact_commit.control @@ -0,0 +1,4 @@ +comment = 'Test code for subtransaction commit error handling' +default_version = '1.0' +module_pathname = '$libdir/test_subxact_commit' +relocatable = true -- 2.53.0