database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-10-31 10:30:33 +03:00
Code Activity

Remove volatiles from {procarray,volatile}.c and fix memory ordering issue.

Browse Source 
The use of volatiles in procarray.c largely originated from the time
when postgres did not have reliable compiler and memory
barriers. That's not the case anymore, so we can do better.

Several of the functions in procarray.c can be bottlenecks, and
removal of volatile yields mildly better code.

The new state, with explicit memory barriers, is also more
correct. The previous use of volatile did not actually deliver
sufficient guarantees on weakly ordered machines, in particular the
logic in GetNewTransactionId() does not look safe.  It seems unlikely
to be a problem in practice, but worth fixing.

Thomas and I independently wrote a patch for this.

Reported-By: Andres Freund and Thomas Munro
Author: Andres Freund, with cherrypicked changes from a patch by Thomas Munro
Discussion:
    https://postgr.es/m/20181005172955.wyjb4fzcdzqtaxjq@alap3.anarazel.de
    https://postgr.es/m/CAEepm=1nff0x=7i3YQO16jLA2qw-F9O39YmUew4oq-xcBQBs0g@mail.gmail.com
This commit is contained in:
Andres Freund
2018-11-09 20:54:40 -08:00
parent 69ee2ff930
commit 450c7defa6
2 changed files with 108 additions and 99 deletions
Download Patch File Download Diff File Expand all files Collapse all files

155
src/backend/storage/ipc/procarray.c
View File

@@ -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);