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

Commitda8a716

Browse files
committed
Fix latent(?) race condition in LockReleaseAll.
We have for a long time checked the head pointer of each of the backend'sproclock lists and skipped acquiring the corresponding locktable partitionlock if the head pointer was NULL. This was safe enough in the days whenproclock lists were changed only by the owning backend, but it is prettyquestionable now that the fast-path patch added cases where backends addentries to other backends' proclock lists. However, we don't really wishto revert to locking each partition lock every time, because in simpletransactions that would add a lot of useless lock/unlock cycles onalready-heavily-contended LWLocks. Fortunately, the only way that anotherbackend could be modifying our proclock list at this point would be if itwas promoting a formerly fast-path lock of ours; and any such lock must beone that we'd decided not to delete in the previous loop over the locallocktable. So it's okay if we miss seeing it in this loop; we'd just decidenot to delete it again. However, once we've detected a non-empty list,we'd better re-fetch the list head pointer after acquiring the partitionlock. This guards against possibly fetching a corrupt-but-non-null pointerif pointer fetch/store isn't atomic. It's not clear if any practicalarchitectures are like that, but we've never assumed that before and don'twish to start here. In any case, the situation certainly deserves a codecomment.While at it, refactor the partition traversal loop to use a for() constructinstead of a while() loop with goto's.Back-patch, just in case the risk is real and not hypothetical.
1 parentd51a8c5 commitda8a716

File tree

1 file changed

+46
-24
lines changed
  • src/backend/storage/lmgr

1 file changed

+46
-24
lines changed

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

Lines changed: 46 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -2098,19 +2098,39 @@ LockReleaseAll(LOCKMETHODID lockmethodid, bool allLocks)
20982098
{
20992099
LWLockIdpartitionLock=FirstLockMgrLock+partition;
21002100
SHM_QUEUE*procLocks=&(MyProc->myProcLocks[partition]);
2101+
PROCLOCK*nextplock;
21012102

2102-
proclock= (PROCLOCK*)SHMQueueNext(procLocks,procLocks,
2103-
offsetof(PROCLOCK,procLink));
2104-
2105-
if (!proclock)
2103+
/*
2104+
* If the proclock list for this partition is empty, we can skip
2105+
* acquiring the partition lock. This optimization is trickier than
2106+
* it looks, because another backend could be in process of adding
2107+
* something to our proclock list due to promoting one of our
2108+
* fast-path locks. However, any such lock must be one that we
2109+
* decided not to delete above, so it's okay to skip it again now;
2110+
* we'd just decide not to delete it again. We must, however, be
2111+
* careful to re-fetch the list header once we've acquired the
2112+
* partition lock, to be sure we have a valid, up-to-date pointer.
2113+
* (There is probably no significant risk if pointer fetch/store is
2114+
* atomic, but we don't wish to assume that.)
2115+
*
2116+
* XXX This argument assumes that the locallock table correctly
2117+
* represents all of our fast-path locks. While allLocks mode
2118+
* guarantees to clean up all of our normal locks regardless of the
2119+
* locallock situation, we lose that guarantee for fast-path locks.
2120+
* This is not ideal.
2121+
*/
2122+
if (SHMQueueNext(procLocks,procLocks,
2123+
offsetof(PROCLOCK,procLink))==NULL)
21062124
continue;/* needn't examine this partition */
21072125

21082126
LWLockAcquire(partitionLock,LW_EXCLUSIVE);
21092127

2110-
while (proclock)
2128+
for (proclock= (PROCLOCK*)SHMQueueNext(procLocks,procLocks,
2129+
offsetof(PROCLOCK,procLink));
2130+
proclock;
2131+
proclock=nextplock)
21112132
{
21122133
boolwakeupNeeded= false;
2113-
PROCLOCK*nextplock;
21142134

21152135
/* Get link first, since we may unlink/delete this proclock */
21162136
nextplock= (PROCLOCK*)
@@ -2123,7 +2143,7 @@ LockReleaseAll(LOCKMETHODID lockmethodid, bool allLocks)
21232143

21242144
/* Ignore items that are not of the lockmethod to be removed */
21252145
if (LOCK_LOCKMETHOD(*lock)!=lockmethodid)
2126-
gotonext_item;
2146+
continue;
21272147

21282148
/*
21292149
* In allLocks mode, force release of all locks even if locallock
@@ -2139,7 +2159,7 @@ LockReleaseAll(LOCKMETHODID lockmethodid, bool allLocks)
21392159
* holdMask == 0 and are therefore recyclable
21402160
*/
21412161
if (proclock->releaseMask==0&&proclock->holdMask!=0)
2142-
gotonext_item;
2162+
continue;
21432163

21442164
PROCLOCK_PRINT("LockReleaseAll",proclock);
21452165
LOCK_PRINT("LockReleaseAll",lock,0);
@@ -2168,9 +2188,6 @@ LockReleaseAll(LOCKMETHODID lockmethodid, bool allLocks)
21682188
lockMethodTable,
21692189
LockTagHashCode(&lock->tag),
21702190
wakeupNeeded);
2171-
2172-
next_item:
2173-
proclock=nextplock;
21742191
}/* loop over PROCLOCKs within this partition */
21752192

