Feedback-ID: ia2694551:Fastmail
Date: Sun, 3 Mar 2024 15:29:22 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: pgsql-hackers@lists.postgresql.org
Cc: Dilip Kumar <dilipbalaut@gmail.com>
Subject: Re: pgsql: Improve performance of subsystems on top of SLRU
Message-ID: <202403031429.zvfh7chvuk24@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="rb5vb6debo3bx6lt"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <E1rfMSf-001FbI-Co@gemulon.postgresql.org>
Archived-At: 
 <https://www.postgresql.org/message-id/202403031429.zvfh7chvuk24%40alvherre.pgsql>
Precedence: bulk


--rb5vb6debo3bx6lt
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

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/

--rb5vb6debo3bx6lt
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="0001-rework-locking-code-in-GetMultiXactIdMembers.patch"

From a82fa380c7ff5a468ca920f4c06e626946bc8ecf Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@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


--rb5vb6debo3bx6lt--