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

Commit0fcc3c2

Browse files
committed
Repair a low-probability race condition identified by Qingqing Zhou.
If a process abandons a wait in LockBufferForCleanup (in practice,only happens if someone cancels a VACUUM) just before someone elsesends it a signal indicating the buffer is available, it was possiblefor the wakeup to remain in the process' semaphore, causing misbehaviornext time the process waited for an lmgr lock. Rather than try toprevent the race condition directly, it seems best to make the lockmanager robust against leftover wakeups, by having it repeat waitingon the semaphore if the lock has not actually been granted or deniedyet.
1 parentcc39aca commit0fcc3c2

File tree

5 files changed

+44
-53
lines changed

5 files changed

+44
-53
lines changed

‎src/backend/storage/buffer/bufmgr.c‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/storage/buffer/bufmgr.c,v 1.206 2006/03/31 23:32:06 tgl Exp $
11+
* $PostgreSQL: pgsql/src/backend/storage/buffer/bufmgr.c,v 1.207 2006/04/14 03:38:55 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -1726,8 +1726,6 @@ UnlockBuffers(void)
17261726

17271727
if (buf)
17281728
{
1729-
HOLD_INTERRUPTS();/* don't want to die() partway through... */
1730-
17311729
LockBufHdr(buf);
17321730

17331731
/*
@@ -1740,11 +1738,7 @@ UnlockBuffers(void)
17401738

17411739
UnlockBufHdr(buf);
17421740

1743-
ProcCancelWaitForSignal();
1744-
17451741
PinCountWaitBuf=NULL;
1746-
1747-
RESUME_INTERRUPTS();
17481742
}
17491743
}
17501744

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

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/storage/lmgr/lock.c,v 1.163 2006/03/05 15:58:38 momjian Exp $
11+
* $PostgreSQL: pgsql/src/backend/storage/lmgr/lock.c,v 1.164 2006/04/14 03:38:55 tgl Exp $
1212
*
1313
* NOTES
1414
* A lock table is a shared memory hash table. When
@@ -1116,10 +1116,12 @@ WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner)
11161116
}
11171117

11181118
/*
1119-
* Remove a proc from the wait-queue it is on
1120-
* (caller must know it is on one).
1119+
* Remove a proc from the wait-queue it is on (caller must know it is on one).
1120+
* This is only used when the proc has failed to get the lock, so we set its
1121+
* waitStatus to STATUS_ERROR.
11211122
*
1122-
* Appropriate partition lock must be held by caller.
1123+
* Appropriate partition lock must be held by caller. Also, caller is
1124+
* responsible for signaling the proc if needed.
11231125
*
11241126
* NB: this does not clean up any locallock object that may exist for the lock.
11251127
*/
@@ -1132,6 +1134,7 @@ RemoveFromWaitQueue(PGPROC *proc, int partition)
11321134
LOCKMETHODIDlockmethodid=LOCK_LOCKMETHOD(*waitLock);
11331135

11341136
/* Make sure proc is waiting */
1137+
Assert(proc->waitStatus==STATUS_WAITING);
11351138
Assert(proc->links.next!=INVALID_OFFSET);
11361139
Assert(waitLock);
11371140
Assert(waitLock->waitProcs.size>0);
@@ -1151,9 +1154,10 @@ RemoveFromWaitQueue(PGPROC *proc, int partition)
11511154
if (waitLock->granted[lockmode]==waitLock->requested[lockmode])
11521155
waitLock->waitMask &=LOCKBIT_OFF(lockmode);
11531156

1154-
/* Clean up the proc's own state */
1157+
/* Clean up the proc's own state, and pass it the ok/fail signal */
11551158
proc->waitLock=NULL;
11561159
proc->waitProcLock=NULL;
1160+
proc->waitStatus=STATUS_ERROR;
11571161

11581162
/*
11591163
* Delete the proclock immediately if it represents no already-held locks.

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

Lines changed: 30 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/storage/lmgr/proc.c,v 1.173 2006/03/05 15:58:39 momjian Exp $
11+
* $PostgreSQL: pgsql/src/backend/storage/lmgr/proc.c,v 1.174 2006/04/14 03:38:55 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -267,7 +267,8 @@ InitProcess(void)
267267

268268
/*
269269
* We might be reusing a semaphore that belonged to a failed process. So
270-
* be careful and reinitialize its value here.
270+
* be careful and reinitialize its value here. (This is not strictly
271+
* necessary anymore, but seems like a good idea for cleanliness.)
271272
*/
272273
PGSemaphoreReset(&MyProc->sem);
273274

@@ -397,7 +398,8 @@ InitDummyProcess(void)
397398

398399
/*
399400
* We might be reusing a semaphore that belonged to a failed process. So
400-
* be careful and reinitialize its value here.
401+
* be careful and reinitialize its value here. (This is not strictly
402+
* necessary anymore, but seems like a good idea for cleanliness.)
401403
*/
402404
PGSemaphoreReset(&MyProc->sem);
403405

@@ -465,7 +467,6 @@ LockWaitCancel(void)
465467
if (MyProc->links.next!=INVALID_OFFSET)
466468
{
467469
/* We could not have been granted the lock yet */
468-
Assert(MyProc->waitStatus==STATUS_ERROR);
469470
RemoveFromWaitQueue(MyProc,lockAwaited->partition);
470471
}
471472
else
@@ -485,15 +486,14 @@ LockWaitCancel(void)
485486
LWLockRelease(partitionLock);
486487

487488
/*
488-
*Reset the proc wait semaphoretozero. This is necessary in the
489-
*scenario where someone else granted us the lock we wanted before we
490-
*were able to remove ourselves from the wait-list. The semaphore will
491-
*have been bumped to 1 by the would-be grantor, and since we are no
492-
*longer going towait on the sema, we have to force it back to zero.
493-
*Otherwise, our next attempt to wait for a lock will fall through
494-
*prematurely.
489+
*We used to do PGSemaphoreReset() heretoensure that our proc's wait
490+
*semaphore is reset to zero. This prevented a leftover wakeup signal
491+
*from remaining in the semaphore if someone else had granted us the
492+
*lock we wanted before we were able to remove ourselves from the
493+
* wait-list. However, now that ProcSleep loops until waitStatus changes,
494+
*a leftover wakeup signal isn't harmful, and it seems not worth
495+
*expending cycles to get rid of a signal that most likely isn't there.
495496
*/
496-
PGSemaphoreReset(&MyProc->sem);
497497

498498
/*
499499
* Return true even if we were kicked off the lock before we were able to
@@ -767,7 +767,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
767767
MyProc->waitProcLock=proclock;
768768
MyProc->waitLockMode=lockmode;
769769

770-
MyProc->waitStatus=STATUS_ERROR;/* initialize result for error */
770+
MyProc->waitStatus=STATUS_WAITING;
771771

772772
/*
773773
* If we detected deadlock, give up without waiting. This must agree with
@@ -808,9 +808,11 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
808808
/*
809809
* If someone wakes us between LWLockRelease and PGSemaphoreLock,
810810
* PGSemaphoreLock will not block.The wakeup is "saved" by the semaphore
811-
* implementation.Note also that if CheckDeadLock is invoked but does
812-
* not detect a deadlock, PGSemaphoreLock() will continue to wait.There
813-
* used to be a loop here, but it was useless code...
811+
* implementation. While this is normally good, there are cases where
812+
* a saved wakeup might be leftover from a previous operation (for
813+
* example, we aborted ProcWaitForSignal just before someone did
814+
* ProcSendSignal). So, loop to wait again if the waitStatus shows
815+
* we haven't been granted nor denied the lock yet.
814816
*
815817
* We pass interruptOK = true, which eliminates a window in which
816818
* cancel/die interrupts would be held off undesirably. This is a promise
@@ -820,7 +822,9 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
820822
* updating the locallock table, but if we lose control to an error,
821823
* LockWaitCancel will fix that up.
822824
*/
823-
PGSemaphoreLock(&MyProc->sem, true);
825+
do {
826+
PGSemaphoreLock(&MyProc->sem, true);
827+
}while (MyProc->waitStatus==STATUS_WAITING);
824828

825829
/*
826830
* Disable the timer, if it's still running
@@ -865,6 +869,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
865869
* XXX: presently, this code is only used for the "success" case, and only
866870
* works correctly for that case. To clean up in failure case, would need
867871
* to twiddle the lock's request counts too --- see RemoveFromWaitQueue.
872+
* Hence, in practice the waitStatus parameter must be STATUS_OK.
868873
*/
869874
PGPROC*
870875
ProcWakeup(PGPROC*proc,intwaitStatus)
@@ -875,6 +880,7 @@ ProcWakeup(PGPROC *proc, int waitStatus)
875880
if (proc->links.prev==INVALID_OFFSET||
876881
proc->links.next==INVALID_OFFSET)
877882
returnNULL;
883+
Assert(proc->waitStatus==STATUS_WAITING);
878884

879885
/* Save next process before we zap the list link */
880886
retProc= (PGPROC*)MAKE_PTR(proc->links.next);
@@ -1014,16 +1020,13 @@ CheckDeadLock(void)
10141020
* efficiently by relying on lockAwaited, but use this coding to preserve
10151021
* the flexibility to kill some other transaction than the one detecting
10161022
* the deadlock.)
1023+
*
1024+
* RemoveFromWaitQueue sets MyProc->waitStatus to STATUS_ERROR, so
1025+
* ProcSleep will report an error after we return from the signal handler.
10171026
*/
10181027
Assert(MyProc->waitLock!=NULL);
10191028
RemoveFromWaitQueue(MyProc,LockTagToPartition(&(MyProc->waitLock->tag)));
10201029

