From 3ebcc94322080611ca73f76dcc9be2a5b477559a Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Thu, 20 Aug 2026 16:14:03 -0400 Subject: [PATCH v3] Export subxip[] for snapshots taken during recovery. A snapshot taken during recovery stores its whole in-progress set in subxip, every running top-level XID included, and leaves xip empty. Its suboverflowed flag therefore does not mean that subxip is redundant. We nevertheless treated it that way, which allowed queries running on standbys to see in-progress transactions as aborted. This could lead to hint bits being incorrectly set on standbys, which could result in wrong answers for sessions that never imported the snapshot. To fix, teach snapshot export to include the subxip[] array regardless of the overflow flag when its snapshot was taken during recovery. Claude Code found this problem. The committed TAP test is a simplified version of the one that it wrote to demonstrate this bug. Author: Peter Geoghegan Author: Bertrand Drouvot Discussion: https://postgr.es/m/CAH2-WzmHVeYY%3Dpjz9x8DhhxVjXHX0pvoQ-MdiB1Tt6%3Do2GTiKg%40mail.gmail.com Backpatch-through: 14 --- src/backend/utils/time/snapmgr.c | 42 ++-- src/test/recovery/meson.build | 1 + .../recovery/t/056_standby_snapshot_export.pl | 222 ++++++++++++++++++ 3 files changed, 249 insertions(+), 16 deletions(-) create mode 100644 src/test/recovery/t/056_standby_snapshot_export.pl diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c index 70aed370a..24f3ffb7a 100644 --- a/src/backend/utils/time/snapmgr.c +++ b/src/backend/utils/time/snapmgr.c @@ -1117,8 +1117,10 @@ ExportSnapshot(Snapshot snapshot) TransactionId topXid; TransactionId *children; ExportedSnapshot *esnap; + int nsubxids; int nchildren; int addTopXid; + bool suboverflowed; StringInfoData buf; FILE *f; MemoryContext oldcxt; @@ -1223,16 +1225,29 @@ ExportSnapshot(Snapshot snapshot) appendStringInfo(&buf, "xip:%u\n", topXid); /* - * Similarly, we add our subcommitted child XIDs to the subxid data. Here, - * we have to cope with possible overflow. + * Similarly, we add our subcommitted child XIDs to the subxid data. + * + * Report overflow when the snapshot overflowed, and also when our subxids + * won't fit in what a snapshot can hold; claiming overflow is always + * safe, since it just makes importers fall back on pg_subtrans. */ - if (snapshot->suboverflowed || - snapshot->subxcnt + nchildren > GetMaxSnapshotSubxidCount()) - appendStringInfoString(&buf, "sof:1\n"); - else + nsubxids = snapshot->subxcnt + nchildren; + suboverflowed = snapshot->suboverflowed || + nsubxids > GetMaxSnapshotSubxidCount(); + + /* + * Ignore the subxid array if it has overflowed, unless the snapshot was + * taken during recovery - in that case, top-level XIDs are in subxip as + * well, and we mustn't lose them. CopySnapshot() and SerializeSnapshot() + * make the same exception. + */ + if (suboverflowed && !snapshot->takenDuringRecovery) + nsubxids = 0; + + appendStringInfo(&buf, "sof:%u\n", suboverflowed); + appendStringInfo(&buf, "sxcnt:%d\n", nsubxids); + if (nsubxids > 0) { - appendStringInfoString(&buf, "sof:0\n"); - appendStringInfo(&buf, "sxcnt:%d\n", snapshot->subxcnt + nchildren); for (int32 i = 0; i < snapshot->subxcnt; i++) appendStringInfo(&buf, "sxp:%u\n", snapshot->subxip[i]); for (int32 i = 0; i < nchildren; i++) @@ -1493,11 +1508,11 @@ ImportSnapshot(const char *idstr) snapshot.xip[i] = parseXidFromText("xip:", &filebuf, path); snapshot.suboverflowed = parseIntFromText("sof:", &filebuf, path); + snapshot.subxcnt = xcnt = parseIntFromText("sxcnt:", &filebuf, path); + snapshot.subxip = NULL; - if (!snapshot.suboverflowed) + if (snapshot.subxcnt) { - snapshot.subxcnt = xcnt = parseIntFromText("sxcnt:", &filebuf, path); - /* sanity-check the xid count before palloc */ if (xcnt < 0 || xcnt > GetMaxSnapshotSubxidCount()) ereport(ERROR, @@ -1508,11 +1523,6 @@ ImportSnapshot(const char *idstr) for (i = 0; i < xcnt; i++) snapshot.subxip[i] = parseXidFromText("sxp:", &filebuf, path); } - else - { - snapshot.subxcnt = 0; - snapshot.subxip = NULL; - } snapshot.takenDuringRecovery = parseIntFromText("rec:", &filebuf, path); diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build index 39ec8c494..72113c5ac 100644 --- a/src/test/recovery/meson.build +++ b/src/test/recovery/meson.build @@ -64,6 +64,7 @@ tests += { 't/053_standby_login_event_trigger.pl', 't/054_unlogged_sequence_promotion.pl', 't/055_cascade_reconnect.pl', + 't/056_standby_snapshot_export.pl', ], }, } diff --git a/src/test/recovery/t/056_standby_snapshot_export.pl b/src/test/recovery/t/056_standby_snapshot_export.pl new file mode 100644 index 000000000..d5300336a --- /dev/null +++ b/src/test/recovery/t/056_standby_snapshot_export.pl @@ -0,0 +1,222 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group +# +# Test snapshot export and import on a standby. +# +# A snapshot taken during recovery keeps its whole in-progress set, including +# every running top-level XID, in subxip, and leaves xip empty. Exporting it +# must write subxip out even once the snapshot is marked suboverflowed, which +# happens as soon as a transaction on the primary reports 64 subtransactions. +# A buggy importer that loses subxip believes nothing at all is running between +# xmin and xmax, treats live transactions as aborted, and stamps +# HEAP_XMIN_INVALID/HEAP_XMAX_INVALID on their tuples. Every other session +# on the standby then sees those hint bits. + +use strict; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +# Enough to arm lastOverflowedXid on the standby (which happens at 64) and to +# overflow the primary's own subxid cache (at 65), so that later +# xl_running_xacts records keep it armed. +my $nsubxacts = 80; + +my $primary = PostgreSQL::Test::Cluster->new('primary'); +$primary->init(allows_streaming => 1); +$primary->append_conf('postgresql.conf', 'autovacuum = off'); +$primary->start; + +$primary->backup('backup'); +my $standby = PostgreSQL::Test::Cluster->new('standby'); +$standby->init_from_backup($primary, 'backup', has_streaming => 1); +$standby->append_conf('postgresql.conf', 'max_standby_streaming_delay = -1'); +$standby->start; + +# visibility_test holds the rows whose hint bits the test checks. xid_burner +# exists only to consume XIDs and subxids. +$primary->safe_psql( + 'postgres', q[ +CREATE TABLE visibility_test(k int, pad text); +INSERT INTO visibility_test + SELECT g, repeat('x', 1200) FROM generate_series(1, 40) g; +CREATE TABLE xid_burner(i int); +]); + +# Hold the primary's removal horizon below the deleting transaction, so that +# the row version it deletes stays RECENTLY_DEAD. Otherwise the re-INSERT +# prunes that version on the primary, and replay of the prune record removes +# the evidence from the standby. +my $horizon_holder = $primary->background_psql('postgres'); +$horizon_holder->query_safe( + 'BEGIN ISOLATION LEVEL REPEATABLE READ; + SELECT count(*) FROM visibility_test'); + +# This transaction deletes a row and stays open, so every snapshot taken from +# here on must report its XID as running. +my $toplevel_deleter = $primary->background_psql('postgres'); +$toplevel_deleter->query_safe('BEGIN'); +$toplevel_deleter->query_safe('DELETE FROM visibility_test WHERE k = 7'); +my $deleter_xid = + $toplevel_deleter->query_safe('SELECT pg_current_xact_id()'); + +# This transaction deletes another row in an early subtransaction, then +# overflows its subxid cache and stays open. Recovery removes the deleting +# subtransaction's XID from KnownAssignedXids, so checking that tuple's xmax +# later has to map the child XID back to its parent through pg_subtrans. +# Keeping the transaction open also keeps lastOverflowedXid armed on the +# standby. +my $subxact_deleter = $primary->background_psql('postgres'); +$subxact_deleter->query_safe('BEGIN'); +$subxact_deleter->query_safe('SAVEPOINT early'); +$subxact_deleter->query_safe('DELETE FROM visibility_test WHERE k = 8'); +$subxact_deleter->query_safe('RELEASE early'); +$subxact_deleter->query_safe( + qq[DO \$\$ BEGIN + FOR i IN 1..$nsubxacts LOOP + BEGIN INSERT INTO xid_burner VALUES (i); + EXCEPTION WHEN OTHERS THEN NULL; END; + END LOOP; END \$\$]); + +# Commit something so that the standby's xmax ends up past the deleting XID, +# and to flush the xid-assignment WAL that those subtransactions wrote. Then +# wait until the standby has removed the subxids and marks snapshots +# overflowed. Without the extra commit, XidInMVCCSnapshot() answers through +# its xid >= xmax range test, and the test would pass without examining the +# in-progress array. +$primary->safe_psql('postgres', 'INSERT INTO xid_burner VALUES (0)'); +$primary->wait_for_replay_catchup($standby); + +my $exporter = $standby->background_psql('postgres'); +$exporter->query_safe('BEGIN ISOLATION LEVEL REPEATABLE READ'); +my $recovery_snap = $exporter->query_safe('SELECT pg_export_snapshot()'); + +my $recovery_file = + slurp_file($standby->data_dir . "/pg_snapshots/$recovery_snap"); +note("exported snapshot $recovery_snap:\n$recovery_file"); + +like($recovery_file, qr/^rec:1$/m, 'snapshot was taken during recovery'); +like($recovery_file, qr/^sof:1$/m, 'snapshot is suboverflowed'); + +# Without these the test could pass while exercising nothing: an xmax below +# the deleting XID answers "not running" through the plain range test instead. +my ($exported_xmin) = $recovery_file =~ /^xmin:(\d+)$/m; +my ($exported_xmax) = $recovery_file =~ /^xmax:(\d+)$/m; +cmp_ok($exported_xmin, '<=', $deleter_xid, + 'exported xmin does not follow the running XID'); +cmp_ok($deleter_xid, '<', $exported_xmax, + 'running XID precedes exported xmax'); + +like($recovery_file, qr/^sxcnt:[1-9]/m, + 'suboverflowed recovery snapshot exports its subxip array'); +like($recovery_file, qr/^sxp:$deleter_xid$/m, 'running XID is exported'); + +# Import the snapshot and read the table. This answers correctly (a +# still-running transaction and an aborted one are indistinguishable here), +# but it is what writes the hint bits. +my $importer = $standby->background_psql('postgres'); +$importer->query_safe( + qq[BEGIN ISOLATION LEVEL REPEATABLE READ; + SET TRANSACTION SNAPSHOT '$recovery_snap']); +is($importer->query_safe('SELECT count(*) FROM visibility_test'), + 40, 'importing backend sees its own snapshot'); +$importer->query_safe('COMMIT'); +$exporter->query_safe('COMMIT'); + +# The deleting transaction commits, and the freed key is used again. +$toplevel_deleter->query_safe('COMMIT'); +$primary->safe_psql('postgres', + q[INSERT INTO visibility_test VALUES (7, repeat('y', 1200))]); +$primary->wait_for_replay_catchup($standby); + +# Sessions that never touched the exported snapshot must agree with the +# primary. A stale HEAP_XMAX_INVALID on the deleted row version brings it +# back to life, so the key is returned twice. +is( $standby->safe_psql( + 'postgres', 'SELECT count(*) FROM visibility_test WHERE k = 7'), + 1, + 'standby sees the re-inserted row once'); + +my $digest = + q[SELECT md5(string_agg(k::text, ',' ORDER BY k)) FROM visibility_test]; +is( $standby->safe_psql('postgres', $digest), + $primary->safe_psql('postgres', $digest), + 'primary and standby agree on table contents'); + +# pg_snapshots is only emptied at startup, and ImportSnapshot() takes rec: +# from the file rather than from RecoveryInProgress(). A snapshot exported +# by a standby therefore outlives promotion, and keeps taking +# XidInMVCCSnapshot()'s recovery branch on a server no longer in recovery. +my $promotion_exporter = $standby->background_psql('postgres'); +$promotion_exporter->query_safe('BEGIN ISOLATION LEVEL REPEATABLE READ'); +my $promotion_snap = + $promotion_exporter->query_safe('SELECT pg_export_snapshot()'); +my $promotion_count = + $promotion_exporter->query_safe('SELECT count(*) FROM visibility_test'); + +like(slurp_file($standby->data_dir . "/pg_snapshots/$promotion_snap"), + qr/^sof:1$/m, + 'snapshot saved for post-promotion re-export is suboverflowed'); + +# The deleting child XID is no longer in KnownAssignedXids, so this needs +# pg_subtrans to reach its parent. +$subxact_deleter->query_safe('COMMIT'); +$primary->wait_for_replay_catchup($standby); +is( $standby->safe_psql( + 'postgres', 'SELECT count(*) FROM visibility_test WHERE k = 8'), + 0, + 'standby sees the committed subtransaction delete'); + +$standby->promote; +$standby->poll_query_until('postgres', 'SELECT NOT pg_is_in_recovery()') + or die "standby never finished promotion"; + +my $reexporter = $standby->background_psql('postgres'); +$reexporter->query_safe( + qq[BEGIN ISOLATION LEVEL REPEATABLE READ; + SET TRANSACTION SNAPSHOT '$promotion_snap']); +is($reexporter->query_safe('SELECT count(*) FROM visibility_test'), + $promotion_count, + 'recovery-taken snapshot still imports after promotion'); + +# That importing transaction is read-write, since the read-only requirement +# binds only a SERIALIZABLE source. So it can acquire subcommitted children +# and export again, which hands ExportSnapshot() a snapshot taken during +# recovery together with a non-empty children array. The subxip array must +# still be written out. +$reexporter->query_safe('SAVEPOINT sp'); +$reexporter->query_safe( + q[INSERT INTO visibility_test VALUES (5000, repeat('z', 10))]); +$reexporter->query_safe('RELEASE sp'); +my $reexported_snap = $reexporter->query_safe('SELECT pg_export_snapshot()'); + +my $reexported_file = + slurp_file($standby->data_dir . "/pg_snapshots/$reexported_snap"); +like($reexported_file, qr/^sof:1$/m, + 're-exported recovery snapshot remains suboverflowed'); +like($reexported_file, qr/^sxcnt:[1-9]/m, + 'a recovery-taken snapshot re-exports its subxip array after a write'); + +my $reimporter = $standby->background_psql('postgres'); +$reimporter->query_safe( + qq[BEGIN ISOLATION LEVEL REPEATABLE READ; + SET TRANSACTION SNAPSHOT '$reexported_snap']); +is($reimporter->query_safe('SELECT count(*) FROM visibility_test'), + $promotion_count, 're-exported recovery snapshot can be imported'); + +$reimporter->query_safe('COMMIT'); +$reexporter->query_safe('COMMIT'); +$promotion_exporter->query_safe('COMMIT'); + +$horizon_holder->quit; +$toplevel_deleter->quit; +$subxact_deleter->quit; +$exporter->quit; +$importer->quit; +$promotion_exporter->quit; +$reexporter->quit; +$reimporter->quit; +$standby->stop; +$primary->stop; + +done_testing(); -- 2.53.0