agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Peter Geoghegan <pg@bowt.ie>
Subject: [PATCH v2 1/2] Don't discard subxip when exporting a snapshot taken during recovery
Date: Sun, 26 Jul 2026 21:17:34 -0400
A snapshot taken during recovery stores its in-progress set in subxip,
including every running top-level XID, and leaves xip empty.
ExportSnapshot() nevertheless discarded that array whenever suboverflowed
was set. An importer then treated live transactions as completed and
could set incorrect HEAP_XMIN_INVALID or HEAP_XMAX_INVALID hint bits on
the standby.
Export overflowed recovery subxip arrays and teach ImportSnapshot() to
read them. Keep the overflow flag so XidInMVCCSnapshot() still consults
pg_subtrans for children already removed from KnownAssignedXids.
A recovery snapshot can survive promotion, after which the importing
transaction can acquire committed children and export it again. Filter
recovery subxip entries and those children to [xmin, xmax) before
counting and writing them. Reject a snapshot that still exceeds the
import limit before pseudo-registering it, avoiding leftover cleanup
state on error.
Add a recovery TAP test covering the standby corruption scenario,
pg_subtrans fallback, and import/re-export across promotion.
Co-authored-by: Peter Geoghegan <pg@bowt.ie>
Co-authored-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Discussion: https://postgr.es/m/CAH2-WzmHVeYY%3Dpjz9x8DhhxVjXHX0pvoQ-MdiB1Tt6%3Do2GTiKg%40mail.gmail.com
---
src/backend/utils/time/snapmgr.c | 112 ++++++++-
src/test/recovery/meson.build | 1 +
.../recovery/t/055_standby_snapshot_export.pl | 227 ++++++++++++++++++
3 files changed, 330 insertions(+), 10 deletions(-)
32.3% src/backend/utils/time/
67.3% src/test/recovery/t/
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index bc98a4361bf..48c2f56b4d9 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -548,6 +548,17 @@ 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 its in-progress set in subxip,
+ * including every running top-level XID. 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;
/*
@@ -1118,7 +1129,9 @@ ExportSnapshot(Snapshot snapshot)
TransactionId *children;
ExportedSnapshot *esnap;
int nchildren;
+ int nsubxids;
int addTopXid;
+ bool write_subxids;
StringInfoData buf;
FILE *f;
MemoryContext oldcxt;
@@ -1161,6 +1174,47 @@ ExportSnapshot(Snapshot snapshot)
*/
nchildren = xactGetCommittedChildren(&children);
+ /*
+ * SnapshotData allows subxip entries outside [xmin, xmax), but they carry
+ * no information in an exported snapshot and cannot be represented in a
+ * pg_snapshot. This matters for a recovery snapshot that survives
+ * promotion: committed children acquired afterwards are at or above the
+ * old xmax. Filter such entries before counting them.
+ */
+ if (snapshot->takenDuringRecovery)
+ {
+ nsubxids = 0;
+ for (int32 i = 0; i < snapshot->subxcnt; i++)
+ {
+ if (TransactionIdFollowsOrEquals(snapshot->subxip[i],
+ snapshot->xmin) &&
+ TransactionIdPrecedes(snapshot->subxip[i], snapshot->xmax))
+ nsubxids++;
+ }
+ for (int32 i = 0; i < nchildren; i++)
+ {
+ if (TransactionIdFollowsOrEquals(children[i], snapshot->xmin) &&
+ TransactionIdPrecedes(children[i], snapshot->xmax))
+ nsubxids++;
+ }
+
+ /*
+ * The importer's array is bounded by the same value, so a recovery
+ * snapshot that does not fit cannot be represented at all. Check
+ * before pseudo-registering the exported snapshot, so that an error
+ * cannot leave cleanup state behind.
+ */
+ if (nsubxids > GetMaxSnapshotSubxidCount())
+ ereport(ERROR,
+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("cannot export a snapshot containing %d transaction IDs",
+ nsubxids),
+ errdetail("Snapshots taken during recovery record running top-level transaction IDs in the subtransaction array, which is limited to %d entries.",
+ GetMaxSnapshotSubxidCount())));
+ }
+ else
+ nsubxids = snapshot->subxcnt + nchildren;
+
/*
* Generate file path for the snapshot. We start numbering of snapshots
* inside the transaction from 1.
@@ -1211,8 +1265,8 @@ ExportSnapshot(Snapshot snapshot)
*
* However, it could be that our topXid is after the xmax, in which case
* we shouldn't include it because xip[] members are expected to be before
- * xmax. (We need not make the same check for subxip[] members, see
- * snapshot.h.)
+ * xmax. SnapshotData does not require the same of subxip[] members (see
+ * snapshot.h), but the filtering above enforces it for the file format.
*/
addTopXid = (TransactionIdIsValid(topXid) &&
TransactionIdPrecedes(topXid, snapshot->xmax)) ? 1 : 0;
@@ -1222,23 +1276,57 @@ 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())
+ nsubxids > GetMaxSnapshotSubxidCount())
+ {
appendStringInfoString(&buf, "sof:1\n");
+ write_subxids = snapshot->takenDuringRecovery;
+ }
else
{
appendStringInfoString(&buf, "sof:0\n");
- appendStringInfo(&buf, "sxcnt:%d\n", snapshot->subxcnt + nchildren);
+ write_subxids = true;
+ }
+
+ if (write_subxids)
+ {
+ appendStringInfo(&buf, "sxcnt:%d\n", nsubxids);
for (int32 i = 0; i < snapshot->subxcnt; i++)
- appendStringInfo(&buf, "sxp:%u\n", snapshot->subxip[i]);
+ {
+ if (!snapshot->takenDuringRecovery ||
+ (TransactionIdFollowsOrEquals(snapshot->subxip[i],
+ snapshot->xmin) &&
+ TransactionIdPrecedes(snapshot->subxip[i], snapshot->xmax)))
+ appendStringInfo(&buf, "sxp:%u\n", snapshot->subxip[i]);
+ }
for (int32 i = 0; i < nchildren; i++)
- appendStringInfo(&buf, "sxp:%u\n", children[i]);
+ {
+ if (!snapshot->takenDuringRecovery ||
+ (TransactionIdFollowsOrEquals(children[i], snapshot->xmin) &&
+ TransactionIdPrecedes(children[i], snapshot->xmax)))
+ 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 +1580,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 its
+ * in-progress 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 +1608,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 ad0d85f4189..8f24a168614 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 00000000000..704ac1141bc
--- /dev/null
+++ b/src/test/recovery/t/055_standby_snapshot_export.pl
@@ -0,0 +1,227 @@
+# Copyright (c) 2026, PostgreSQL Global Development Group
+#
+# Test snapshot export and import on a standby.
+#
+# A snapshot taken during recovery stores its in-progress set, including
+# every running top-level XID, in subxip, leaving xip empty. Once it 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 deletes another row in an early subtransaction, then overflows the subxid
+# cache and stays open. Recovery removes the deleting subtransaction's XID
+# from KnownAssignedXids, so a later visibility check of the tuple's xmax
+# must use pg_subtrans to map that child XID back to O. Keeping O open also
+# keeps lastOverflowedXid armed on the standby.
+my $o = $primary->background_psql('postgres');
+$o->query_safe('BEGIN');
+$o->query_safe('SAVEPOINT early');
+$o->query_safe('DELETE FROM victim WHERE k = 8');
+$o->query_safe('RELEASE early');
+$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))]);
+$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');
+
+my $file2 = slurp_file($standby->data_dir . "/pg_snapshots/$snap2");
+like($file2, qr/^sof:1$/m,
+ 'snapshot saved for post-promotion re-export is suboverflowed');
+
+$o->query_safe('COMMIT');
+$primary->wait_for_replay_catchup($standby);
+
+is( $standby->safe_psql(
+ 'postgres', 'SELECT count(*) FROM victim 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 $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");
+my ($xmin3) = $file3 =~ /^xmin:(\d+)$/m;
+my ($xmax3) = $file3 =~ /^xmax:(\d+)$/m;
+my @subxids3 = $file3 =~ /^sxp:(\d+)$/mg;
+like($file3, qr/^sof:1$/m,
+ 're-exported recovery snapshot remains suboverflowed');
+like($file3, qr/^sxcnt:[1-9]/m,
+ 'a recovery-taken snapshot re-exports its subxip array after a write');
+is(scalar(grep { $_ < $xmin3 || $_ >= $xmax3 } @subxids3),
+ 0, 're-exported snapshot omits XIDs outside its range');
+
+my $s5 = $standby->background_psql('postgres');
+$s5->query_safe(
+ qq[BEGIN ISOLATION LEVEL REPEATABLE READ;
+ SET TRANSACTION SNAPSHOT '$snap3']);
+is($s5->query_safe('SELECT count(*) FROM victim'),
+ $before, 're-exported recovery snapshot can be imported');
+
+$s5->query_safe('COMMIT');
+$s4->query_safe('COMMIT');
+$s3->query_safe('COMMIT');
+
+$guard->quit;
+$u->quit;
+$o->quit;
+$s1->quit;
+$s2->quit;
+$s3->quit;
+$s4->quit;
+$s5->quit;
+$standby->stop;
+$primary->stop;
+
+done_testing();
--
2.34.1
--TAL7Re22tEA+B5OU
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v2-0002-Read-the-in-progress-set-from-subxip-in-pg_curren.patch"
view thread (81+ messages) latest in thread
Message-ID: <no-message-id-1505894@localhost>
Permalink: ../no-message-id-1505894@localhost/
Also on: postgresql.org/message-id/no-message-id-1505894@localhost
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pg@bowt.ie
Subject: Re: [PATCH v2 1/2] Don't discard subxip when exporting a snapshot taken during recovery
In-Reply-To: <no-message-id-1505894@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox