[PATCH v1 0/2] Fix exported snapshot xmin handoff race

Started by chee.wooson5 days ago9 messageshackers
Jump to latest
#1chee.wooson
chee.wooson@gmail.com

Hi,

ProcArrayInstallImportedXmin() holds ProcArrayLock in shared mode while it
checks that the exporting transaction is still running and installs the
imported xmin. That normally prevents transaction end from removing the
source from the proc array. However, a read-only exporting transaction has
no assigned XID, so ProcArrayEndTransaction() clears its xmin without
ProcArrayLock.

This permits a horizon scan holding ProcArrayLock shared to pass the importer
before the imported xmin is installed. The importer can then validate the
source under another shared lock, while the source concurrently clears its
xmin lock-free. When the horizon scan reaches the source, it can therefore
miss both xmins and allow VACUUM to remove a tuple that is still visible to
the imported snapshot.

Tom Lane described the same general hazard in 2016:

/messages/by-id/6078.1478985619@sss.pgh.pa.us

That discussion suspected it might not be a live bug because an exported
snapshot keeps the source xmin until transaction end. The missing case is
that an XID-less source can clear xmin at transaction end even while another
backend holds ProcArrayLock shared.

This series adds a dedicated atomic three-state field to PGPROC. Snapshot
export publishes EXPORTED before the snapshot file is made visible. An
importer changes EXPORTED to REFERENCED before installing the xmin. At
transaction end, an unreferenced export retains the existing lock-free
cleanup path, while a referenced export acquires ProcArrayLock exclusively
before clearing xmin. If transaction end changes EXPORTED to ENDING first,
the importer fails safely.

Patch 1 adds deterministic injection points and a TAP test. With patch 1
alone, the test reproduces the bug by failing both the expected lock wait and
the final visibility check. Patch 2 implements the atomic handoff; all five
subtests then pass. The test also covers the source-wins case, repeated
exports in one transaction, and the lock-free path for an export that was
never imported.

The series is based on PostgreSQL master at
c12c101b0846b1e6488f2dc986a852fbc6bf2e3b. I also reproduced the issue on
REL_17_STABLE.

Validation on current master:

- ninja -C build
- meson test -C build test_misc/015_export_snapshot
- meson test -C build --suite regress --suite isolation
--suite injection_points --suite test_misc
- pgindent --check on all modified C and header files

All tests pass.

Performance impact is limited to one four-byte atomic field per PGPROC, an
atomic read when an XID-less transaction ends, and state transitions during
snapshot export/import. ProcArrayLock exclusive is added only when an
imported snapshot references the ending XID-less source. I have not run a
dedicated performance benchmark.

chee.wooson (2):
Add test for exported snapshot xmin race
Fix exported snapshot xmin handoff race

src/backend/access/transam/twophase.c | 1 +
src/backend/storage/ipc/procarray.c | 99 ++++-
src/backend/storage/lmgr/proc.c | 4 +
src/backend/utils/time/snapmgr.c | 13 +
src/include/storage/proc.h | 21 +
src/test/modules/test_misc/meson.build | 1 +
.../test_misc/t/015_export_snapshot.pl | 394 ++++++++++++++++++
7 files changed, 525 insertions(+), 8 deletions(-)
create mode 100644 src/test/modules/test_misc/t/015_export_snapshot.pl

--
2.43.0

#2chee.wooson
chee.wooson@gmail.com
In reply to: chee.wooson (#1)
[PATCH v1 1/2] Add test for exported snapshot xmin race

Add injection points that pause horizon calculation and snapshot import at
deterministic locations. Add a TAP test that reproduces the
exported-snapshot xmin handoff race.

The test also covers both sides of the atomic handoff, repeated exports in
one transaction, and the lock-free cleanup path for an exported snapshot
that was never imported.
---
src/backend/storage/ipc/procarray.c | 13 +
src/test/modules/test_misc/meson.build | 1 +
.../test_misc/t/015_export_snapshot.pl | 394 ++++++++++++++++++
3 files changed, 408 insertions(+)
create mode 100644 src/test/modules/test_misc/t/015_export_snapshot.pl

diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 60336b31803..66b394e2b4e 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -1736,6 +1736,15 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 		TransactionId xid;
 		TransactionId xmin;
+#ifdef USE_INJECTION_POINTS
+		{
+			char		ip_name[64];
+
+			snprintf(ip_name, sizeof(ip_name),
+					 "compute-xid-horizons-at-%d", index);
+			InjectionPointRun(ip_name, NULL);
+		}
+#endif
 		/* Fetch xid just once - see GetNewTransactionId */
 		xid = UINT32_ACCESS_ONCE(other_xids[index]);
 		xmin = UINT32_ACCESS_ONCE(proc->xmin);
@@ -1945,6 +1954,8 @@ GetOldestNonRemovableTransactionId(Relation rel)
 {
 	ComputeXidHorizonsResult horizons;
+	INJECTION_POINT("get-oldest-nonremovable-txid", NULL);
+
 	ComputeXidHorizons(&horizons);

switch (GlobalVisHorizonKindForRel(rel))
@@ -2521,6 +2532,8 @@ ProcArrayInstallImportedXmin(TransactionId xmin,
if (proc->databaseId != MyDatabaseId)
continue;

+		INJECTION_POINT("install-imported-xmin-before-reference", NULL);
+
 		/*
 		 * Likewise, let's just make real sure its xmin does cover us.
 		 */
diff --git a/src/test/modules/test_misc/meson.build b/src/test/modules/test_misc/meson.build
index ee290698b31..eb48ee35d1d 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_export_snapshot.pl',
     ],
     # The injection points are cluster-wide, so disable installcheck
     'runningcheck': false,
diff --git a/src/test/modules/test_misc/t/015_export_snapshot.pl b/src/test/modules/test_misc/t/015_export_snapshot.pl
new file mode 100644
index 00000000000..1ddae2d16a2
--- /dev/null
+++ b/src/test/modules/test_misc/t/015_export_snapshot.pl
@@ -0,0 +1,394 @@
+# Copyright (c) 2024-2026, PostgreSQL Global Development Group
+#
+# Test: reproduce the exported-snapshot xmin handoff race.
+#
+# Strategy (two-injection-point approach):
+#   1. VACUUM attaches "get-oldest-nonremovable-txid" with injection_wait
+#      (PID-filtered via injection_points_set_local).
+#   2. Start VACUUM.  The first ComputeXidHorizons call (from on-access
+#      catalog pruning via GlobalVisUpdate) passes through unblocked.
+#   3. VACUUM reaches GetOldestNonRemovableTransactionId -> blocked at
+#      "get-oldest-nonremovable-txid".
+#   4. Coordinator detects the block -> GLOBALLY attaches
+#      "compute-xid-horizons-at-1" with injection_wait (no PID filter).
+#   5. Coordinator wakes VACUUM from "get-oldest-nonremovable-txid".
+#   6. VACUUM enters ComputeXidHorizons (holding ProcArrayLock shared)
+#      -> blocked at "compute-xid-horizons-at-1" (after scanning importer
+#        at index 0 with xmin=Invalid).
+#   7. Coordinator detects the second block -> handoff setup:
+#       a. A transaction whose snapshot was exported but never imported
+#          commits without waiting for ProcArrayLock.
+#       b. Importer runs SET TRANSACTION SNAPSHOT (ProcArrayInstallImportedXmin
+#          under ProcArrayLock shared, compatible with VACUUM's shared lock).
+#       c. Source starts COMMIT.  The fix makes its no-XID cleanup wait for
+#          ProcArrayLock exclusive instead of clearing xmin lock-free.
+#   8. Coordinator verifies that COMMIT is waiting, then detaches + wakes
+#      "compute-xid-horizons-at-1".
+#   9. VACUUM sees the source xmin before source COMMIT can clear it, so its
+#      horizon remains low enough to preserve the deleted tuple.
+#  10. Importer queries -> 1 row.
+#
+# Without the fix, source COMMIT does not wait and VACUUM misses both xmin
+# values, so the lock-wait assertion and final visibility check both fail.
+#
+# Depends only on: injection_points (built-in test module)
+
+use strict;
+use warnings FATAL => 'all';
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+use Time::HiRes qw(usleep);
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+	plan skip_all => 'Injection points not supported by this build';
+}
+
+# ---- Node setup ----
+my $node = PostgreSQL::Test::Cluster->new('export_race_node');
+$node->init;
+$node->append_conf('postgresql.conf',
+	"shared_preload_libraries = 'injection_points'");
+$node->start;
+
+$node->safe_psql('postgres', 'CREATE EXTENSION injection_points');
+
+# ---- Schema ----
+$node->safe_psql('postgres', qq{
+	CREATE TABLE race_test (id int, data text);
+	INSERT INTO race_test VALUES (1, 'should_be_visible');
+});
+
+# ---- Proc-array scan order ----
+# ComputeXidHorizons iterates over pgprocnos[], which contains only
+# client backends that called ProcArrayAdd().  System processes
+# (checkpointer, bgwriter, walwriter, etc.) are NOT in pgprocnos[].
+#
+# Ending a transaction does not remove its backend from pgprocnos[]; only
+# disconnecting does.  The test only relies on importer preceding source, with
+# at least one entry between them.
+#
+# Injection point at index=1 pauses VACUUM after scanning importer(0).
+
+# ---- Connect sessions in scan order ----
+my $imp   = $node->background_psql('postgres');  # index 0
+my $fill  = $node->background_psql('postgres');  # index 1 (gap)
+my $src   = $node->background_psql('postgres');  # index 2 (source)
+my $vac   = $node->background_psql('postgres');  # index 3 (VACUUM)
+# index 4 (deleter, removed after commit)
+my $del = $node->background_psql('postgres');
+# index 5, then 4 after the deleter is removed
+my $coord = $node->background_psql('postgres');
+
+# ---- Phase 1: Prepare ----
+# Source: begin, read table, export snapshot
+$src->query("BEGIN ISOLATION LEVEL REPEATABLE READ");
+$src->query("SELECT * FROM race_test");
+my $token = $src->query("SELECT pg_export_snapshot()");
+$token =~ s/\s+//g;
+diag("exported snapshot token: $token");
+
+# Deleter: remove the row and commit.
+$del->query("DELETE FROM race_test WHERE id = 1");
+$del->query("COMMIT");
+diag("deleter committed");
+
+# Advance the XID counter so that the horizon (latestCompletedXid+1 when
+# no backend has a valid xmin) is strictly greater than the deleter's XID.
+# Each safe_psql opens a new connection, consumes one XID, and disconnects.
+for (my $i = 0; $i < 100; $i++)
+{
+	$node->safe_psql('postgres', "SELECT txid_current()");
+}
+diag("100 filler XIDs consumed");
+
+# Importer: begin (ready to import the snapshot later).
+# No query after BEGIN -> xmin stays Invalid, essential for the race.
+$imp->query("BEGIN ISOLATION LEVEL REPEATABLE READ");
+
+# ---- Phase 2: Two-injection-point race reproduction ----
+
+# Step 1: In VACUUM session, set local mode and attach first injection point.
+my $vac_pid = $vac->query("SELECT pg_backend_pid()");
+$vac_pid =~ s/\s+//g;
+diag("VACUUM PID: $vac_pid");
+
+$vac->query("SELECT injection_points_set_local()");
+$vac->query(
+	"SELECT injection_points_attach('get-oldest-nonremovable-txid', 'wait')");
+diag("VACUUM attached get-oldest-nonremovable-txid (PID-filtered)");
+
+# Step 2: Start VACUUM asynchronously.
+$vac->query_until(qr/vac_started/,
+	"\\echo vac_started\nVACUUM race_test;\n");
+diag("VACUUM started, waiting for first injection point...");
+
+# Step 3: Wait for VACUUM to be blocked at get-oldest-nonremovable-txid.
+{
+	my $blocked = 0;
+	for (my $i = 0; $i < 1800; $i++)
+	{
+		my $result = $coord->query(
+			"SELECT count(*) = 1 FROM pg_stat_activity"
+			  . " WHERE pid = $vac_pid"
+			  . " AND wait_event_type = 'InjectionPoint'");
+		if ($result =~ /t/)
+		{
+			$blocked = 1;
+			last;
+		}
+		usleep(100_000);
+	}
+	die "VACUUM did not reach first injection point within 180s"
+		unless $blocked;
+}
+diag("VACUUM blocked at get-oldest-nonremovable-txid");
+
+# Step 4: Coordinator GLOBALLY attaches compute-xid-horizons-at-1.
+# No set_local -> this blocks ALL backends that hit it.  Only VACUUM
+# calls ComputeXidHorizons after this point (coordinator queries use
+# pg_stat_activity which doesn't call ComputeXidHorizons).
+$coord->query(
+	"SELECT injection_points_attach('compute-xid-horizons-at-1', 'wait')");
+diag("coordinator attached compute-xid-horizons-at-1 (GLOBAL)");
+
+# Step 5: Wake VACUUM from the first block.
+$coord->query(
+	"SELECT injection_points_wakeup('get-oldest-nonremovable-txid')");
+diag("woke VACUUM from get-oldest-nonremovable-txid");
+
+# Step 6: Wait for VACUUM to block at compute-xid-horizons-at-1.
+# VACUUM entered GetOldestNonRemovableTransactionId -> ComputeXidHorizons
+# -> acquired ProcArrayLock shared -> scanned index 0 (importer) ->
+# blocked at index 1 (filler).
+{
+	my $blocked = 0;
+	for (my $i = 0; $i < 1800; $i++)
+	{
+		my $result = $coord->query(
+			"SELECT count(*) = 1 FROM pg_stat_activity"
+			  . " WHERE pid = $vac_pid"
+			  . " AND wait_event_type = 'InjectionPoint'");
+		if ($result =~ /t/)
+		{
+			$blocked = 1;
+			last;
+		}
+		usleep(100_000);
+	}
+	die "VACUUM did not reach second injection point within 180s"
+		unless $blocked;
+}
+diag("VACUUM blocked at compute-xid-horizons-at-1 "
+	  . "(inside ComputeXidHorizons)");
+
+# Step 7a: An export without any importer must retain the normal lock-free
+# no-XID cleanup path.
+$fill->query("BEGIN ISOLATION LEVEL REPEATABLE READ");
+my $unused_token = $fill->query("SELECT pg_export_snapshot()");
+$unused_token =~ s/\s+//g;
+
+my $fill_pid = $fill->query("SELECT pg_backend_pid()");
+$fill_pid =~ s/\s+//g;
+$fill->query_until(qr/unreferenced_commit_started/,
+	"\\echo unreferenced_commit_started\nCOMMIT;\n"
+	  . "\\echo unreferenced_commit_done\n");
+
+my $unreferenced_finished = 0;
+for (my $i = 0; $i < 100; $i++)
+{
+	my $result = $coord->query(
+		"SELECT CASE"
+		  . " WHEN state = 'idle' THEN 'done'"
+		  . " WHEN wait_event_type = 'LWLock'"
+		  . " AND wait_event = 'ProcArray' THEN 'blocked'"
+		  . " ELSE 'running' END"
+		  . " FROM pg_stat_activity WHERE pid = $fill_pid");
+	if ($result =~ /done/)
+	{
+		$unreferenced_finished = 1;
+		last;
+	}
+	last if $result =~ /blocked/;
+	usleep(100_000);
+}
+diag("unreferenced export started COMMIT while VACUUM holds ProcArrayLock");
+
+# Step 7b: Set up the actual handoff while VACUUM holds ProcArrayLock shared.
+#   - importer imports the snapshot -> ProcArrayInstallImportedXmin
+#     acquires ProcArrayLock shared (compatible).
+#   - source starts committing -> its no-XID cleanup must wait for
+#     ProcArrayLock exclusive.
+$imp->query("SET TRANSACTION SNAPSHOT '$token'");
+diag("importer installed snapshot (xmin now valid, low value)");
+
+# A later export in the same source transaction must preserve REFERENCED.
+my $second_token = $src->query("SELECT pg_export_snapshot()");
+$second_token =~ s/\s+//g;
+
+my $src_pid = $src->query("SELECT pg_backend_pid()");
+$src_pid =~ s/\s+//g;
+$src->query_until(qr/source_commit_started/,
+	"\\echo source_commit_started\nCOMMIT;\n\\echo source_commit_done\n");
+diag("source started COMMIT");
+
+my $source_waiting = 0;
+for (my $i = 0; $i < 100; $i++)
+{
+	my $result = $coord->query(
+		"SELECT wait_event_type = 'LWLock' AND wait_event = 'ProcArray'"
+		  . " FROM pg_stat_activity"
+		  . " WHERE pid = $src_pid");
+	if ($result =~ /t/)
+	{
+		$source_waiting = 1;
+		last;
+	}
+	last if $result eq '';
+	usleep(100_000);
+}
+
+# Step 8: Detach BOTH injection points, then wake VACUUM.
+#   compute-xid-horizons-at-1 is the one VACUUM is currently blocked on.
+#   get-oldest-nonremovable-txid must also be detached, because VACUUM
+#   may call GetOldestNonRemovableTransactionId again (e.g., during
+#   index vacuum or other internal processing), and nobody would wake it.
+$coord->query(
+	"SELECT injection_points_detach('compute-xid-horizons-at-1')");
+$coord->query(
+	"SELECT injection_points_detach('get-oldest-nonremovable-txid')");
+$coord->query(
+	"SELECT injection_points_wakeup('compute-xid-horizons-at-1')");
+diag("detached both injection points + woke VACUUM");
+
+$src->query_until(qr/source_commit_done/, "");
+$fill->query_until(qr/unreferenced_commit_done/, "");
+ok($unreferenced_finished,
+	"unreferenced export clears xmin without waiting for ProcArrayLock");
+ok($source_waiting,
+	"exporting transaction waits for ProcArrayLock while clearing xmin");
+diag("source committed after VACUUM released ProcArrayLock");
+
+# Step 9: Wait for VACUUM to finish.
+{
+	my $done = 0;
+	for (my $i = 0; $i < 1800; $i++)
+	{
+		my $result = $coord->query(
+			"SELECT count(*) = 1 FROM pg_stat_activity"
+			  . " WHERE pid = $vac_pid"
+			  . " AND state = 'idle'");
+		if ($result =~ /t/)
+		{
+			$done = 1;
+			last;
+		}
+		usleep(100_000);
+	}
+	die "VACUUM did not finish within 180s" unless $done;
+}
+diag("VACUUM finished");
+
+# ---- Phase 3: Check ----
+# Importer queries using the imported snapshot.  The snapshot should see
+# the row if VACUUM correctly computed the horizon.  With the race bug,
+# VACUUM computed a horizon that is too high (missed importer's low xmin),
+# so the tuple was removed -> importer sees 0 rows.
+my $count = $imp->query("SELECT count(*) FROM race_test");
+$count =~ s/\s+//g;
+diag("importer sees $count row(s)");
+
+is($count, 1,
+	"imported snapshot still sees the row after concurrent VACUUM")
+  or diag("BUG DETECTED: export-snapshot xmin race caused "
+			. "premature tuple removal (expected 1 row, got $count)");
+
+# ---- Cleanup ----
+$imp->query("COMMIT");
+
+# ---- Phase 4: Source wins the atomic transition ----
+# Pause an importer after it finds the source but before it changes EXPORTED
+# to REFERENCED.  The source must be able to change EXPORTED to ENDING and
+# commit without ProcArrayLock; the importer must then fail.
+my $imp_fail =
+  $node->background_psql('postgres', on_error_stop => 0);
+
+$src->query("BEGIN ISOLATION LEVEL REPEATABLE READ");
+my $token2 = $src->query("SELECT pg_export_snapshot()");
+$token2 =~ s/\s+//g;
+
+$imp_fail->query("SELECT injection_points_set_local()");
+$imp_fail->query(
+	"SELECT injection_points_attach("
+	  . "'install-imported-xmin-before-reference', 'wait')");
+my $imp_fail_pid = $imp_fail->query("SELECT pg_backend_pid()");
+$imp_fail_pid =~ s/\s+//g;
+$imp_fail->query("BEGIN ISOLATION LEVEL REPEATABLE READ");
+
+$imp_fail->query_until(qr/failing_import_started/,
+	"\\echo failing_import_started\nSET TRANSACTION SNAPSHOT '$token2';\n"
+	  . "\\echo failing_import_done\n");
+
+{
+	my $blocked = 0;
+	for (my $i = 0; $i < 100; $i++)
+	{
+		my $result = $coord->query(
+			"SELECT wait_event_type = 'InjectionPoint'"
+			  . " FROM pg_stat_activity WHERE pid = $imp_fail_pid");
+		if ($result =~ /t/)
+		{
+			$blocked = 1;
+			last;
+		}
+		usleep(100_000);
+	}
+	die "importer did not reach pre-reference injection point"
+	  unless $blocked;
+}
+
+$src->query_until(qr/source_won_commit_started/,
+	"\\echo source_won_commit_started\nCOMMIT;\n"
+	  . "\\echo source_won_commit_done\n");
+
+my $source_won = 0;
+for (my $i = 0; $i < 100; $i++)
+{
+	my $result = $coord->query(
+		"SELECT CASE"
+		  . " WHEN state = 'idle' THEN 'done'"
+		  . " WHEN wait_event_type = 'LWLock'"
+		  . " AND wait_event = 'ProcArray' THEN 'blocked'"
+		  . " ELSE 'running' END"
+		  . " FROM pg_stat_activity WHERE pid = $src_pid");
+	if ($result =~ /done/)
+	{
+		$source_won = 1;
+		last;
+	}
+	last if $result =~ /blocked/;
+	usleep(100_000);
+}
+
+$coord->query(
+	"SELECT injection_points_detach("
+	  . "'install-imported-xmin-before-reference')");
+$coord->query(
+	"SELECT injection_points_wakeup("
+	  . "'install-imported-xmin-before-reference')");
+
+$src->query_until(qr/source_won_commit_done/, "");
+$imp_fail->query_until(qr/failing_import_done/, "");
+
+ok($source_won,
+	"unreferenced source changes EXPORTED to ENDING without waiting");
+like($imp_fail->{stderr},
+	qr/could not import the requested snapshot/,
+	"import fails after source wins the atomic transition");
+$imp_fail->{stderr} = '';
+$imp_fail->query("ROLLBACK");
+
+$node->stop;
+done_testing();
-- 
2.43.0
#3chee.wooson
chee.wooson@gmail.com
In reply to: chee.wooson (#1)
[PATCH v1 2/2] Fix exported snapshot xmin handoff race

A transaction that has not assigned an XID clears its advertised xmin at
transaction end without ProcArrayLock. ProcArrayInstallImportedXmin() can
therefore verify the source under a shared lock, then lose the source xmin
before it installs the importer xmin. A concurrent horizon scan can miss
both.

Track each PGPROC exported xmin with an atomic three-state protocol. An
importer marks the xmin referenced before installing it. If the importer
wins the handoff, the source clears its xmin under ProcArrayLock exclusive;
if transaction end wins, the import fails. Snapshots that were never
imported retain the existing lock-free cleanup path.
---
src/backend/access/transam/twophase.c | 1 +
src/backend/storage/ipc/procarray.c | 86 ++++++++++++++++++++++++---
src/backend/storage/lmgr/proc.c | 4 ++
src/backend/utils/time/snapmgr.c | 13 ++++
src/include/storage/proc.h | 21 +++++++
5 files changed, 117 insertions(+), 8 deletions(-)

diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index fa3bc50ec48..a64b1a41d0c 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -479,6 +479,7 @@ MarkAsPreparingGuts(GlobalTransaction gxact, FullTransactionId fxid,
 	proc->waitLock = NULL;
 	dlist_node_init(&proc->waitLink);
 	proc->waitProcLock = NULL;
+	pg_atomic_init_u32(&proc->xminExportState, PROC_XMIN_EXPORT_ENDING);
 	pg_atomic_init_u64(&proc->waitStart, 0);
 	for (i = 0; i < NUM_LOCK_PARTITIONS; i++)
 		dlist_init(&proc->myProcLocks[i]);
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 66b394e2b4e..a2556314325 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -687,15 +687,39 @@ ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid)
 	}
 	else
 	{
+		uint32		xmin_export_state;
+		bool		clear_xmin_exclusively = false;
+
 		/*
-		 * If we have no XID, we don't need to lock, since we won't affect
-		 * anyone else's calculation of a snapshot.  We might change their
-		 * estimate of global xmin, but that's OK.
+		 * If we have no XID, we ordinarily don't need to lock, since we won't
+		 * affect anyone else's calculation of a snapshot.  Atomically close
+		 * an exported xmin to new importers.  If an importer acquired a
+		 * reference first, clear under an exclusive lock to serialize with
+		 * both that importer and horizon calculations.
 		 */
 		Assert(!TransactionIdIsValid(proc->xid));
 		Assert(proc->subxidStatus.count == 0);
 		Assert(!proc->subxidStatus.overflowed);
+		xmin_export_state = pg_atomic_read_u32(&proc->xminExportState);
+		Assert(xmin_export_state == PROC_XMIN_EXPORT_ENDING ||
+			   xmin_export_state == PROC_XMIN_EXPORT_EXPORTED ||
+			   xmin_export_state == PROC_XMIN_EXPORT_REFERENCED);
+
+		if (xmin_export_state != PROC_XMIN_EXPORT_ENDING)
+		{
+			xmin_export_state =
+				pg_atomic_exchange_u32(&proc->xminExportState,
+									   PROC_XMIN_EXPORT_ENDING);
+			Assert(xmin_export_state == PROC_XMIN_EXPORT_EXPORTED ||
+				   xmin_export_state == PROC_XMIN_EXPORT_REFERENCED);
+			clear_xmin_exclusively =
+				(xmin_export_state == PROC_XMIN_EXPORT_REFERENCED);
+		}
+
+		if (clear_xmin_exclusively)
+			LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+
 		proc->vxid.lxid = InvalidLocalTransactionId;
 		proc->xmin = InvalidTransactionId;
@@ -706,13 +730,20 @@ ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid)
 		/* avoid unnecessarily dirtying shared cachelines */
 		if (proc->statusFlags & PROC_VACUUM_STATE_MASK)
 		{
-			Assert(!LWLockHeldByMe(ProcArrayLock));
-			LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+			if (!clear_xmin_exclusively)
+			{
+				Assert(!LWLockHeldByMe(ProcArrayLock));
+				LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+			}
 			Assert(proc->statusFlags == ProcGlobal->statusFlags[proc->pgxactoff]);
 			proc->statusFlags &= ~PROC_VACUUM_STATE_MASK;
 			ProcGlobal->statusFlags[proc->pgxactoff] = proc->statusFlags;
-			LWLockRelease(ProcArrayLock);
+			if (!clear_xmin_exclusively)
+				LWLockRelease(ProcArrayLock);
 		}
+
+		if (clear_xmin_exclusively)
+			LWLockRelease(ProcArrayLock);
 	}
 }
@@ -738,6 +769,7 @@ ProcArrayEndTransactionInternal(PGPROC *proc, TransactionId latestXid)
 	proc->xid = InvalidTransactionId;
 	proc->vxid.lxid = InvalidLocalTransactionId;
 	proc->xmin = InvalidTransactionId;
+	pg_atomic_write_u32(&proc->xminExportState, PROC_XMIN_EXPORT_ENDING);

/* be sure this is cleared in abort */
proc->delayChkptFlags = 0;
@@ -2498,7 +2530,10 @@ ProcArrayInstallImportedXmin(TransactionId xmin,
if (!sourcevxid)
return false;

-	/* Get lock so source xact can't end while we're doing this */
+	/*
+	 * Stabilize the proc array while locating the source.  A source without
+	 * an XID can still begin ending until we acquire an xmin reference below.
+	 */
 	LWLockAcquire(ProcArrayLock, LW_SHARED);

/*
@@ -2512,6 +2547,7 @@ ProcArrayInstallImportedXmin(TransactionId xmin,
PGPROC *proc = &allProcs[pgprocno];
int statusFlags = ProcGlobal->statusFlags[index];
TransactionId xid;
+ uint32 xmin_export_state;

/* Ignore procs running LAZY VACUUM */
if (statusFlags & PROC_IN_VACUUM)
@@ -2535,8 +2571,42 @@ ProcArrayInstallImportedXmin(TransactionId xmin,
INJECTION_POINT("install-imported-xmin-before-reference", NULL);

 		/*
-		 * Likewise, let's just make real sure its xmin does cover us.
+		 * Acquire a reference to the exported xmin.  This atomic transition
+		 * arbitrates with lock-free transaction end: either we change
+		 * EXPORTED to REFERENCED first, forcing the source to clear under
+		 * ProcArrayLock exclusive, or the source changes it to ENDING first
+		 * and this import fails.
 		 */
+		xmin_export_state = pg_atomic_read_u32(&proc->xminExportState);
+		while (xmin_export_state == PROC_XMIN_EXPORT_EXPORTED)
+		{
+			if (pg_atomic_compare_exchange_u32(&proc->xminExportState,
+											   &xmin_export_state,
+											   PROC_XMIN_EXPORT_REFERENCED))
+			{
+				xmin_export_state = PROC_XMIN_EXPORT_REFERENCED;
+				break;
+			}
+		}
+
+		if (xmin_export_state != PROC_XMIN_EXPORT_REFERENCED)
+		{
+			Assert(xmin_export_state == PROC_XMIN_EXPORT_ENDING);
+			continue;
+		}
+
+		/*
+		 * The source could have ended and reused its PGPROC between the
+		 * initial VXID match and the state transition above.  Recheck its
+		 * identity after acquiring the reference.  We intentionally leave a
+		 * conservatively acquired reference in place on failure.
+		 */
+		if (proc->vxid.procNumber != sourcevxid->procNumber ||
+			proc->vxid.lxid != sourcevxid->localTransactionId ||
+			proc->databaseId != MyDatabaseId)
+			continue;
+
+		/* Lastly, make real sure its xmin does cover us. */
 		xid = UINT32_ACCESS_ONCE(proc->xmin);
 		if (!TransactionIdIsNormal(xid) ||
 			!TransactionIdPrecedesOrEquals(xid, xmin))
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 9d6e69175a5..0af8774d585 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -372,6 +372,8 @@ ProcGlobalShmemInit(void *arg)
 		 */
 		pg_atomic_init_u32(&(proc->procArrayGroupNext), INVALID_PROC_NUMBER);
 		pg_atomic_init_u32(&(proc->clogGroupNext), INVALID_PROC_NUMBER);
+		pg_atomic_init_u32(&(proc->xminExportState),
+						   PROC_XMIN_EXPORT_ENDING);
 		pg_atomic_init_u64(&(proc->waitStart), 0);
 	}
@@ -477,6 +479,7 @@ InitProcess(void)
 	MyProc->fpLocalTransactionId = InvalidLocalTransactionId;
 	MyProc->xid = InvalidTransactionId;
 	MyProc->xmin = InvalidTransactionId;
+	pg_atomic_write_u32(&MyProc->xminExportState, PROC_XMIN_EXPORT_ENDING);
 	MyProc->pid = MyProcPid;
 	MyProc->vxid.procNumber = MyProcNumber;
 	MyProc->vxid.lxid = InvalidLocalTransactionId;
@@ -682,6 +685,7 @@ InitAuxiliaryProcess(void)
 	MyProc->fpLocalTransactionId = InvalidLocalTransactionId;
 	MyProc->xid = InvalidTransactionId;
 	MyProc->xmin = InvalidTransactionId;
+	pg_atomic_write_u32(&MyProc->xminExportState, PROC_XMIN_EXPORT_ENDING);
 	MyProc->vxid.procNumber = INVALID_PROC_NUMBER;
 	MyProc->vxid.lxid = InvalidLocalTransactionId;
 	MyProc->databaseId = InvalidOid;
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index bc98a4361bf..3760dff7558 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -1122,6 +1122,7 @@ ExportSnapshot(Snapshot snapshot)
 	StringInfoData buf;
 	FILE	   *f;
 	MemoryContext oldcxt;
+	uint32		xmin_export_state;
 	char		path[MAXPGPATH];
 	char		pathtmp[MAXPGPATH];

@@ -1264,6 +1265,18 @@ ExportSnapshot(Snapshot snapshot)
(errcode_for_file_access(),
errmsg("could not write to file \"%s\": %m", pathtmp)));