1021-
/*
1022-
* Set MyProc->waitStatus to STATUS_ERROR so that ProcSleep will report an
1023-
* error after we return from the signal handler.
1024-
*/
1025-
MyProc->waitStatus=STATUS_ERROR;
1026-
10271030
/*
10281031
* Unlock my semaphore so that the interrupted ProcSleep() call can
10291032
* finish.
@@ -1058,27 +1061,17 @@ CheckDeadLock(void)
10581061
* This can share the semaphore normally used for waiting for locks,
10591062
* since a backend could never be waiting for a lock and a signal at
10601063
* the same time. As with locks, it's OK if the signal arrives just
1061-
* before we actually reach the waiting state.
1064+
* before we actually reach the waiting state. Also as with locks,
1065+
* it's necessary that the caller be robust against bogus wakeups:
1066+
* always check that the desired state has occurred, and wait again
1067+
* if not. This copes with possible "leftover" wakeups.
10621068
*/
10631069
void
10641070
ProcWaitForSignal(void)
10651071
{
10661072
PGSemaphoreLock(&MyProc->sem, true);
10671073
}
10681074

1069-
/*
1070-
* ProcCancelWaitForSignal - clean up an aborted wait for signal
1071-
*
1072-
* We need this in case the signal arrived after we aborted waiting,
1073-
* or if it arrived but we never reached ProcWaitForSignal() at all.
1074-
* Caller should call this after resetting the signal request status.
1075-
*/
1076-
void
1077-
ProcCancelWaitForSignal(void)
1078-
{
1079-
PGSemaphoreReset(&MyProc->sem);
1080-
}
1081-
10821075
/*
10831076
* ProcSendSignal - send a signal to a backend identified by PID
10841077
*/

