diff --git a/src/backend/access/transam/varsup.c b/src/backend/access/transam/varsup.c index cede579d73..664735b381 100644 --- a/src/backend/access/transam/varsup.c +++ b/src/backend/access/transam/varsup.c @@ -186,20 +186,23 @@ GetNewTransactionId(bool isSubXact) * latestCompletedXid is present in the ProcArray, which is essential for * correct OldestXmin tracking; see src/backend/access/transam/README. * - * XXX by storing xid into MyPgXact without acquiring ProcArrayLock, we - * are relying on fetch/store of an xid to be atomic, else other backends - * might see a partially-set xid here. But holding both locks at once - * would be a nasty concurrency hit. So for now, assume atomicity. - * * Note that readers of PGXACT xid fields should be careful to fetch the * value only once, rather than assume they can read a value multiple - * times and get the same answer each time. + * times and get the same answer each time. Note we are assuming that + * TransactionId and int fetch/store are atomic. * * The same comments apply to the subxact xid count and overflow fields. * - * A solution to the atomic-store problem would be to give each PGXACT its - * own spinlock used only for fetching/storing that PGXACT's xid and - * related fields. + * Use of a write barrier prevents dangerous code rearrangement in this + * function; other backends could otherwise e.g. be examining my subxids + * info concurrently, and we don't want them to see an invalid + * intermediate state, such as an incremented nxids before the array entry + * is filled. + * + * Other processes that read nxids should do so before reading xids + * elements with a pg_read_barrier() in between, so that they can be sure + * not to read an uninitialized array element; see + * src/backend/storage/lmgr/README.barrier. * * If there's no room to fit a subtransaction XID into PGPROC, set the * cache-overflowed flag instead. This forces readers to look in @@ -211,31 +214,20 @@ GetNewTransactionId(bool isSubXact) * window *will* include the parent XID, so they will deliver the correct * answer later on when someone does have a reason to inquire.) */ + if (!isSubXact) + MyPgXact->xid = xid; /* LWLockRelease acts as barrier */ + else { - /* - * Use volatile pointer to prevent code rearrangement; other backends - * could be examining my subxids info concurrently, and we don't want - * them to see an invalid intermediate state, such as incrementing - * nxids before filling the array entry. Note we are assuming that - * TransactionId and int fetch/store are atomic. - */ - volatile PGPROC *myproc = MyProc; - volatile PGXACT *mypgxact = MyPgXact; + int nxids = MyPgXact->nxids; - if (!isSubXact) - mypgxact->xid = xid; - else + if (nxids < PGPROC_MAX_CACHED_SUBXIDS) { - int nxids = mypgxact->nxids; - - if (nxids < PGPROC_MAX_CACHED_SUBXIDS) - { - myproc->subxids.xids[nxids] = xid; - mypgxact->nxids = nxids + 1; - } - else - mypgxact->overflowed = true; + MyProc->subxids.xids[nxids] = xid; + pg_write_barrier(); + MyPgXact->nxids = nxids + 1; } + else + MyPgXact->overflowed = true; } LWLockRelease(XidGenLock); diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index c861f3e17f..dc7e875680 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -61,6 +61,7 @@ #include "utils/rel.h" #include "utils/snapmgr.h" +#define UINT32_ACCESS_ONCE(var) ((uint32)(*((volatile uint32 *)&(var)))) /* Our shared memory area */ typedef struct ProcArrayStruct @@ -483,7 +484,7 @@ ProcArrayEndTransactionInternal(PGPROC *proc, PGXACT *pgxact, static void ProcArrayGroupClearXid(PGPROC *proc, TransactionId latestXid) { - volatile PROC_HDR *procglobal = ProcGlobal; + PROC_HDR *procglobal = ProcGlobal; uint32 nextidx; uint32 wakeidx; @@ -1077,16 +1078,17 @@ TransactionIdIsInProgress(TransactionId xid) for (i = 0; i < arrayP->numProcs; i++) { int pgprocno = arrayP->pgprocnos[i]; - volatile PGPROC *proc = &allProcs[pgprocno]; - volatile PGXACT *pgxact = &allPgXact[pgprocno]; + PGPROC *proc = &allProcs[pgprocno]; + PGXACT *pgxact = &allPgXact[pgprocno]; TransactionId pxid; + int pxids; /* Ignore my own proc --- dealt with it above */ if (proc == MyProc) continue; /* Fetch xid just once - see GetNewTransactionId */ - pxid = pgxact->xid; + pxid = UINT32_ACCESS_ONCE(pgxact->xid); if (!TransactionIdIsValid(pxid)) continue; @@ -1111,10 +1113,12 @@ TransactionIdIsInProgress(TransactionId xid) /* * Step 2: check the cached child-Xids arrays */ - for (j = pgxact->nxids - 1; j >= 0; j--) + pxids = pgxact->nxids; + pg_read_barrier(); /* pairs with barrier in GetNewTransactionId() */ + for (j = pxids - 1; j >= 0; j--) { /* Fetch xid just once - see GetNewTransactionId */ - TransactionId cxid = proc->subxids.xids[j]; + TransactionId cxid = UINT32_ACCESS_ONCE(proc->subxids.xids[j]); if (TransactionIdEquals(cxid, xid)) { @@ -1233,12 +1237,12 @@ TransactionIdIsActive(TransactionId xid) for (i = 0; i < arrayP->numProcs; i++) { int pgprocno = arrayP->pgprocnos[i]; - volatile PGPROC *proc = &allProcs[pgprocno]; - volatile PGXACT *pgxact = &allPgXact[pgprocno]; + PGPROC *proc = &allProcs[pgprocno]; + PGXACT *pgxact = &allPgXact[pgprocno]; TransactionId pxid; /* Fetch xid just once - see GetNewTransactionId */ - pxid = pgxact->xid; + pxid = UINT32_ACCESS_ONCE(pgxact->xid); if (!TransactionIdIsValid(pxid)) continue; @@ -1321,8 +1325,8 @@ GetOldestXmin(Relation rel, int flags) int index; bool allDbs; - volatile TransactionId replication_slot_xmin = InvalidTransactionId; - volatile TransactionId replication_slot_catalog_xmin = InvalidTransactionId; + TransactionId replication_slot_xmin = InvalidTransactionId; + TransactionId replication_slot_catalog_xmin = InvalidTransactionId; /* * If we're not computing a relation specific limit, or if a shared @@ -1349,8 +1353,8 @@ GetOldestXmin(Relation rel, int flags) for (index = 0; index < arrayP->numProcs; index++) { int pgprocno = arrayP->pgprocnos[index]; - volatile PGPROC *proc = &allProcs[pgprocno]; - volatile PGXACT *pgxact = &allPgXact[pgprocno]; + PGPROC *proc = &allProcs[pgprocno]; + PGXACT *pgxact = &allPgXact[pgprocno]; if (pgxact->vacuumFlags & (flags & PROCARRAY_PROC_FLAGS_MASK)) continue; @@ -1360,7 +1364,7 @@ GetOldestXmin(Relation rel, int flags) proc->databaseId == 0) /* always include WalSender */ { /* Fetch xid just once - see GetNewTransactionId */ - TransactionId xid = pgxact->xid; + TransactionId xid = UINT32_ACCESS_ONCE(pgxact->xid); /* First consider the transaction's own Xid, if any */ if (TransactionIdIsNormal(xid) && @@ -1374,14 +1378,18 @@ GetOldestXmin(Relation rel, int flags) * have an Xmin but not (yet) an Xid; conversely, if it has an * Xid, that could determine some not-yet-set Xmin. */ - xid = pgxact->xmin; /* Fetch just once */ + xid = UINT32_ACCESS_ONCE(pgxact->xmin); if (TransactionIdIsNormal(xid) && TransactionIdPrecedes(xid, result)) result = xid; } } - /* fetch into volatile var while ProcArrayLock is held */ + /* + * Fetch into local variable while ProcArrayLock is held - the + * LWLockRelease below is a barrier, ensuring this happens inside the + * lock. + */ replication_slot_xmin = procArray->replication_slot_xmin; replication_slot_catalog_xmin = procArray->replication_slot_catalog_xmin; @@ -1518,8 +1526,8 @@ GetSnapshotData(Snapshot snapshot) int count = 0; int subcount = 0; bool suboverflowed = false; - volatile TransactionId replication_slot_xmin = InvalidTransactionId; - volatile TransactionId replication_slot_catalog_xmin = InvalidTransactionId; + TransactionId replication_slot_xmin = InvalidTransactionId; + TransactionId replication_slot_catalog_xmin = InvalidTransactionId; Assert(snapshot != NULL); @@ -1585,7 +1593,7 @@ GetSnapshotData(Snapshot snapshot) for (index = 0; index < numProcs; index++) { int pgprocno = pgprocnos[index]; - volatile PGXACT *pgxact = &allPgXact[pgprocno]; + PGXACT *pgxact = &allPgXact[pgprocno]; TransactionId xid; /* @@ -1597,13 +1605,13 @@ GetSnapshotData(Snapshot snapshot) continue; /* Update globalxmin to be the smallest valid xmin */ - xid = pgxact->xmin; /* fetch just once */ + xid = UINT32_ACCESS_ONCE(pgxact->xmin); if (TransactionIdIsNormal(xid) && NormalTransactionIdPrecedes(xid, globalxmin)) globalxmin = xid; /* Fetch xid just once - see GetNewTransactionId */ - xid = pgxact->xid; + xid = UINT32_ACCESS_ONCE(pgxact->xid); /* * If the transaction has no XID assigned, we can skip it; it @@ -1652,7 +1660,9 @@ GetSnapshotData(Snapshot snapshot) if (nxids > 0) { - volatile PGPROC *proc = &allProcs[pgprocno]; + PGPROC *proc = &allProcs[pgprocno]; + + pg_read_barrier(); /* pairs with GetNewTransactionId */ memcpy(snapshot->subxip + subcount, (void *) proc->subxids.xids, @@ -1702,7 +1712,11 @@ GetSnapshotData(Snapshot snapshot) } - /* fetch into volatile var while ProcArrayLock is held */ + /* + * Fetch into local variable while ProcArrayLock is held - the + * LWLockRelease below is a barrier, ensuring this happens inside the + * lock. + */ replication_slot_xmin = procArray->replication_slot_xmin; replication_slot_catalog_xmin = procArray->replication_slot_catalog_xmin; @@ -1810,8 +1824,8 @@ ProcArrayInstallImportedXmin(TransactionId xmin, for (index = 0; index < arrayP->numProcs; index++) { int pgprocno = arrayP->pgprocnos[index]; - volatile PGPROC *proc = &allProcs[pgprocno]; - volatile PGXACT *pgxact = &allPgXact[pgprocno]; + PGPROC *proc = &allProcs[pgprocno]; + PGXACT *pgxact = &allPgXact[pgprocno]; TransactionId xid; /* Ignore procs running LAZY VACUUM */ @@ -1836,7 +1850,7 @@ ProcArrayInstallImportedXmin(TransactionId xmin, /* * Likewise, let's just make real sure its xmin does cover us. */ - xid = pgxact->xmin; /* fetch just once */ + xid = UINT32_ACCESS_ONCE(pgxact->xmin); if (!TransactionIdIsNormal(xid) || !TransactionIdPrecedesOrEquals(xid, xmin)) continue; @@ -1872,7 +1886,7 @@ ProcArrayInstallRestoredXmin(TransactionId xmin, PGPROC *proc) { bool result = false; TransactionId xid; - volatile PGXACT *pgxact; + PGXACT *pgxact; Assert(TransactionIdIsNormal(xmin)); Assert(proc != NULL); @@ -1888,7 +1902,7 @@ ProcArrayInstallRestoredXmin(TransactionId xmin, PGPROC *proc) * can't go backwards. Also, make sure it's running in the same database, * so that the per-database xmin cannot go backwards. */ - xid = pgxact->xmin; /* fetch just once */ + xid = UINT32_ACCESS_ONCE(pgxact->xmin); if (proc->databaseId == MyDatabaseId && TransactionIdIsNormal(xid) && TransactionIdPrecedesOrEquals(xid, xmin)) @@ -1995,11 +2009,11 @@ GetRunningTransactionData(void) for (index = 0; index < arrayP->numProcs; index++) { int pgprocno = arrayP->pgprocnos[index]; - volatile PGXACT *pgxact = &allPgXact[pgprocno]; + PGXACT *pgxact = &allPgXact[pgprocno]; TransactionId xid; /* Fetch xid just once - see GetNewTransactionId */ - xid = pgxact->xid; + xid = UINT32_ACCESS_ONCE(pgxact->xid); /* * We don't need to store transactions that don't have a TransactionId @@ -2039,8 +2053,8 @@ GetRunningTransactionData(void) for (index = 0; index < arrayP->numProcs; index++) { int pgprocno = arrayP->pgprocnos[index]; - volatile PGPROC *proc = &allProcs[pgprocno]; - volatile PGXACT *pgxact = &allPgXact[pgprocno]; + PGPROC *proc = &allProcs[pgprocno]; + PGXACT *pgxact = &allPgXact[pgprocno]; int nxids; /* @@ -2050,6 +2064,9 @@ GetRunningTransactionData(void) nxids = pgxact->nxids; if (nxids > 0) { + /* barrier not really required, as XidGenLock is held, but ... */ + pg_read_barrier(); /* pairs with GetNewTransactionId */ + memcpy(&xids[count], (void *) proc->subxids.xids, nxids * sizeof(TransactionId)); count += nxids; @@ -2131,11 +2148,11 @@ GetOldestActiveTransactionId(void) for (index = 0; index < arrayP->numProcs; index++) { int pgprocno = arrayP->pgprocnos[index]; - volatile PGXACT *pgxact = &allPgXact[pgprocno]; + PGXACT *pgxact = &allPgXact[pgprocno]; TransactionId xid; /* Fetch xid just once - see GetNewTransactionId */ - xid = pgxact->xid; + xid = UINT32_ACCESS_ONCE(pgxact->xid); if (!TransactionIdIsNormal(xid)) continue; @@ -2229,11 +2246,11 @@ GetOldestSafeDecodingTransactionId(bool catalogOnly) for (index = 0; index < arrayP->numProcs; index++) { int pgprocno = arrayP->pgprocnos[index]; - volatile PGXACT *pgxact = &allPgXact[pgprocno]; + PGXACT *pgxact = &allPgXact[pgprocno]; TransactionId xid; /* Fetch xid just once - see GetNewTransactionId */ - xid = pgxact->xid; + xid = UINT32_ACCESS_ONCE(pgxact->xid); if (!TransactionIdIsNormal(xid)) continue; @@ -2283,8 +2300,8 @@ GetVirtualXIDsDelayingChkpt(int *nvxids) for (index = 0; index < arrayP->numProcs; index++) { int pgprocno = arrayP->pgprocnos[index]; - volatile PGPROC *proc = &allProcs[pgprocno]; - volatile PGXACT *pgxact = &allPgXact[pgprocno]; + PGPROC *proc = &allProcs[pgprocno]; + PGXACT *pgxact = &allPgXact[pgprocno]; if (pgxact->delayChkpt) { @@ -2323,8 +2340,8 @@ HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids) for (index = 0; index < arrayP->numProcs; index++) { int pgprocno = arrayP->pgprocnos[index]; - volatile PGPROC *proc = &allProcs[pgprocno]; - volatile PGXACT *pgxact = &allPgXact[pgprocno]; + PGPROC *proc = &allProcs[pgprocno]; + PGXACT *pgxact = &allPgXact[pgprocno]; VirtualTransactionId vxid; GET_VXID_FROM_PGPROC(vxid, *proc); @@ -2433,8 +2450,8 @@ BackendXidGetPid(TransactionId xid) for (index = 0; index < arrayP->numProcs; index++) { int pgprocno = arrayP->pgprocnos[index]; - volatile PGPROC *proc = &allProcs[pgprocno]; - volatile PGXACT *pgxact = &allPgXact[pgprocno]; + PGPROC *proc = &allProcs[pgprocno]; + PGXACT *pgxact = &allPgXact[pgprocno]; if (pgxact->xid == xid) { @@ -2505,8 +2522,8 @@ GetCurrentVirtualXIDs(TransactionId limitXmin, bool excludeXmin0, for (index = 0; index < arrayP->numProcs; index++) { int pgprocno = arrayP->pgprocnos[index]; - volatile PGPROC *proc = &allProcs[pgprocno]; - volatile PGXACT *pgxact = &allPgXact[pgprocno]; + PGPROC *proc = &allProcs[pgprocno]; + PGXACT *pgxact = &allPgXact[pgprocno]; if (proc == MyProc) continue; @@ -2517,7 +2534,7 @@ GetCurrentVirtualXIDs(TransactionId limitXmin, bool excludeXmin0, if (allDbs || proc->databaseId == MyDatabaseId) { /* Fetch xmin just once - might change on us */ - TransactionId pxmin = pgxact->xmin; + TransactionId pxmin = UINT32_ACCESS_ONCE(pgxact->xmin); if (excludeXmin0 && !TransactionIdIsValid(pxmin)) continue; @@ -2602,8 +2619,8 @@ GetConflictingVirtualXIDs(TransactionId limitXmin, Oid dbOid) for (index = 0; index < arrayP->numProcs; index++) { int pgprocno = arrayP->pgprocnos[index]; - volatile PGPROC *proc = &allProcs[pgprocno]; - volatile PGXACT *pgxact = &allPgXact[pgprocno]; + PGPROC *proc = &allProcs[pgprocno]; + PGXACT *pgxact = &allPgXact[pgprocno]; /* Exclude prepared transactions */ if (proc->pid == 0) @@ -2613,7 +2630,7 @@ GetConflictingVirtualXIDs(TransactionId limitXmin, Oid dbOid) proc->databaseId == dbOid) { /* Fetch xmin just once - can't change on us, but good coding */ - TransactionId pxmin = pgxact->xmin; + TransactionId pxmin = UINT32_ACCESS_ONCE(pgxact->xmin); /* * We ignore an invalid pxmin because this means that backend has @@ -2661,7 +2678,7 @@ CancelVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode) for (index = 0; index < arrayP->numProcs; index++) { int pgprocno = arrayP->pgprocnos[index]; - volatile PGPROC *proc = &allProcs[pgprocno]; + PGPROC *proc = &allProcs[pgprocno]; VirtualTransactionId procvxid; GET_VXID_FROM_PGPROC(procvxid, *proc); @@ -2716,8 +2733,8 @@ MinimumActiveBackends(int min) for (index = 0; index < arrayP->numProcs; index++) { int pgprocno = arrayP->pgprocnos[index]; - volatile PGPROC *proc = &allProcs[pgprocno]; - volatile PGXACT *pgxact = &allPgXact[pgprocno]; + PGPROC *proc = &allProcs[pgprocno]; + PGXACT *pgxact = &allPgXact[pgprocno]; /* * Since we're not holding a lock, need to be prepared to deal with @@ -2763,7 +2780,7 @@ CountDBBackends(Oid databaseid) for (index = 0; index < arrayP->numProcs; index++) { int pgprocno = arrayP->pgprocnos[index]; - volatile PGPROC *proc = &allProcs[pgprocno]; + PGPROC *proc = &allProcs[pgprocno]; if (proc->pid == 0) continue; /* do not count prepared xacts */ @@ -2793,7 +2810,7 @@ CountDBConnections(Oid databaseid) for (index = 0; index < arrayP->numProcs; index++) { int pgprocno = arrayP->pgprocnos[index]; - volatile PGPROC *proc = &allProcs[pgprocno]; + PGPROC *proc = &allProcs[pgprocno]; if (proc->pid == 0) continue; /* do not count prepared xacts */ @@ -2825,7 +2842,7 @@ CancelDBBackends(Oid databaseid, ProcSignalReason sigmode, bool conflictPending) for (index = 0; index < arrayP->numProcs; index++) { int pgprocno = arrayP->pgprocnos[index]; - volatile PGPROC *proc = &allProcs[pgprocno]; + PGPROC *proc = &allProcs[pgprocno]; if (databaseid == InvalidOid || proc->databaseId == databaseid) { @@ -2864,7 +2881,7 @@ CountUserBackends(Oid roleid) for (index = 0; index < arrayP->numProcs; index++) { int pgprocno = arrayP->pgprocnos[index]; - volatile PGPROC *proc = &allProcs[pgprocno]; + PGPROC *proc = &allProcs[pgprocno]; if (proc->pid == 0) continue; /* do not count prepared xacts */ @@ -2927,8 +2944,8 @@ CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared) for (index = 0; index < arrayP->numProcs; index++) { int pgprocno = arrayP->pgprocnos[index]; - volatile PGPROC *proc = &allProcs[pgprocno]; - volatile PGXACT *pgxact = &allPgXact[pgprocno]; + PGPROC *proc = &allProcs[pgprocno]; + PGXACT *pgxact = &allPgXact[pgprocno]; if (proc->databaseId != databaseId) continue; @@ -3017,6 +3034,7 @@ ProcArrayGetReplicationSlotXmin(TransactionId *xmin, #define XidCacheRemove(i) \ do { \ MyProc->subxids.xids[i] = MyProc->subxids.xids[MyPgXact->nxids - 1]; \ + pg_write_barrier(); \ MyPgXact->nxids--; \ } while (0) @@ -3044,6 +3062,11 @@ XidCacheRemoveRunningXids(TransactionId xid, * possible this could be relaxed since we know this routine is only used * to abort subtransactions, but pending closer analysis we'd best be * conservative. + * + * Note that we do not have to be careful about memory ordering of our own + * reads wrt. GetNewTransactionId() here - only this process can modify + * relevant fields of MyProc/MyPgXact. But we do have to be careful about + * our own writes being well ordered. */ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); @@ -3401,8 +3424,7 @@ ExpireOldKnownAssignedTransactionIds(TransactionId xid) static void KnownAssignedXidsCompress(bool force) { - /* use volatile pointer to prevent code rearrangement */ - volatile ProcArrayStruct *pArray = procArray; + ProcArrayStruct *pArray = procArray; int head, tail; int compress_index; @@ -3465,8 +3487,7 @@ static void KnownAssignedXidsAdd(TransactionId from_xid, TransactionId to_xid, bool exclusive_lock) { - /* use volatile pointer to prevent code rearrangement */ - volatile ProcArrayStruct *pArray = procArray; + ProcArrayStruct *pArray = procArray; TransactionId next_xid; int head, tail; @@ -3583,8 +3604,7 @@ KnownAssignedXidsAdd(TransactionId from_xid, TransactionId to_xid, static bool KnownAssignedXidsSearch(TransactionId xid, bool remove) { - /* use volatile pointer to prevent code rearrangement */ - volatile ProcArrayStruct *pArray = procArray; + ProcArrayStruct *pArray = procArray; int first, last; int head; @@ -3738,8 +3758,7 @@ KnownAssignedXidsRemoveTree(TransactionId xid, int nsubxids, static void KnownAssignedXidsRemovePreceding(TransactionId removeXid) { - /* use volatile pointer to prevent code rearrangement */ - volatile ProcArrayStruct *pArray = procArray; + ProcArrayStruct *pArray = procArray; int count = 0; int head, tail, @@ -3924,8 +3943,7 @@ KnownAssignedXidsGetOldestXmin(void) static void KnownAssignedXidsDisplay(int trace_level) { - /* use volatile pointer to prevent code rearrangement */ - volatile ProcArrayStruct *pArray = procArray; + ProcArrayStruct *pArray = procArray; StringInfoData buf; int head, tail, @@ -3963,8 +3981,7 @@ KnownAssignedXidsDisplay(int trace_level) static void KnownAssignedXidsReset(void) { - /* use volatile pointer to prevent code rearrangement */ - volatile ProcArrayStruct *pArray = procArray; + ProcArrayStruct *pArray = procArray; LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);