From: "chee.wooson" <chee.wooson@gmail.com>
To: pgsql-hackers@postgresql.org
Cc: "chee.wooson" <chee.wooson@gmail.com>
Subject: [PATCH v3] Fix exported snapshot xmin handoff race
Date: Mon,  3 Aug 2026 17:31:41 +0800
Message-ID: <20260803093141.4010626-1-chee.wooson@gmail.com>
In-Reply-To: <20260729101045.422679-1-chee.wooson@gmail.com>
References: <20260729101045.422679-1-chee.wooson@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/20260803093141.4010626-1-chee.wooson%40gmail.com>
Precedence: bulk

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