mirrors/postgres
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Rework locking code in GetMultiXactIdMembers

Browse Source 
After commit 53c2a97a9266, the code flow around the "retry" goto label
in GetMultiXactIdMembers was confused about what was possible: we never
return there with a held lock, so there's no point in testing for one.
This realization lets us simplify the code a bit.  While at it, make the
scope of a couple of local variables in the same function a bit tighter.

Per Coverity.
This commit is contained in:
Alvaro Herrera 2024-03-04 17:48:01 +01:00
parent f9baaf96d3
commit a0b808baef
No known key found for this signature in database
GPG Key ID: 1C20ACB9D5C564AE
1 changed files with 22 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

53
src/backend/access/transam/multixact.c
View File

@ -1247,14 +1247,12 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
MultiXactOffset offset; MultiXactOffset offset;
int length; int length;
int truelength; int truelength;
int i;
MultiXactId oldestMXact; MultiXactId oldestMXact;
MultiXactId nextMXact; MultiXactId nextMXact;
MultiXactId tmpMXact; MultiXactId tmpMXact;
MultiXactOffset nextOffset; MultiXactOffset nextOffset;
MultiXactMember *ptr; MultiXactMember *ptr;
LWLock *lock; LWLock *lock;
LWLock *prevlock = NULL;
debug_elog3(DEBUG2, "GetMembers: asked for %u", multi); debug_elog3(DEBUG2, "GetMembers: asked for %u", multi);
@ -1361,18 +1359,9 @@ retry:
pageno = MultiXactIdToOffsetPage(multi); pageno = MultiXactIdToOffsetPage(multi);
entryno = MultiXactIdToOffsetEntry(multi); entryno = MultiXactIdToOffsetEntry(multi);
/* /* Acquire the bank lock for the page we need. */
* If this page falls under a different bank, release the old bank's lock
* and acquire the lock of the new bank.
*/
lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
if (lock != prevlock) LWLockAcquire(lock, LW_EXCLUSIVE);
{
if (prevlock != NULL)
LWLockRelease(prevlock);
LWLockAcquire(lock, LW_EXCLUSIVE);
prevlock = lock;
}
slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, multi); slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, multi);
offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno]; offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno];
@ -1407,17 +1396,19 @@ retry:
if (pageno != prev_pageno) if (pageno != prev_pageno)
{ {
LWLock *newlock;
/* /*
* Since we're going to access a different SLRU page, if this page * Since we're going to access a different SLRU page, if this page
* falls under a different bank, release the old bank's lock and * falls under a different bank, release the old bank's lock and
* acquire the lock of the new bank. * acquire the lock of the new bank.
*/ */
lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); newlock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
if (prevlock != lock) if (newlock != lock)
{ {
LWLockRelease(prevlock); LWLockRelease(lock);
LWLockAcquire(lock, LW_EXCLUSIVE); LWLockAcquire(newlock, LW_EXCLUSIVE);
prevlock = lock; lock = newlock;
} }
slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, tmpMXact); slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, tmpMXact);
} }
@ -1429,8 +1420,7 @@ retry:
if (nextMXOffset == 0) if (nextMXOffset == 0)
{ {
/* Corner case 2: next multixact is still being filled in */ /* Corner case 2: next multixact is still being filled in */
LWLockRelease(prevlock); LWLockRelease(lock);
prevlock = NULL;
CHECK_FOR_INTERRUPTS(); CHECK_FOR_INTERRUPTS();
pg_usleep(1000L); pg_usleep(1000L);
goto retry; goto retry;
@ -1439,14 +1429,14 @@ retry:
length = nextMXOffset - offset; length = nextMXOffset - offset;
} }
LWLockRelease(prevlock); LWLockRelease(lock);
prevlock = NULL; lock = NULL;
ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember)); ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember));
truelength = 0; truelength = 0;
prev_pageno = -1; prev_pageno = -1;
for (i = 0; i < length; i++, offset++) for (int i = 0; i < length; i++, offset++)
{ {
TransactionId *xactptr; TransactionId *xactptr;
uint32 *flagsptr; uint32 *flagsptr;
@ -1459,18 +1449,20 @@ retry:
if (pageno != prev_pageno) if (pageno != prev_pageno)
{ {
LWLock *newlock;
/* /*
* Since we're going to access a different SLRU page, if this page * Since we're going to access a different SLRU page, if this page
* falls under a different bank, release the old bank's lock and * falls under a different bank, release the old bank's lock and
* acquire the lock of the new bank. * acquire the lock of the new bank.
*/ */
lock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno); newlock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno);
if (lock != prevlock) if (newlock != lock)
{ {
if (prevlock) if (lock)
LWLockRelease(prevlock); LWLockRelease(lock);
LWLockAcquire(lock, LW_EXCLUSIVE); LWLockAcquire(newlock, LW_EXCLUSIVE);
prevlock = lock; lock = newlock;
} }
slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, multi); slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, multi);
@ -1496,8 +1488,7 @@ retry:
truelength++; truelength++;
} }
if (prevlock) LWLockRelease(lock);
LWLockRelease(prevlock);
/* A multixid with zero members should not happen */ /* A multixid with zero members should not happen */
Assert(truelength > 0); Assert(truelength > 0);