Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit4aec498

Browse files
committed
Assorted code review for recent ProcArrayLock patch.
Post-commit review by Andres Freund discovered a couple of concurrencybugs in the original patch: specifically, if the leader cleared afollower's XID before it reached PGSemaphoreLock, the semaphore would beleft in the wrong state; and if another process did PGSemaphoreUnlockfor some unrelated reason, we might resume execution before the factthat our XID was cleared was globally visible.Also, improve the wording of some comments, rename nextClearXidElemto firstClearXidElem in PROC_HDR for clarity, and drop some volatilequalifiers that aren't necessary.Amit Kapila, reviewed and slightly revised by me.
1 parent1ea5ce5 commit4aec498

File tree

3 files changed

+34
-15
lines changed

3 files changed

+34
-15
lines changed

‎src/backend/storage/ipc/procarray.c

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -470,10 +470,10 @@ ProcArrayEndTransactionInternal(PGPROC *proc, PGXACT *pgxact,
470470
* commit time, add ourselves to a list of processes that need their XIDs
471471
* cleared. The first process to add itself to the list will acquire
472472
* ProcArrayLock in exclusive mode and perform ProcArrayEndTransactionInternal
473-
* on behalf of all group members. This avoids a great deal ofcontext
474-
*switchingwhen many processes are trying to commit at once, since the lock
475-
*only needs to be handed from the last share-locker to oneprocess waiting
476-
*for the exclusive lock, rather thantoeach one in turn.
473+
* on behalf of all group members. This avoids a great deal ofcontention
474+
*around ProcArrayLockwhen many processes are trying to commit at once,
475+
*since the lock need not be repeatedly handed off from onecommitting
476+
*processtothe next.
477477
*/
478478
staticvoid
479479
ProcArrayGroupClearXid(PGPROC*proc,TransactionIdlatestXid)
@@ -487,28 +487,39 @@ ProcArrayGroupClearXid(PGPROC *proc, TransactionId latestXid)
487487
Assert(TransactionIdIsValid(allPgXact[proc->pgprocno].xid));
488488

489489
/* Add ourselves to the list of processes needing a group XID clear. */
490+
proc->clearXid= true;
490491
proc->backendLatestXid=latestXid;
491492
while (true)
492493
{
493-
nextidx=pg_atomic_read_u32(&procglobal->nextClearXidElem);
494+
nextidx=pg_atomic_read_u32(&procglobal->firstClearXidElem);
494495
pg_atomic_write_u32(&proc->nextClearXidElem,nextidx);
495496

496-
if (pg_atomic_compare_exchange_u32(&procglobal->nextClearXidElem,
497+
if (pg_atomic_compare_exchange_u32(&procglobal->firstClearXidElem,
497498
&nextidx,
498499
(uint32)proc->pgprocno))
499500
break;
500501
}
501502

502-
/* If the list was not empty, the leader will clear our XID. */
503+
/*
504+
* If the list was not empty, the leader will clear our XID. It is
505+
* impossible to have followers without a leader because the first process
506+
* that has added itself to the list will always have nextidx as
507+
* INVALID_PGPROCNO.
508+
*/
503509
if (nextidx!=INVALID_PGPROCNO)
504510
{
505511
/* Sleep until the leader clears our XID. */
506-
while (pg_atomic_read_u32(&proc->nextClearXidElem)!=INVALID_PGPROCNO)
512+
for (;;)
507513
{
508-
extraWaits++;
514+
/* acts as a read barrier */
509515
PGSemaphoreLock(&proc->sem);
516+
if (!proc->clearXid)
517+
break;
518+
extraWaits++;
510519
}
511520

521+
Assert(pg_atomic_read_u32(&proc->nextClearXidElem)==INVALID_PGPROCNO);
522+
512523
/* Fix semaphore count for any absorbed wakeups */
513524
while (extraWaits-->0)
514525
PGSemaphoreUnlock(&proc->sem);
@@ -520,12 +531,13 @@ ProcArrayGroupClearXid(PGPROC *proc, TransactionId latestXid)
520531

521532
/*
522533
* Now that we've got the lock, clear the list of processes waiting for
523-
* group XID clearing, saving a pointer to the head of the list.
534+
* group XID clearing, saving a pointer to the head of the list. Trying
535+
* to pop elements one at a time could lead to an ABA problem.
524536
*/
525537
while (true)
526538
{
527-
nextidx=pg_atomic_read_u32(&procglobal->nextClearXidElem);
528-
if (pg_atomic_compare_exchange_u32(&procglobal->nextClearXidElem,
539+
nextidx=pg_atomic_read_u32(&procglobal->firstClearXidElem);
540+
if (pg_atomic_compare_exchange_u32(&procglobal->firstClearXidElem,
529541
&nextidx,
530542
INVALID_PGPROCNO))
531543
break;
@@ -563,6 +575,11 @@ ProcArrayGroupClearXid(PGPROC *proc, TransactionId latestXid)
563575
wakeidx=pg_atomic_read_u32(&proc->nextClearXidElem);
564576
pg_atomic_write_u32(&proc->nextClearXidElem,INVALID_PGPROCNO);
565577

578+
/* ensure all previous writes are visible before follower continues. */
579+
pg_write_barrier();
580+
581+
proc->clearXid= false;
582+
566583
if (proc!=MyProc)
567584
PGSemaphoreUnlock(&proc->sem);
568585
}

‎src/backend/storage/lmgr/proc.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ InitProcGlobal(void)
181181
ProcGlobal->startupBufferPinWaitBufId=-1;
182182
ProcGlobal->walwriterLatch=NULL;
183183
ProcGlobal->checkpointerLatch=NULL;
184-
pg_atomic_init_u32(&ProcGlobal->nextClearXidElem,INVALID_PGPROCNO);
184+
pg_atomic_init_u32(&ProcGlobal->firstClearXidElem,INVALID_PGPROCNO);
185185

186186
/*
187187
* Create and initialize all the PGPROC structures we'll need. There are
@@ -395,6 +395,7 @@ InitProcess(void)
395395
SHMQueueElemInit(&(MyProc->syncRepLinks));
396396

397397
/* Initialize fields for group XID clearing. */
398+
MyProc->clearXid= false;
398399
MyProc->backendLatestXid=InvalidTransactionId;
399400
pg_atomic_init_u32(&MyProc->nextClearXidElem,INVALID_PGPROCNO);
400401

‎src/include/storage/proc.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,8 @@ struct PGPROC
142142
structXidCachesubxids;/* cache for subtransaction XIDs */
143143

144144
/* Support for group XID clearing. */
145-
volatilepg_atomic_uint32nextClearXidElem;
145+
boolclearXid;
146+
pg_atomic_uint32nextClearXidElem;
146147
TransactionIdbackendLatestXid;
147148

148149
/* Per-backend LWLock. Protects fields below. */
@@ -207,7 +208,7 @@ typedef struct PROC_HDR
207208
/* Head of list of bgworker free PGPROC structures */
208209
PGPROC*bgworkerFreeProcs;
209210
/* First pgproc waiting for group XID clear */
210-
volatilepg_atomic_uint32nextClearXidElem;
211+
pg_atomic_uint32firstClearXidElem;
211212
/* WALWriter process's latch */
212213
Latch*walwriterLatch;
213214
/* Checkpointer process's latch */

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp