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> (raw)
In-Reply-To: <20260729101045.422679-1-chee.wooson@gmail.com>
References: <20260729101045.422679-1-chee.wooson@gmail.com>
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.cindex 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.cindex 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.cindex 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.cindex 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.hindex 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
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: chee.wooson@gmail.com
Subject: Re: [PATCH v1 2/2] Fix exported snapshot xmin handoff race
In-Reply-To: <20260729101045.422679-3-chee.wooson@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox