MIME-Version: 1.0
References: <E1rfMSf-001FbI-Co@gemulon.postgresql.org>
 <202403031429.zvfh7chvuk24@alvherre.pgsql>
In-Reply-To: <202403031429.zvfh7chvuk24@alvherre.pgsql>
From: Dilip Kumar <dilipbalaut@gmail.com>
Date: Mon, 4 Mar 2024 11:44:48 +0530
Message-ID: 
 <CAFiTN-sAPL1iCBfUOj1VimVBngO_0HyDNEgShWKL_-6RqbNDLg@mail.gmail.com>
Subject: Re: pgsql: Improve performance of subsystems on top of SLRU
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: pgsql-hackers@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAFiTN-sAPL1iCBfUOj1VimVBngO_0HyDNEgShWKL_-6RqbNDLg%40mail.gmail.com>
Precedence: bulk

On Mon, Mar 4, 2024 at 1:56=E2=80=AFAM Alvaro Herrera <alvherre@alvh.no-ip.=
org> wrote:
>
> 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/multixa=
ct.c: 1375 in GetMultiXactIdMembers()
> 1369             * and acquire the lock of the new bank.
> 1370             */
> 1371            lock =3D SimpleLruGetBankLock(MultiXactOffsetCtl, pageno)=
;
> 1372            if (lock !=3D prevlock)
> 1373            {
> 1374                    if (prevlock !=3D NULL)
> >>>     CID 1592913:  Control flow issues  (DEADCODE)
> >>>     Execution cannot reach this statement: "LWLockRelease(prevlock);"=
.
> 1375                            LWLockRelease(prevlock);
> 1376                    LWLockAcquire(lock, LW_EXCLUSIVE);
> 1377                    prevlock =3D lock;
> 1378            }
> 1379
> 1380            slotno =3D 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.


+ /* Acquire the bank lock for the page we need. */
  lock =3D SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
- if (lock !=3D prevlock)
- {
- if (prevlock !=3D NULL)
- LWLockRelease(prevlock);
- LWLockAcquire(lock, LW_EXCLUSIVE);
- prevlock =3D lock;
- }
+ LWLockAcquire(lock, LW_EXCLUSIVE);

This part is definitely an improvement.

I am not sure about the other changes, I mean that makes the code much
simpler but now we are not releasing the 'MultiXactOffsetCtl' related
bank lock, and later in the following loop, we are comparing that lock
against 'MultiXactMemberCtl' related bank lock. This makes code
simpler because now in the loop we are sure that we are always holding
the lock but I do not like comparing the bank locks for 2 different
SLRUs, although there is no problem as there would not be a common
lock address, anyway, I do not have any strong objection to what you
have done here.

--=20
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com