+	/*
+	 * Make our xmin available for import before publishing the file. Preserve
+	 * REFERENCED if an earlier snapshot from this transaction was already
+	 * imported.
+	 */
+	xmin_export_state = PROC_XMIN_EXPORT_ENDING;
+	if (!pg_atomic_compare_exchange_u32(&MyProc->xminExportState,
+										&xmin_export_state,
+										PROC_XMIN_EXPORT_EXPORTED))
+		Assert(xmin_export_state == PROC_XMIN_EXPORT_EXPORTED ||
+			   xmin_export_state == PROC_XMIN_EXPORT_REFERENCED);
+
 	/*
 	 * Now that we have written everything into a .tmp file, rename the file
 	 * to remove the .tmp suffix.
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 03a1a466fa8..ee8ee3862a4 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -81,6 +81,25 @@ struct XidCache
  */
 #define		PROC_XMIN_FLAGS (PROC_IN_VACUUM | PROC_IN_SAFE_IC)
+/*
+ * States for PGPROC.xminExportState:
+ *
+ * ENDING --ExportSnapshot()--> EXPORTED
+ * EXPORTED --snapshot import--> REFERENCED
+ * EXPORTED/REFERENCED --transaction end--> ENDING
+ *
+ * ENDING is also the quiescent state before the current transaction exports
+ * a snapshot.  Transaction end may clear xmin lock-free when it changes
+ * EXPORTED to ENDING, but must use ProcArrayLock exclusive when it changes
+ * REFERENCED to ENDING.
+ */
+typedef enum ProcXminExportState
+{
+	PROC_XMIN_EXPORT_ENDING = 0,
+	PROC_XMIN_EXPORT_EXPORTED,
+	PROC_XMIN_EXPORT_REFERENCED
+}			ProcXminExportState;
+
 /*
  * We allow a limited number of "weak" relation locks (AccessShareLock,
  * RowShareLock, RowExclusiveLock) to be recorded in the PGPROC structure
@@ -250,6 +269,8 @@ typedef struct PGPROC
 								 * vacuum must not remove tuples deleted by
 								 * xid >= xmin ! */
