From 4aec49899e5782247e134f94ce1c6ee926f88e1c Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Thu, 3 Sep 2015 13:10:53 -0400 Subject: [PATCH] Assorted code review for recent ProcArrayLock patch. Post-commit review by Andres Freund discovered a couple of concurrency bugs in the original patch: specifically, if the leader cleared a follower's XID before it reached PGSemaphoreLock, the semaphore would be left in the wrong state; and if another process did PGSemaphoreUnlock for some unrelated reason, we might resume execution before the fact that our XID was cleared was globally visible. Also, improve the wording of some comments, rename nextClearXidElem to firstClearXidElem in PROC_HDR for clarity, and drop some volatile qualifiers that aren't necessary. Amit Kapila, reviewed and slightly revised by me. --- src/backend/storage/ipc/procarray.c | 41 ++++++++++++++++++++--------- src/backend/storage/lmgr/proc.c | 3 ++- src/include/storage/proc.h | 5 ++-- 3 files changed, 34 insertions(+), 15 deletions(-) diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index cced823de0..b13fe03993 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -470,10 +470,10 @@ ProcArrayEndTransactionInternal(PGPROC *proc, PGXACT *pgxact, * commit time, add ourselves to a list of processes that need their XIDs * cleared. The first process to add itself to the list will acquire * ProcArrayLock in exclusive mode and perform ProcArrayEndTransactionInternal - * on behalf of all group members. This avoids a great deal of context - * switching when many processes are trying to commit at once, since the lock - * only needs to be handed from the last share-locker to one process waiting - * for the exclusive lock, rather than to each one in turn. + * on behalf of all group members. This avoids a great deal of contention + * around ProcArrayLock when many processes are trying to commit at once, + * since the lock need not be repeatedly handed off from one committing + * process to the next. */ static void ProcArrayGroupClearXid(PGPROC *proc, TransactionId latestXid) @@ -487,28 +487,39 @@ ProcArrayGroupClearXid(PGPROC *proc, TransactionId latestXid) Assert(TransactionIdIsValid(allPgXact[proc->pgprocno].xid)); /* Add ourselves to the list of processes needing a group XID clear. */ + proc->clearXid = true; proc->backendLatestXid = latestXid; while (true) { - nextidx = pg_atomic_read_u32(&procglobal->nextClearXidElem); + nextidx = pg_atomic_read_u32(&procglobal->firstClearXidElem); pg_atomic_write_u32(&proc->nextClearXidElem, nextidx); - if (pg_atomic_compare_exchange_u32(&procglobal->nextClearXidElem, + if (pg_atomic_compare_exchange_u32(&procglobal->firstClearXidElem, &nextidx, (uint32) proc->pgprocno)) break; } - /* If the list was not empty, the leader will clear our XID. */ + /* + * If the list was not empty, the leader will clear our XID. It is + * impossible to have followers without a leader because the first process + * that has added itself to the list will always have nextidx as + * INVALID_PGPROCNO. + */ if (nextidx != INVALID_PGPROCNO) { /* Sleep until the leader clears our XID. */ - while (pg_atomic_read_u32(&proc->nextClearXidElem) != INVALID_PGPROCNO) + for (;;) { - extraWaits++; + /* acts as a read barrier */ PGSemaphoreLock(&proc->sem); + if (!proc->clearXid) + break; + extraWaits++; } + Assert(pg_atomic_read_u32(&proc->nextClearXidElem) == INVALID_PGPROCNO); + /* Fix semaphore count for any absorbed wakeups */ while (extraWaits-- > 0) PGSemaphoreUnlock(&proc->sem); @@ -520,12 +531,13 @@ ProcArrayGroupClearXid(PGPROC *proc, TransactionId latestXid) /* * Now that we've got the lock, clear the list of processes waiting for - * group XID clearing, saving a pointer to the head of the list. + * group XID clearing, saving a pointer to the head of the list. Trying + * to pop elements one at a time could lead to an ABA problem. */ while (true) { - nextidx = pg_atomic_read_u32(&procglobal->nextClearXidElem); - if (pg_atomic_compare_exchange_u32(&procglobal->nextClearXidElem, + nextidx = pg_atomic_read_u32(&procglobal->firstClearXidElem); + if (pg_atomic_compare_exchange_u32(&procglobal->firstClearXidElem, &nextidx, INVALID_PGPROCNO)) break; @@ -563,6 +575,11 @@ ProcArrayGroupClearXid(PGPROC *proc, TransactionId latestXid) wakeidx = pg_atomic_read_u32(&proc->nextClearXidElem); pg_atomic_write_u32(&proc->nextClearXidElem, INVALID_PGPROCNO); + /* ensure all previous writes are visible before follower continues. */ + pg_write_barrier(); + + proc->clearXid = false; + if (proc != MyProc) PGSemaphoreUnlock(&proc->sem); } diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index 93f2656afb..2c2535b7b1 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -181,7 +181,7 @@ InitProcGlobal(void) ProcGlobal->startupBufferPinWaitBufId = -1; ProcGlobal->walwriterLatch = NULL; ProcGlobal->checkpointerLatch = NULL; - pg_atomic_init_u32(&ProcGlobal->nextClearXidElem, INVALID_PGPROCNO); + pg_atomic_init_u32(&ProcGlobal->firstClearXidElem, INVALID_PGPROCNO); /* * Create and initialize all the PGPROC structures we'll need. There are @@ -395,6 +395,7 @@ InitProcess(void) SHMQueueElemInit(&(MyProc->syncRepLinks)); /* Initialize fields for group XID clearing. */ + MyProc->clearXid = false; MyProc->backendLatestXid = InvalidTransactionId; pg_atomic_init_u32(&MyProc->nextClearXidElem, INVALID_PGPROCNO); diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h index 421bb587df..3d68017178 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -142,7 +142,8 @@ struct PGPROC struct XidCache subxids; /* cache for subtransaction XIDs */ /* Support for group XID clearing. */ - volatile pg_atomic_uint32 nextClearXidElem; + bool clearXid; + pg_atomic_uint32 nextClearXidElem; TransactionId backendLatestXid; /* Per-backend LWLock. Protects fields below. */ @@ -207,7 +208,7 @@ typedef struct PROC_HDR /* Head of list of bgworker free PGPROC structures */ PGPROC *bgworkerFreeProcs; /* First pgproc waiting for group XID clear */ - volatile pg_atomic_uint32 nextClearXidElem; + pg_atomic_uint32 firstClearXidElem; /* WALWriter process's latch */ Latch *walwriterLatch; /* Checkpointer process's latch */