From: "chee.wooson" <chee.wooson@gmail.com>
To: pgsql-hackers@postgresql.org
Cc: "chee.wooson" <chee.wooson@gmail.com>
Subject: [PATCH v1 2/2] Fix exported snapshot xmin handoff race
Date: Wed, 29 Jul 2026 18:10:45 +0800
Message-ID: <20260729101045.422679-3-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/20260729101045.422679-3-chee.wooson%40gmail.com>
Precedence: bulk

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