+	pg_atomic_uint32 xminExportState;	/* one of ProcXminExportState */
+
 	XidCacheStatus subxidStatus;	/* mirrored with
 									 * ProcGlobal->subxidStates[i] */
 	struct XidCache subxids;	/* cache for subtransaction XIDs */
-- 
2.43.0
#4chee.wooson
chee.wooson@gmail.com
In reply to: chee.wooson (#1)
Re: [PATCH v2] Fix exported snapshot xmin handoff race

Hi,

Attached is v2.

Sorry that v1 was a bit noisy. This was my first PostgreSQL patch
submission, and I sent the patch series inline without realizing how much
it would expand in the archives. This version is attached as a single
patch.

Changes since v1:

- simplified the fix to take ProcArrayLock exclusively in
ProcArrayInstallImportedXmin();
- dropped the PGPROC xmin-export state machine;
- squashed the reproducer and fix into one patch;
- adjusted the TAP test to match the exclusive-lock approach.

The race is that SET TRANSACTION SNAPSHOT can install the imported xmin
while VACUUM is already in the middle of ComputeXidHorizons(), because
both paths currently hold ProcArrayLock in shared mode. VACUUM can then
miss both the importer's newly installed xmin and the source transaction's
xmin, and compute a horizon that is too new.

The attached patch changes ProcArrayInstallImportedXmin() to acquire
ProcArrayLock exclusively. This serializes exported-snapshot import with
horizon computation. The patch also adds an injection-point TAP test that
pauses VACUUM inside ComputeXidHorizons(), starts SET TRANSACTION SNAPSHOT
concurrently, and verifies that the importer waits for ProcArrayLock before
the imported snapshot is allowed to protect the deleted tuple.

Validation:

- meson test -C build --suite setup --print-errorlogs
- meson test -C build test_misc/015_export_snapshot --no-rebuild
--print-errorlogs

The new test passes with the fix, with 3 subtests passed. I also verified
that the patch applies cleanly to current master.

Regards,
Chee

Attachments:

v2-0001-Fix-exported-snapshot-xmin-handoff-race.patchtext/x-patch; charset=UTF-8; name=v2-0001-Fix-exported-snapshot-xmin-handoff-race.patchDownload+274-2
In reply to: chee.wooson (#4)
Re: [PATCH v2] Fix exported snapshot xmin handoff race

On Thu, Jul 30, 2026 at 12:25 AM chee.wooson <chee.wooson@gmail.com> wrote:

Attached is v2.

Are you aware that there are 2 other known bugs with pending fixes
that involve snapshot import/export? Both of which were reported very
recently?

See:

/messages/by-id/CAH2-WzmHVeYY=pjz9x8DhhxVjXHX0pvoQ-MdiB1Tt6=o2GTiKg@mail.gmail.com

and:

/messages/by-id/QpAansP4iVg_ttSs9x81PFAptL2sqR3AS06u8Jksm3_bHJvUwQjHOocRajbxBc3iiLdf9ZMC6gtXjZsxdxvOmqp98hLZcZuWyBDuQxS6uZc=@scottray.io

--
Peter Geoghegan

#6chee.wooson
chee.wooson@gmail.com
In reply to: Peter Geoghegan (#5)
Re: [PATCH v2] Fix exported snapshot xmin handoff race

Hi Peter,

Thanks for the pointers. I spent some time looking at both threads.

The standby subxact-overflow export issue seems separate from this patch: it is
about preserving the contents of a recovery snapshot during export/import.

Scott's recovery-conflict issue is much closer. My v2 makes
ProcArrayInstallImportedXmin() take ProcArrayLock exclusively, which prevents a
snapshot import from installing xmin concurrently with a procarray scan such as
GetConflictingVirtualXIDs(). However, I don't think that alone fixes Scott's
case.

Consider this sequence:

1. Standby recovery is replaying a cleanup record with snapshot conflict
horizon H.

2. Backend A has xmin <= H, so GetConflictingVirtualXIDs() returns A's VXID.

3. GetConflictingVirtualXIDs() releases ProcArrayLock. The recovery conflict
wait list is now fixed to A's VXID.

4. Before A actually ends, backend B imports A's exported snapshot.

5. With v2, B's import takes ProcArrayLock exclusively, but the procarray scan
in step 2 has already finished. So B can still install the same old xmin.

6. Recovery waits for, or cancels, only A's VXID.

7. Once A is gone, recovery can replay the cleanup record even though B is still
running with the imported snapshot.

One possible way to close that remaining hole, other than rescanning, would be
to mark source PGPROCs that have been selected as snapshot-conflict waiters by
recovery, and make ProcArrayInstallImportedXmin() reject importing from such a
source.

The intended sequence would be:

1. Standby recovery calls GetConflictingVirtualXIDs(H).

2. While holding ProcArrayLock, GetConflictingVirtualXIDs() finds backend A with
xmin <= H.

3. Before releasing ProcArrayLock, recovery sets a flag in A's PGPROC saying
that A's current VXID has been selected as a snapshot-conflict waiter.

4. Later, backend B tries to import A's exported snapshot.

5. ProcArrayInstallImportedXmin() takes ProcArrayLock exclusively and finds A as
the source VXID.

6. Because A's PGPROC says that its current VXID is already a recovery
snapshot-conflict waiter, B's import is rejected instead of installing A's
old xmin.

7. A can no longer create new conflicting importers after recovery has selected
it for conflict resolution. Once recovery has waited for or cancelled A, the
original wait list is complete.

This relies on the exclusive ProcArrayLock in ProcArrayInstallImportedXmin():
while recovery is scanning the procarray and setting the flag, a concurrent
import cannot slip through before the flag becomes visible.

I have not included that in this patch because it seems to belong to Scott's
recovery-conflict thread, and it changes the scope of the fix. But I can help
explore that direction there if people think it is preferable to rescanning.

Regards,
Chee

#7chee.wooson
chee.wooson@gmail.com
In reply to: chee.wooson (#1)
[PATCH v3] Fix exported snapshot xmin handoff race

ProcArrayInstallImportedXmin() verifies that the source transaction is
still running and then installs the imported xmin. Both steps have to be
serialized with concurrent proc-array horizon computations; otherwise
VACUUM can scan the importer before the xmin is installed while the source
transaction is still allowed to clear its xmin concurrently.

Take ProcArrayLock in exclusive mode while importing an exported snapshot.
That makes the xmin handoff atomic with respect to ComputeXidHorizons(),
while keeping the no-importer transaction end path unchanged.

Add an injection-point TAP test that pauses VACUUM inside
ComputeXidHorizons() while it holds ProcArrayLock shared, starts SET
TRANSACTION SNAPSHOT concurrently, and verifies that the importer waits for
ProcArrayLock before the imported snapshot is allowed to protect the deleted
tuple.
---
src/backend/commands/vacuum.c | 1 +
src/backend/storage/ipc/procarray.c | 12 +-
src/test/modules/test_misc/meson.build | 1 +
.../test_misc/t/015_export_snapshot.pl | 230 ++++++++++++++++++
4 files changed, 243 insertions(+), 1 deletion(-)
create mode 100644 src/test/modules/test_misc/t/015_export_snapshot.pl

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 38539a6fd3d..d41dc5c679a 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -1139,6 +1139,7 @@ vacuum_get_cutoffs(Relation rel, const VacuumParams *params,
 	 * that only one vacuum process can be working on a particular table at
 	 * any time, and that each vacuum is always an independent transaction.
 	 */
+	INJECTION_POINT("vacuum-get-cutoffs-before-oldest-xmin", NULL);
 	cutoffs->OldestXmin = GetOldestNonRemovableTransactionId(rel);
 	Assert(TransactionIdIsNormal(cutoffs->OldestXmin));
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 60336b31803..9ce5dcfcb68 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -1740,6 +1740,16 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 		xid = UINT32_ACCESS_ONCE(other_xids[index]);
 		xmin = UINT32_ACCESS_ONCE(proc->xmin);
+#ifdef USE_INJECTION_POINTS
+		{
+			char		ip_name[64];
+
+			snprintf(ip_name, sizeof(ip_name),
+					 "compute-xid-horizons-after-reading-pid-%d", proc->pid);
+			InjectionPointRun(ip_name, NULL);
+		}
+#endif
+
 		/*
 		 * Consider both the transaction's Xmin, and its Xid.
 		 *
@@ -2488,7 +2498,7 @@ ProcArrayInstallImportedXmin(TransactionId xmin,
 		return false;
 	/* Get lock so source xact can't end while we're doing this */
-	LWLockAcquire(ProcArrayLock, LW_SHARED);
+	LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
 	/*
 	 * Find the PGPROC entry of the source transaction. (This could use
diff --git a/src/test/modules/test_misc/meson.build b/src/test/modules/test_misc/meson.build
index ee290698b31..eb48ee35d1d 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_export_snapshot.pl',
     ],
     # The injection points are cluster-wide, so disable installcheck
     'runningcheck': false,
diff --git a/src/test/modules/test_misc/t/015_export_snapshot.pl b/src/test/modules/test_misc/t/015_export_snapshot.pl
new file mode 100644
index 00000000000..d7a1eebc469
--- /dev/null
+++ b/src/test/modules/test_misc/t/015_export_snapshot.pl
@@ -0,0 +1,230 @@
+# Copyright (c) 2024-2026, PostgreSQL Global Development Group
+#
+# Test: reproduce the exported-snapshot xmin handoff race.
+#
+# Strategy:
+#   1. Source exports a snapshot that can still see a tuple deleted later.
+#   2. Importer starts a transaction but does not yet import the snapshot, so
+#      its xmin is Invalid.
+#   3. VACUUM reaches vacuum_get_cutoffs(), then waits after
+#      ComputeXidHorizons() has read the importer's still-Invalid xmin.
+#   4. Without the fix, SET TRANSACTION SNAPSHOT can complete while VACUUM is
+#      paused in the proc-array scan.  The test then commits the source
+#      transaction and wakes VACUUM, allowing VACUUM to miss both the importer
+#      and the source xmin and remove the deleted tuple.
+#   5. With the fix, SET TRANSACTION SNAPSHOT waits for ProcArrayLock
+#      exclusive.  The test wakes VACUUM while the source is still open, so
+#      VACUUM sees the source xmin before the importer installs the snapshot.
+#
+# The final query must see the deleted tuple through the imported snapshot.
+# On an unfixed server it instead sees zero rows.
+#
+# Depends only on: injection_points (built-in test module)
+
+use strict;
+use warnings FATAL => 'all';
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+use Time::HiRes qw(usleep);
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+	plan skip_all => 'Injection points not supported by this build';
+}
+
+my $node = PostgreSQL::Test::Cluster->new('export_race_node');
+$node->init;
+$node->append_conf('postgresql.conf',
+	"shared_preload_libraries = 'injection_points'");
+$node->start;
+
+$node->safe_psql('postgres', 'CREATE EXTENSION injection_points');
+
+$node->safe_psql('postgres', qq{
+	CREATE TABLE race_test (id int, data text);
+	INSERT INTO race_test VALUES (1, 'should_be_visible');
+});
+
+# Create the importer before the source.  The test does not depend on a fixed
+# proc-array index, but the wrong-horizon interleaving requires VACUUM to read
+# the importer's Invalid xmin before it reads the source's xmin.
+my $imp   = $node->background_psql('postgres');
+my $src   = $node->background_psql('postgres');
+my $vac   = $node->background_psql('postgres');
+my $del   = $node->background_psql('postgres');
+my $coord = $node->background_psql('postgres');
+
+my $imp_pid = $imp->query("SELECT pg_backend_pid()");
+$imp_pid =~ s/\s+//g;
+my $after_reading_importer_ip =
+  "compute-xid-horizons-after-reading-pid-$imp_pid";
+
+$src->query("BEGIN ISOLATION LEVEL REPEATABLE READ");
+$src->query("SELECT * FROM race_test");
+my $token = $src->query("SELECT pg_export_snapshot()");
+$token =~ s/\s+//g;
+diag("exported snapshot token: $token");
+
+$del->query("DELETE FROM race_test WHERE id = 1");
+$del->query("COMMIT");
+diag("deleter committed");
+
+# Advance the XID counter so that the horizon (latestCompletedXid + 1 when
+# no backend has a valid xmin) is strictly greater than the deleter's XID.
+for (my $i = 0; $i < 100; $i++)
+{
+	$node->safe_psql('postgres', "SELECT txid_current()");
+}
+diag("100 filler XIDs consumed");
+
+# No query after BEGIN: importer xmin stays Invalid until SET TRANSACTION
+# SNAPSHOT, which is essential for this race.
+$imp->query("BEGIN ISOLATION LEVEL REPEATABLE READ");
+
+my $vac_pid = $vac->query("SELECT pg_backend_pid()");
+$vac_pid =~ s/\s+//g;
+diag("VACUUM PID: $vac_pid");
+
+$vac->query("SELECT injection_points_set_local()");
+$vac->query(
+	"SELECT injection_points_attach('vacuum-get-cutoffs-before-oldest-xmin', 'wait')");
+diag("VACUUM attached vacuum-get-cutoffs-before-oldest-xmin");
+
+$vac->query_until(qr/vac_started/,
+	"\\echo vac_started\nVACUUM race_test;\n");
+diag("VACUUM started");
+
+{
+	my $blocked = 0;
+	for (my $i = 0; $i < 1800; $i++)
+	{
+		my $result = $coord->query(
+			"SELECT count(*) = 1 FROM pg_stat_activity"
+			  . " WHERE pid = $vac_pid"
+			  . " AND wait_event_type = 'InjectionPoint'"
+			  . " AND wait_event = 'vacuum-get-cutoffs-before-oldest-xmin'");
+		if ($result =~ /t/)
+		{
+			$blocked = 1;
+			last;
+		}
+		usleep(100_000);
+	}
+	die "VACUUM did not reach vacuum_get_cutoffs within 180s"
+		unless $blocked;
+}
+diag("VACUUM blocked before computing VACUUM cutoffs");
+
+$coord->query(
+	"SELECT injection_points_detach('vacuum-get-cutoffs-before-oldest-xmin')");
+diag("detached vacuum-get-cutoffs-before-oldest-xmin");
+
+$coord->query(
+	"SELECT injection_points_attach('$after_reading_importer_ip', 'wait')");
+diag("coordinator attached $after_reading_importer_ip");
+
+$coord->query(
+	"SELECT injection_points_wakeup('vacuum-get-cutoffs-before-oldest-xmin')");
+diag("woke VACUUM from vacuum_get_cutoffs");
+
+{
+	my $blocked = 0;
+	for (my $i = 0; $i < 1800; $i++)
+	{
+		my $result = $coord->query(
+			"SELECT count(*) = 1 FROM pg_stat_activity"
+			  . " WHERE pid = $vac_pid"
+			  . " AND wait_event_type = 'InjectionPoint'"
+			  . " AND wait_event = '$after_reading_importer_ip'");
+		if ($result =~ /t/)
+		{
+			$blocked = 1;
+			last;
+		}
+		usleep(100_000);
+	}
+	die "VACUUM did not scan the importer within 180s" unless $blocked;
+}
+diag("VACUUM blocked after reading importer xmin");
+
+$imp->query_until(qr/import_started/,
+	"\\echo import_started\nSET TRANSACTION SNAPSHOT '$token';\n"
+	  . "\\echo import_done\n");
+diag("importer started SET TRANSACTION SNAPSHOT");
+
+my $importer_waiting = 0;
+my $importer_wait_state = '';
+for (my $i = 0; $i < 100; $i++)
+{
+	my $result = $coord->query(
+		"SELECT COALESCE(state, '') || '|' ||"
+		  . " COALESCE(wait_event_type, '') || '|' ||"
+		  . " COALESCE(wait_event, '')"
+		  . " FROM pg_stat_activity"
+		  . " WHERE pid = $imp_pid");
+	$importer_wait_state = $result;
+	if ($result =~ /active\|LWLock\|ProcArray/)
+	{
+		$importer_waiting = 1;
+		last;
+	}
+	last if $result eq '';
+	usleep(100_000);
+}
+
+if ($importer_waiting)
+{
+	$coord->query("SELECT injection_points_wakeup('$after_reading_importer_ip')");
+	$coord->query("SELECT injection_points_detach('$after_reading_importer_ip')");
+	diag("woke VACUUM while source transaction is still open");
+	$imp->query_until(qr/import_done/, "");
+	$src->query("COMMIT");
+}
+else
+{
+	$imp->query_until(qr/import_done/, "");
+	$src->query("COMMIT");
+	diag("source committed before waking VACUUM");
+	$coord->query("SELECT injection_points_wakeup('$after_reading_importer_ip')");
+	$coord->query("SELECT injection_points_detach('$after_reading_importer_ip')");
+}
+
+ok($importer_waiting,
+	"importing transaction waits for ProcArrayLock while VACUUM computes horizons");
+
+{
+	my $done = 0;
+	for (my $i = 0; $i < 1800; $i++)
+	{
+		my $result = $coord->query(
+			"SELECT count(*) = 1 FROM pg_stat_activity"
+			  . " WHERE pid = $vac_pid"
+			  . " AND state = 'idle'");
+		if ($result =~ /t/)
+		{
+			$done = 1;
+			last;
+	}
+	usleep(100_000);
+}
+chomp($importer_wait_state);
+diag("importer wait state: $importer_wait_state");
+	die "VACUUM did not finish within 180s" unless $done;
+}
+diag("VACUUM finished");
+
+my $count = $imp->query("SELECT count(*) FROM race_test");
+$count =~ s/\s+//g;
+diag("importer sees $count row(s)");
+
+is($count, 1,
+	"imported snapshot still sees the row after concurrent VACUUM")
+  or diag("BUG DETECTED: export-snapshot xmin race caused "
+		. "premature tuple removal (expected 1 row, got $count)");
+
+$imp->query("COMMIT");
+
+$node->stop;
+done_testing();
-- 
2.43.0
#8chee.wooson
chee.wooson@gmail.com
In reply to: chee.wooson (#1)
[PATCH v4] Fix exported snapshot xmin handoff race

ProcArrayInstallImportedXmin() verifies that the source transaction is
still running and then installs the imported xmin. Both steps have to be
serialized with concurrent proc-array horizon computations; otherwise
VACUUM can scan the importer before the xmin is installed while the source
transaction is still allowed to clear its xmin concurrently.

Take ProcArrayLock in exclusive mode while importing an exported snapshot.
That makes the xmin handoff atomic with respect to ComputeXidHorizons(),
while keeping the no-importer transaction end path unchanged.

Add an injection-point TAP test that pauses VACUUM inside
ComputeXidHorizons() while it holds ProcArrayLock shared, starts SET
TRANSACTION SNAPSHOT concurrently, and verifies that the importer waits for
ProcArrayLock before the imported snapshot is allowed to protect the deleted
tuple.
---
src/backend/commands/vacuum.c | 1 +
src/backend/storage/ipc/procarray.c | 12 +-
src/test/modules/test_misc/meson.build | 1 +
.../test_misc/t/015_export_snapshot.pl | 230 ++++++++++++++++++
4 files changed, 243 insertions(+), 1 deletion(-)
create mode 100644 src/test/modules/test_misc/t/015_export_snapshot.pl

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 38539a6fd3d..d41dc5c679a 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -1139,6 +1139,7 @@ vacuum_get_cutoffs(Relation rel, const VacuumParams *params,
 	 * that only one vacuum process can be working on a particular table at
 	 * any time, and that each vacuum is always an independent transaction.
 	 */
+	INJECTION_POINT("vacuum-get-cutoffs-before-oldest-xmin", NULL);
 	cutoffs->OldestXmin = GetOldestNonRemovableTransactionId(rel);
 	Assert(TransactionIdIsNormal(cutoffs->OldestXmin));
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 60336b31803..9ce5dcfcb68 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -1740,6 +1740,16 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 		xid = UINT32_ACCESS_ONCE(other_xids[index]);
 		xmin = UINT32_ACCESS_ONCE(proc->xmin);
+#ifdef USE_INJECTION_POINTS
+		{
+			char		ip_name[64];
+
+			snprintf(ip_name, sizeof(ip_name),
+					 "compute-xid-horizons-after-reading-pid-%d", proc->pid);
+			InjectionPointRun(ip_name, NULL);
+		}
+#endif
+
 		/*
 		 * Consider both the transaction's Xmin, and its Xid.
 		 *
@@ -2488,7 +2498,7 @@ ProcArrayInstallImportedXmin(TransactionId xmin,
 		return false;
 	/* Get lock so source xact can't end while we're doing this */
-	LWLockAcquire(ProcArrayLock, LW_SHARED);
+	LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
 	/*
 	 * Find the PGPROC entry of the source transaction. (This could use
diff --git a/src/test/modules/test_misc/meson.build b/src/test/modules/test_misc/meson.build
index ee290698b31..eb48ee35d1d 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_export_snapshot.pl',
     ],
     # The injection points are cluster-wide, so disable installcheck
     'runningcheck': false,
diff --git a/src/test/modules/test_misc/t/015_export_snapshot.pl b/src/test/modules/test_misc/t/015_export_snapshot.pl
new file mode 100644
index 00000000000..d7a1eebc469
--- /dev/null
+++ b/src/test/modules/test_misc/t/015_export_snapshot.pl
@@ -0,0 +1,230 @@
+# Copyright (c) 2024-2026, PostgreSQL Global Development Group
+#
+# Test: reproduce the exported-snapshot xmin handoff race.
+#
+# Strategy:
+#   1. Source exports a snapshot that can still see a tuple deleted later.
+#   2. Importer starts a transaction but does not yet import the snapshot, so
+#      its xmin is Invalid.
+#   3. VACUUM reaches vacuum_get_cutoffs(), then waits after
+#      ComputeXidHorizons() has read the importer's still-Invalid xmin.
+#   4. Without the fix, SET TRANSACTION SNAPSHOT can complete while VACUUM is
+#      paused in the proc-array scan.  The test then commits the source
+#      transaction and wakes VACUUM, allowing VACUUM to miss both the importer
+#      and the source xmin and remove the deleted tuple.
+#   5. With the fix, SET TRANSACTION SNAPSHOT waits for ProcArrayLock
+#      exclusive.  The test wakes VACUUM while the source is still open, so
+#      VACUUM sees the source xmin before the importer installs the snapshot.
+#
+# The final query must see the deleted tuple through the imported snapshot.
+# On an unfixed server it instead sees zero rows.
+#
+# Depends only on: injection_points (built-in test module)
+
+use strict;
+use warnings FATAL => 'all';
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+use Time::HiRes qw(usleep);
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+	plan skip_all => 'Injection points not supported by this build';
+}
+
+my $node = PostgreSQL::Test::Cluster->new('export_race_node');
+$node->init;
+$node->append_conf('postgresql.conf',
+	"shared_preload_libraries = 'injection_points'");
+$node->start;
+
+$node->safe_psql('postgres', 'CREATE EXTENSION injection_points');
+
+$node->safe_psql('postgres', qq{
+	CREATE TABLE race_test (id int, data text);
+	INSERT INTO race_test VALUES (1, 'should_be_visible');
+});
+
+# Create the importer before the source.  The test does not depend on a fixed
+# proc-array index, but the wrong-horizon interleaving requires VACUUM to read
+# the importer's Invalid xmin before it reads the source's xmin.
+my $imp   = $node->background_psql('postgres');
+my $src   = $node->background_psql('postgres');
+my $vac   = $node->background_psql('postgres');
+my $del   = $node->background_psql('postgres');
+my $coord = $node->background_psql('postgres');
+
+my $imp_pid = $imp->query("SELECT pg_backend_pid()");
+$imp_pid =~ s/\s+//g;
+my $after_reading_importer_ip =
+  "compute-xid-horizons-after-reading-pid-$imp_pid";
+
+$src->query("BEGIN ISOLATION LEVEL REPEATABLE READ");
+$src->query("SELECT * FROM race_test");
+my $token = $src->query("SELECT pg_export_snapshot()");
+$token =~ s/\s+//g;
+diag("exported snapshot token: $token");
+
+$del->query("DELETE FROM race_test WHERE id = 1");
+$del->query("COMMIT");
+diag("deleter committed");
+
+# Advance the XID counter so that the horizon (latestCompletedXid + 1 when
+# no backend has a valid xmin) is strictly greater than the deleter's XID.
+for (my $i = 0; $i < 100; $i++)
+{
+	$node->safe_psql('postgres', "SELECT txid_current()");
+}
+diag("100 filler XIDs consumed");
+
+# No query after BEGIN: importer xmin stays Invalid until SET TRANSACTION
+# SNAPSHOT, which is essential for this race.
+$imp->query("BEGIN ISOLATION LEVEL REPEATABLE READ");
+
+my $vac_pid = $vac->query("SELECT pg_backend_pid()");
+$vac_pid =~ s/\s+//g;
+diag("VACUUM PID: $vac_pid");
+
+$vac->query("SELECT injection_points_set_local()");
+$vac->query(
+	"SELECT injection_points_attach('vacuum-get-cutoffs-before-oldest-xmin', 'wait')");
+diag("VACUUM attached vacuum-get-cutoffs-before-oldest-xmin");
+
+$vac->query_until(qr/vac_started/,
+	"\\echo vac_started\nVACUUM race_test;\n");
+diag("VACUUM started");
+
+{
+	my $blocked = 0;
+	for (my $i = 0; $i < 1800; $i++)
+	{
+		my $result = $coord->query(
+			"SELECT count(*) = 1 FROM pg_stat_activity"
+			  . " WHERE pid = $vac_pid"
+			  . " AND wait_event_type = 'InjectionPoint'"
+			  . " AND wait_event = 'vacuum-get-cutoffs-before-oldest-xmin'");
+		if ($result =~ /t/)
+		{
+			$blocked = 1;
+			last;
+		}
+		usleep(100_000);
+	}
+	die "VACUUM did not reach vacuum_get_cutoffs within 180s"
+		unless $blocked;
+}
+diag("VACUUM blocked before computing VACUUM cutoffs");
+
+$coord->query(
+	"SELECT injection_points_detach('vacuum-get-cutoffs-before-oldest-xmin')");
+diag("detached vacuum-get-cutoffs-before-oldest-xmin");
+
+$coord->query(
+	"SELECT injection_points_attach('$after_reading_importer_ip', 'wait')");
+diag("coordinator attached $after_reading_importer_ip");
+
+$coord->query(
+	"SELECT injection_points_wakeup('vacuum-get-cutoffs-before-oldest-xmin')");
+diag("woke VACUUM from vacuum_get_cutoffs");
+
+{
+	my $blocked = 0;
+	for (my $i = 0; $i < 1800; $i++)
+	{
+		my $result = $coord->query(
+			"SELECT count(*) = 1 FROM pg_stat_activity"
+			  . " WHERE pid = $vac_pid"
+			  . " AND wait_event_type = 'InjectionPoint'"
+			  . " AND wait_event = '$after_reading_importer_ip'");
+		if ($result =~ /t/)
+		{
+			$blocked = 1;
+			last;
+		}
+		usleep(100_000);
+	}
+	die "VACUUM did not scan the importer within 180s" unless $blocked;
+}
+diag("VACUUM blocked after reading importer xmin");
+
+$imp->query_until(qr/import_started/,
+	"\\echo import_started\nSET TRANSACTION SNAPSHOT '$token';\n"
+	  . "\\echo import_done\n");
+diag("importer started SET TRANSACTION SNAPSHOT");
+
+my $importer_waiting = 0;
+my $importer_wait_state = '';
+for (my $i = 0; $i < 100; $i++)
+{
+	my $result = $coord->query(
+		"SELECT COALESCE(state, '') || '|' ||"
+		  . " COALESCE(wait_event_type, '') || '|' ||"
+		  . " COALESCE(wait_event, '')"
+		  . " FROM pg_stat_activity"
+		  . " WHERE pid = $imp_pid");
+	$importer_wait_state = $result;
+	if ($result =~ /active\|LWLock\|ProcArray/)
+	{
+		$importer_waiting = 1;
+		last;
+	}
+	last if $result eq '';
+	usleep(100_000);
+}
+
+if ($importer_waiting)
+{
+	$coord->query("SELECT injection_points_wakeup('$after_reading_importer_ip')");
+	$coord->query("SELECT injection_points_detach('$after_reading_importer_ip')");
+	diag("woke VACUUM while source transaction is still open");
+	$imp->query_until(qr/import_done/, "");
+	$src->query("COMMIT");
+}
+else
+{
+	$imp->query_until(qr/import_done/, "");
+	$src->query("COMMIT");
+	diag("source committed before waking VACUUM");
+	$coord->query("SELECT injection_points_wakeup('$after_reading_importer_ip')");
+	$coord->query("SELECT injection_points_detach('$after_reading_importer_ip')");
+}
+
+ok($importer_waiting,
+	"importing transaction waits for ProcArrayLock while VACUUM computes horizons");
+
+{
+	my $done = 0;
+	for (my $i = 0; $i < 1800; $i++)
+	{
+		my $result = $coord->query(
+			"SELECT count(*) = 1 FROM pg_stat_activity"
+			  . " WHERE pid = $vac_pid"
+			  . " AND state = 'idle'");
+		if ($result =~ /t/)
+		{
+			$done = 1;
+			last;
+	}
+	usleep(100_000);
+}
+chomp($importer_wait_state);
+diag("importer wait state: $importer_wait_state");
+	die "VACUUM did not finish within 180s" unless $done;
+}
+diag("VACUUM finished");
+
+my $count = $imp->query("SELECT count(*) FROM race_test");
+$count =~ s/\s+//g;
+diag("importer sees $count row(s)");
+
+is($count, 1,
+	"imported snapshot still sees the row after concurrent VACUUM")
+  or diag("BUG DETECTED: export-snapshot xmin race caused "
+		. "premature tuple removal (expected 1 row, got $count)");
+
+$imp->query("COMMIT");
+
+$node->stop;
+done_testing();
-- 
2.43.0
#9chee.wooson
chee.wooson@gmail.com
In reply to: chee.wooson (#1)
[PATCH v4] Fix exported snapshot xmin handoff race

Hi,

The v2 patch had TAP test failures on some CommitFest platforms. I
prepared v3 to fix those test issues, but accidentally sent it inline
instead of as a patch attachment.

Attached is v4. It has the same code changes as v3, but is sent as an
attachment so that the CommitFest app and cfbot can process it properly.

Thanks,
Chee

Attachments:

v4-0001-Fix-exported-snapshot-xmin-handoff-race.patchtext/x-patch; charset=UTF-8; name=v4-0001-Fix-exported-snapshot-xmin-handoff-race.patchDownload+243-2