On 2024-Feb-28, Alvaro Herrera wrote:
> Improve performance of subsystems on top of SLRU
Coverity had the following complaint about this commit:
________________________________________________________________________________________________________
*** CID NNNNNNN: Control flow issues (DEADCODE)
/srv/coverity/git/pgsql-git/postgresql/src/backend/access/transam/multixact.c:
1375 in GetMultiXactIdMembers()
1369 * and acquire the lock of the new bank.
1370 */
1371 lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
1372 if (lock != prevlock)
1373 {
1374 if (prevlock != NULL)
>>> CID 1592913: Control flow issues (DEADCODE)
>>>
>>> Execution cannot reach this statement: "LWLockRelease(prevlock);".
>>>
1375 LWLockRelease(prevlock);
1376 LWLockAcquire(lock, LW_EXCLUSIVE);
1377 prevlock = lock;
1378 }
1379
1380 slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true,
multi);
And I think it's correct that this is somewhat bogus, or at least
confusing: the only way to have control back here on line 1371 after
having executed once is via the "goto retry" line below; and there we
release "prevlock" and set it to NULL beforehand, so it's impossible for
prevlock to be NULL. Looking closer I think this code is all confused,
so I suggest to rework it as shown in the attached patch.
I'll have a look at the other places where we use this "prevlock" coding
pattern tomorrow.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
>From a82fa380c7ff5a468ca920f4c06e626946bc8ecf Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Sun, 3 Mar 2024 15:20:36 +0100
Subject: [PATCH] rework locking code in GetMultiXactIdMembers
Per Coverity
---
src/backend/access/transam/multixact.c | 52 ++++++++++----------------
1 file changed, 20 insertions(+), 32 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 9b81506145..ec446949a9 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1250,14 +1250,12 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
MultiXactOffset offset;
int length;
int truelength;
- int i;
MultiXactId oldestMXact;
MultiXactId nextMXact;
MultiXactId tmpMXact;
MultiXactOffset nextOffset;
MultiXactMember *ptr;
LWLock *lock;
- LWLock *prevlock = NULL;
debug_elog3(DEBUG2, "GetMembers: asked for %u", multi);
@@ -1364,18 +1362,9 @@ retry:
pageno = MultiXactIdToOffsetPage(multi);
entryno = MultiXactIdToOffsetEntry(multi);
- /*
- * If this page falls under a different bank, release the old bank's lock
- * and acquire the lock of the new bank.
- */
+ /* Acquire the bank lock for the page we need. */
lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
- if (lock != prevlock)
- {
- if (prevlock != NULL)
- LWLockRelease(prevlock);
- LWLockAcquire(lock, LW_EXCLUSIVE);
- prevlock = lock;
- }
+ LWLockAcquire(lock, LW_EXCLUSIVE);
slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, multi);
offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno];
@@ -1410,17 +1399,19 @@ retry:
if (pageno != prev_pageno)
{
+ LWLock *newlock;
+
/*
* 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
* acquire the lock of the new bank.
*/
- lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
- if (prevlock != lock)
+ newlock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
+ if (newlock != lock)
{
- LWLockRelease(prevlock);
- LWLockAcquire(lock, LW_EXCLUSIVE);
- prevlock = lock;
+ LWLockRelease(lock);
+ LWLockAcquire(newlock, LW_EXCLUSIVE);
+ lock = newlock;
}
slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, tmpMXact);
}
@@ -1432,8 +1423,8 @@ retry:
if (nextMXOffset == 0)
{
/* Corner case 2: next multixact is still being filled in */
- LWLockRelease(prevlock);
- prevlock = NULL;
+ LWLockRelease(lock);
+ lock = NULL;
CHECK_FOR_INTERRUPTS();
pg_usleep(1000L);
goto retry;
@@ -1442,14 +1433,11 @@ retry:
length = nextMXOffset - offset;
}
- LWLockRelease(prevlock);
- prevlock = NULL;
-
ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember));
truelength = 0;
prev_pageno = -1;
- for (i = 0; i < length; i++, offset++)
+ for (int i = 0; i < length; i++, offset++)
{
TransactionId *xactptr;
uint32 *flagsptr;
@@ -1462,18 +1450,19 @@ retry:
if (pageno != prev_pageno)
{
+ LWLock *newlock;
+
/*
* 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
* acquire the lock of the new bank.
*/
- lock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno);
- if (lock != prevlock)
+ newlock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno);
+ if (newlock != lock)
{
- if (prevlock)
- LWLockRelease(prevlock);
- LWLockAcquire(lock, LW_EXCLUSIVE);
- prevlock = lock;
+ LWLockRelease(lock);
+ LWLockAcquire(newlock, LW_EXCLUSIVE);
+ lock = newlock;
}
slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, multi);
@@ -1499,8 +1488,7 @@ retry:
truelength++;
}
- if (prevlock)
- LWLockRelease(prevlock);
+ LWLockRelease(lock);
/* A multixid with zero members should not happen */
Assert(truelength > 0);
--
2.39.2