‎src/include/c.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
* Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
1313
* Portions Copyright (c) 1994, Regents of the University of California
1414
*
15-
* $PostgreSQL: pgsql/src/include/c.h,v 1.199 2006/03/05 15:58:52 momjian Exp $
15+
* $PostgreSQL: pgsql/src/include/c.h,v 1.200 2006/04/14 03:38:56 tgl Exp $
1616
*
1717
*-------------------------------------------------------------------------
1818
*/
@@ -710,6 +710,7 @@ typedef NameData *Name;
710710
#defineSTATUS_ERROR(-1)
711711
#defineSTATUS_EOF(-2)
712712
#defineSTATUS_FOUND(1)
713+
#defineSTATUS_WAITING(2)
713714

714715

715716
/* ----------------------------------------------------------------

‎src/include/storage/proc.h‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
88
* Portions Copyright (c) 1994, Regents of the University of California
99
*
10-
* $PostgreSQL: pgsql/src/include/storage/proc.h,v 1.87 2006/03/05 15:59:00 momjian Exp $
10+
* $PostgreSQL: pgsql/src/include/storage/proc.h,v 1.88 2006/04/14 03:38:56 tgl Exp $
1111
*
1212
*-------------------------------------------------------------------------
1313
*/
@@ -61,7 +61,7 @@ struct PGPROC
6161
SHM_QUEUElinks;/* list link if process is in a list */
6262

6363
PGSemaphoreDatasem;/* ONE semaphore to sleep on */
64-
intwaitStatus;/* STATUS_OK or STATUS_ERROR after wakeup */
64+
intwaitStatus;/*STATUS_WAITING,STATUS_OK or STATUS_ERROR */
6565

6666
TransactionIdxid;/* transaction currently being executed by
6767
* this proc */
@@ -147,7 +147,6 @@ extern void ProcLockWakeup(LockMethod lockMethodTable, LOCK *lock);
147147
externboolLockWaitCancel(void);
148148

149149
externvoidProcWaitForSignal(void);
150-
externvoidProcCancelWaitForSignal(void);
151150
externvoidProcSendSignal(intpid);
152151

153152
externboolenable_sig_alarm(intdelayms,boolis_statement_timeout);

0 commit comments

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