21762193
LWLockRelease(partitionLock);
@@ -3142,19 +3159,27 @@ PostPrepare_Locks(TransactionId xid)
31423159
{
31433160
LWLockIdpartitionLock=FirstLockMgrLock+partition;
31443161
SHM_QUEUE*procLocks=&(MyProc->myProcLocks[partition]);
3162+
PROCLOCK*nextplock;
31453163

3146-
proclock= (PROCLOCK*)SHMQueueNext(procLocks,procLocks,
3147-
offsetof(PROCLOCK,procLink));
3148-
3149-
if (!proclock)
3164+
/*
3165+
* If the proclock list for this partition is empty, we can skip
3166+
* acquiring the partition lock. This optimization is safer than the
3167+
* situation in LockReleaseAll, because we got rid of any fast-path
3168+
* locks during AtPrepare_Locks, so there cannot be any case where
3169+
* another backend is adding something to our lists now. For safety,
3170+
* though, we code this the same way as in LockReleaseAll.
3171+
*/
3172+
if (SHMQueueNext(procLocks,procLocks,
3173+
offsetof(PROCLOCK,procLink))==NULL)
31503174
continue;/* needn't examine this partition */
31513175

31523176
LWLockAcquire(partitionLock,LW_EXCLUSIVE);
31533177

3154-
while (proclock)
3178+
for (proclock= (PROCLOCK*)SHMQueueNext(procLocks,procLocks,
3179+
offsetof(PROCLOCK,procLink));
3180+
proclock;
3181+
proclock=nextplock)
31553182
{
3156-
PROCLOCK*nextplock;
3157-
31583183
/* Get link first, since we may unlink/relink this proclock */
31593184
nextplock= (PROCLOCK*)
31603185
SHMQueueNext(procLocks,&proclock->procLink,
@@ -3166,7 +3191,7 @@ PostPrepare_Locks(TransactionId xid)
31663191

31673192
/* Ignore VXID locks */
31683193
if (lock->tag.locktag_type==LOCKTAG_VIRTUALTRANSACTION)
3169-
gotonext_item;
3194+
continue;
31703195

31713196
PROCLOCK_PRINT("PostPrepare_Locks",proclock);
31723197
LOCK_PRINT("PostPrepare_Locks",lock,0);
@@ -3177,7 +3202,7 @@ PostPrepare_Locks(TransactionId xid)
31773202

31783203
/* Ignore it if nothing to release (must be a session lock) */
31793204
if (proclock->releaseMask==0)
3180-
gotonext_item;
3205+
continue;
31813206

31823207
/* Else we should be releasing all locks */
31833208
if (proclock->releaseMask!=proclock->holdMask)
@@ -3219,9 +3244,6 @@ PostPrepare_Locks(TransactionId xid)
32193244
&proclock->procLink);
32203245

32213246
PROCLOCK_PRINT("PostPrepare_Locks: updated",proclock);
3222-
3223-
next_item:
3224-
proclock=nextplock;
32253247
}/* loop over PROCLOCKs within this partition */
32263248

32273249
LWLockRelease(partitionLock);
@@ -3918,7 +3940,7 @@ VirtualXactLockTableInsert(VirtualTransactionId vxid)
39183940
*unblocking waiters.
39193941
*/
39203942
void
3921-
VirtualXactLockTableCleanup()
3943+
VirtualXactLockTableCleanup(void)
39223944
{
39233945
boolfastpath;
39243946
LocalTransactionIdlxid;

0 commit comments

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