From 2c17ded222408a12caf80093aad5c745eeabcacc Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Sun, 26 Jul 2026 21:17:34 -0400
Subject: [PATCH v1 1/2] Don't discard subxip when exporting a snapshot taken
 during recovery

A snapshot taken during recovery stores every running XID -- top-level ones
included -- in subxip, leaving xip empty.  CopySnapshot(),
EstimateSnapshotSpace() and SerializeSnapshot() each make an exception for
such a snapshot when subxip has overflowed, since discarding the array would
lose the only record of what is running.  ExportSnapshot() never got that
exception: it wrote "sof:1" and dropped the array whenever the snapshot was
suboverflowed, which on a standby happens as soon as some transaction on the
primary reports 64 subtransactions.

A backend importing such a file then held a snapshot whose in-progress set
was empty, and XidInMVCCSnapshot() reported every XID between xmin and xmax
as no longer running.  HeapTupleSatisfiesMVCC() took live transactions for
aborted ones and stamped HEAP_XMIN_INVALID or HEAP_XMAX_INVALID on their
tuples.  Those hint bits are set with InvalidTransactionId, which skips the
commit-LSN interlock, so they were applied immediately and were seen by every
other session on the standby, not just by the importer: rows that a committed
transaction had deleted came back to life, rows it had inserted disappeared,
and a unique key could be returned twice.  Only a full page image from the
primary put the page right again.

Export the array for a snapshot taken during recovery, keeping "sof:1" so
that the importer still consults pg_subtrans for the subtransactions that
KnownAssignedXidsRemoveTree() has already dropped.  "rec:" now has to be
written before "sof:", because ImportSnapshot() parses strictly front to back
and must know whether an overflowed array is still meaningful before reading
it.  Nothing outside these two functions reads these files and pg_snapshots
is emptied at startup, so the format change needs no compatibility handling.

Such a snapshot usually belongs to a transaction that can have no XID, and so
no subcommitted children, but it outlives promotion: a transaction on the
promoted server can import one, write, and export again, so the children are
added here as they are in the non-overflowed case.  The combined count can in
principle exceed what the importer will accept, since a recovery snapshot's
subxip is bounded by the same value; there is no way to represent such a
snapshot, so refuse to export it rather than write an array that is quietly
missing entries.

Also assert in SetTransactionSnapshot() that a snapshot taken during recovery
whose XID range is not empty has a non-empty subxip, which is what makes this
class of mistake visible rather than silent.

Reachable through pg_export_snapshot() on a standby since 6c2003f8a1b, and
pg_dump -j is enough to hit it.
---
 src/backend/utils/time/snapmgr.c              |  62 +++++-
 src/test/recovery/meson.build                 |   1 +
 .../recovery/t/055_standby_snapshot_export.pl | 191 ++++++++++++++++++
 3 files changed, 250 insertions(+), 4 deletions(-)
 create mode 100644 src/test/recovery/t/055_standby_snapshot_export.pl

diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index bc98a4361..beb1a6391 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -548,6 +548,16 @@ SetTransactionSnapshot(Snapshot sourcesnap, VirtualTransactionId *sourcevxid,
 	CurrentSnapshot->takenDuringRecovery = sourcesnap->takenDuringRecovery;
 	/* NB: curcid should NOT be copied, it's a local matter */
 
+	/*
+	 * A snapshot taken during recovery keeps every running XID in subxip, and
+	 * its xmin is the oldest of them, so an empty subxip means that nothing
+	 * was running.  Catch a source that lost the array along the way: such a
+	 * snapshot silently reports running transactions as no longer running.
+	 */
+	Assert(!CurrentSnapshot->takenDuringRecovery ||
+		   CurrentSnapshot->subxcnt > 0 ||
+		   CurrentSnapshot->xmin == CurrentSnapshot->xmax);
+
 	CurrentSnapshot->snapXactCompletionCount = 0;
 
 	/*
@@ -1222,13 +1232,54 @@ ExportSnapshot(Snapshot snapshot)
 	if (addTopXid)
 		appendStringInfo(&buf, "xip:%u\n", topXid);
 
+	/*
+	 * The importer has to know whether the snapshot was taken during recovery
+	 * before it reads the subxid data, since that determines whether an
+	 * overflowed subxip array is still meaningful.  Emit it first.
+	 */
+	appendStringInfo(&buf, "rec:%u\n", snapshot->takenDuringRecovery);
+
 	/*
 	 * Similarly, we add our subcommitted child XIDs to the subxid data. Here,
 	 * we have to cope with possible overflow.
+	 *
+	 * 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.
+	 *
+	 * Such a snapshot usually belongs to a transaction that can have no XID,
+	 * and hence no subcommitted children, since it was taken while the server
+	 * was still in recovery.  It can outlive promotion, though: a transaction
+	 * on the promoted server can import one and then write.
 	 */
 	if (snapshot->suboverflowed ||
 		snapshot->subxcnt + nchildren > GetMaxSnapshotSubxidCount())
+	{
 		appendStringInfoString(&buf, "sof:1\n");
+		if (snapshot->takenDuringRecovery)
+		{
+			/*
+			 * The importer's array is bounded by the same value, so a snapshot
+			 * that does not fit cannot be represented at all.  Refusing to
+			 * export it is the only honest answer: an array quietly missing
+			 * entries tells the importer that transactions which are still
+			 * running have finished.
+			 */
+			if (snapshot->subxcnt + nchildren > GetMaxSnapshotSubxidCount())
+				ereport(ERROR,
+						(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+						 errmsg("cannot export a snapshot containing %d transaction IDs",
+								snapshot->subxcnt + nchildren),
+						 errdetail("Snapshots taken during recovery record every running transaction ID, and this one exceeds the limit of %d.",
+								   GetMaxSnapshotSubxidCount())));
+
+			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++)
+				appendStringInfo(&buf, "sxp:%u\n", children[i]);
+		}
+	}
 	else
 	{
 		appendStringInfoString(&buf, "sof:0\n");
@@ -1238,7 +1289,6 @@ ExportSnapshot(Snapshot snapshot)
 		for (int32 i = 0; i < nchildren; i++)
 			appendStringInfo(&buf, "sxp:%u\n", children[i]);
 	}
-	appendStringInfo(&buf, "rec:%u\n", snapshot->takenDuringRecovery);
 
 	/*
 	 * Now write the text representation into a file.  We first write to a
@@ -1492,9 +1542,15 @@ ImportSnapshot(const char *idstr)
 	for (i = 0; i < xcnt; i++)
 		snapshot.xip[i] = parseXidFromText("xip:", &filebuf, path);
 
+	snapshot.takenDuringRecovery = parseIntFromText("rec:", &filebuf, path);
 	snapshot.suboverflowed = parseIntFromText("sof:", &filebuf, path);
 
-	if (!snapshot.suboverflowed)
+	/*
+	 * An overflowed subxip array carries no information and is not written
+	 * out, except for a snapshot taken during recovery: that keeps all running
+	 * XIDs there, so it is written out and must be read back.
+	 */
+	if (!snapshot.suboverflowed || snapshot.takenDuringRecovery)
 	{
 		snapshot.subxcnt = xcnt = parseIntFromText("sxcnt:", &filebuf, path);
 
@@ -1514,8 +1570,6 @@ ImportSnapshot(const char *idstr)
 		snapshot.subxip = NULL;
 	}
 
-	snapshot.takenDuringRecovery = parseIntFromText("rec:", &filebuf, path);
-
 	/*
 	 * Do some additional sanity checking, just to protect ourselves.  We
 	 * don't trouble to check the array elements, just the most critical
diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build
index ad0d85f41..8f24a1686 100644
--- a/src/test/recovery/meson.build
+++ b/src/test/recovery/meson.build
@@ -63,6 +63,7 @@ tests += {
       't/052_checkpoint_segment_missing.pl',
       't/053_standby_login_event_trigger.pl',
       't/054_unlogged_sequence_promotion.pl',
+      't/055_standby_snapshot_export.pl',
     ],
   },
 }
diff --git a/src/test/recovery/t/055_standby_snapshot_export.pl b/src/test/recovery/t/055_standby_snapshot_export.pl
new file mode 100644
index 000000000..139190cf2
--- /dev/null
+++ b/src/test/recovery/t/055_standby_snapshot_export.pl
@@ -0,0 +1,191 @@
+# Copyright (c) 2026, PostgreSQL Global Development Group
+#
+# Test snapshot export and import on a standby.
+#
+# A snapshot taken during recovery stores every running XID -- top-level ones
+# included -- in subxip, leaving xip empty.  Once such a snapshot is also
+# marked suboverflowed, which happens as soon as some transaction on the
+# primary reports 64 subtransactions, exporting it must still write subxip
+# out: an importer that loses it 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.  Those hint bits are
+# then seen by every other session on the standby.
+
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Enough XID-acquiring subtransactions 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;
+
+# Checksums are off on purpose.  With XLogHintBitIsNeeded(),
+# MarkSharedBufferDirtyHint() declines to dirty a page for a hint bit set
+# during recovery, so a wrong hint would live in shared buffers only.
+my $primary = PostgreSQL::Test::Cluster->new('primary');
+$primary->init(allows_streaming => 1, no_data_checksums => 1);
+$primary->append_conf(
+	'postgresql.conf', q[
+autovacuum = off
+checkpoint_timeout = 1h
+max_wal_size = 10GB
+]);
+$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', q[
+hot_standby_feedback = off
+max_standby_streaming_delay = -1
+]);
+$standby->start;
+
+$primary->safe_psql(
+	'postgres', q[
+CREATE TABLE victim(k int PRIMARY KEY, pad text);
+INSERT INTO victim SELECT g, repeat('x', 1200) FROM generate_series(1, 40) g;
+CREATE TABLE burner(i int);
+]);
+
+# Hold the primary's removal horizon below U so that the version U deletes
+# stays RECENTLY_DEAD.  Otherwise the re-INSERT prunes it on the primary and
+# replay of the prune record removes the evidence from the standby.
+my $guard = $primary->background_psql('postgres');
+$guard->query_safe(
+	'BEGIN ISOLATION LEVEL REPEATABLE READ; SELECT count(*) FROM victim');
+
+# U deletes a row and stays open, so every snapshot taken from here on must
+# report its XID as running.
+my $u = $primary->background_psql('postgres');
+$u->query_safe('BEGIN');
+$u->query_safe('DELETE FROM victim WHERE k = 7');
+my $u_xid = $u->query_safe('SELECT pg_current_xact_id()');
+
+# O overflows the subxid cache and stays open, keeping lastOverflowedXid
+# armed on the standby for as long as it lives.
+my $o = $primary->background_psql('postgres');
+$o->query_safe('BEGIN');
+$o->query_safe(
+	qq[DO \$\$ BEGIN
+	     FOR i IN 1..$nsubxacts LOOP
+	       BEGIN INSERT INTO burner VALUES (i); EXCEPTION WHEN OTHERS THEN NULL; END;
+	     END LOOP; END \$\$]);
+
+# Commit something so that the standby's xmax ends up past U's XID.  Without
+# this, XidInMVCCSnapshot() answers via its xid >= xmax range test and the
+# test would pass without exercising anything.
+$primary->safe_psql('postgres', 'INSERT INTO burner VALUES (0)');
+$primary->wait_for_replay_catchup($standby);
+
+my $s1 = $standby->background_psql('postgres');
+$s1->query_safe('BEGIN ISOLATION LEVEL REPEATABLE READ');
+my $snap = $s1->query_safe('SELECT pg_export_snapshot()');
+
+my $file = slurp_file($standby->data_dir . "/pg_snapshots/$snap");
+note("exported snapshot $snap:\n$file");
+
+like($file, qr/^rec:1$/m, 'snapshot was taken during recovery');
+like($file, qr/^sof:1$/m, 'snapshot is suboverflowed');
+
+# Without these the test could pass while exercising nothing: an xmax below
+# U's XID answers "not running" through the plain range test instead.
+my ($xmin) = $file =~ /^xmin:(\d+)$/m;
+my ($xmax) = $file =~ /^xmax:(\d+)$/m;
+cmp_ok($xmin, '<=', $u_xid, 'exported xmin does not follow the running XID');
+cmp_ok($u_xid, '<', $xmax, 'running XID precedes exported xmax');
+
+like($file, qr/^sxcnt:[1-9]/m,
+	'suboverflowed recovery snapshot exports its subxip array');
+like($file, qr/^sxp:$u_xid$/m, 'running XID is exported');
+
+# Import the snapshot and read the victim page.  This answers correctly --
+# a transaction that is still running and one that aborted are
+# indistinguishable here -- but it is what writes the hint bits.
+my $s2 = $standby->background_psql('postgres');
+$s2->query_safe(
+	qq[BEGIN ISOLATION LEVEL REPEATABLE READ;
+	   SET TRANSACTION SNAPSHOT '$snap';
+	   SET enable_indexscan = off;
+	   SET enable_bitmapscan = off;
+	   SET enable_indexonlyscan = off]);
+is($s2->query_safe('SELECT count(*) FROM victim'),
+	40, 'importing backend sees its own snapshot');
+$s2->query_safe('COMMIT');
+$s1->query_safe('COMMIT');
+
+# U commits and the freed key is used again.
+$u->query_safe('COMMIT');
+$primary->safe_psql('postgres',
+	q[INSERT INTO victim VALUES (7, repeat('y', 1200))]);
+$o->query_safe('COMMIT');
+$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 version U deleted brings the old
+# row back to life, so the key is returned twice.
+is($standby->safe_psql('postgres', 'SELECT count(*) FROM victim WHERE k = 7'),
+	1, 'standby sees the re-inserted row once');
+
+is( $standby->safe_psql(
+		'postgres',
+		'SELECT count(*) FROM (SELECT k FROM victim GROUP BY k HAVING count(*) > 1) d'
+	),
+	0, 'no duplicate keys on standby');
+
+my $digest = q[SELECT md5(string_agg(k::text, ',' ORDER BY k)) FROM victim];
+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(), so a snapshot exported by a
+# standby outlives promotion and keeps taking XidInMVCCSnapshot()'s recovery
+# branch on a server that is no longer in recovery.
+my $s3 = $standby->background_psql('postgres');
+$s3->query_safe('BEGIN ISOLATION LEVEL REPEATABLE READ');
+my $snap2 = $s3->query_safe('SELECT pg_export_snapshot()');
+my $before = $s3->query_safe('SELECT count(*) FROM victim');
+
+$standby->promote;
+$standby->poll_query_until('postgres', 'SELECT NOT pg_is_in_recovery()')
+  or die "standby never finished promotion";
+
+my $s4 = $standby->background_psql('postgres');
+$s4->query_safe(
+	qq[BEGIN ISOLATION LEVEL REPEATABLE READ;
+	   SET TRANSACTION SNAPSHOT '$snap2']);
+is($s4->query_safe('SELECT count(*) FROM victim'),
+	$before, '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.
+$s4->query_safe('SAVEPOINT sp');
+$s4->query_safe(q[INSERT INTO victim VALUES (5000, repeat('z', 10))]);
+$s4->query_safe('RELEASE sp');
+my $snap3 = $s4->query_safe('SELECT pg_export_snapshot()');
+
+my $file3 = slurp_file($standby->data_dir . "/pg_snapshots/$snap3");
+like($file3, qr/^sxcnt:[1-9]/m,
+	'a recovery-taken snapshot re-exports its subxip array after a write');
+
+$s4->query_safe('COMMIT');
+$s3->query_safe('COMMIT');
+
+$guard->quit;
+$u->quit;
+$o->quit;
+$s1->quit;
+$s2->quit;
+$s3->quit;
+$s4->quit;
+$standby->stop;
+$primary->stop;
+
+done_testing();
-- 
2.53.0

