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"