From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Tom Lane <tgl@sss.pgh.pa.us>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: pgsql: Improve performance of subsystems on top of SLRU
Date: Mon, 4 Mar 2024 16:17:58 +0100
Message-ID: <202403041517.3a35jw53os65@alvherre.pgsql> (raw)
In-Reply-To: <CAFiTN-sAPL1iCBfUOj1VimVBngO_0HyDNEgShWKL_-6RqbNDLg@mail.gmail.com> <3396247.1709500489@sss.pgh.pa.us>
On 2024-Mar-03, Tom Lane wrote:
> This is certainly simpler, but I notice that it holds the current
> LWLock across the line
>
> ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember));
>
> where the old code did not. Could the palloc take long enough that
> holding the lock is bad?
Hmm, I guess most of the time it shouldn't be much of a problem (if the
length is small so we can palloc without malloc'ing); but it could be in
the worst case. But the fix is simple: just release the lock before.
There's no correctness argument for holding it all the way down. I was
just confused about how the original code worked.
> Also, with this coding the "lock = NULL;" assignment just before
> "goto retry" is a dead store. Not sure if Coverity or other static
> analyzers would whine about that.
Oh, right. I removed that.
On 2024-Mar-04, Dilip Kumar wrote:
> 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,
True. This can be addressed in the same way Tom's first comment is:
just release the lock before entering the second loop, and setting lock
to NULL. This brings the code to a similar state than before, except
that the additional LWLock * variables are in a tighter scope. That's
in 0001.
Now, I had a look at the other users of slru.c and noticed in subtrans.c
that StartupSUBTRANS we have some duplicate code that I think could be
rewritten by making the "while" block test the condition at the end
instead of at the start; changed that in 0002. I'll leave this one for
later, because I want to add some test code for it -- right now it's
pretty much test-uncovered code.
I also looked at slru.c for uses of shared->bank_locks and noticed a
couple that could be made simpler. That's 0003 here.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"If it is not right, do not do it.
If it is not true, do not say it." (Marcus Aurelius, Meditations)
Attachments:download all (3 files, .tar.gz)v2-0001-rework-locking-code-in-GetMultiXactIdMembers.patch (text/x-diff, 4.0K) download
From a6be7cac5de83a36e056147f3e81bea2eb1096bd 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 v2 1/3] rework locking code in GetMultiXactIdMembers
Per Coverity
---
src/backend/access/transam/multixact.c | 53 +++++++++++---------------
1 file changed, 22 insertions(+), 31 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.cindex cd476b94fa..83b578dced 100644--- a/src/backend/access/transam/multixact.c+++ b/src/backend/access/transam/multixact.c@@ -1247,14 +1247,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);
@@ -1361,18 +1359,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];
@@ -1407,17 +1396,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);
}
@@ -1429,8 +1420,7 @@ retry:
if (nextMXOffset == 0)
{
/* Corner case 2: next multixact is still being filled in */
- LWLockRelease(prevlock);- prevlock = NULL;+ LWLockRelease(lock);
CHECK_FOR_INTERRUPTS();
pg_usleep(1000L);
goto retry;
@@ -1439,14 +1429,14 @@ retry:
length = nextMXOffset - offset;
}
- LWLockRelease(prevlock);- prevlock = NULL;+ LWLockRelease(lock);+ lock = 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;
@@ -1459,18 +1449,20 @@ 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;+ if (lock)+ LWLockRelease(lock);+ LWLockAcquire(newlock, LW_EXCLUSIVE);+ lock = newlock;
}
slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, multi);
@@ -1496,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
v2-0002-Rework-redundant-loop-in-subtrans.c.patch (text/x-diff, 2.0K) download
From dfb077ba15399565967d2da4f1d17199bbf142d2 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Mon, 4 Mar 2024 11:49:01 +0100
Subject: [PATCH v2 2/3] Rework redundant loop in subtrans.c
---
src/backend/access/transam/subtrans.c | 28 +++++----------------------
1 file changed, 5 insertions(+), 23 deletions(-)
diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/subtrans.cindex dc9566fb51..1455cdf54a 100644--- a/src/backend/access/transam/subtrans.c+++ b/src/backend/access/transam/subtrans.c@@ -311,7 +311,7 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
FullTransactionId nextXid;
int64 startPage;
int64 endPage;
- LWLock *prevlock;+ LWLock *prevlock = NULL;
LWLock *lock;
/*
@@ -324,19 +324,13 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
nextXid = TransamVariables->nextXid;
endPage = TransactionIdToPage(XidFromFullTransactionId(nextXid));
- prevlock = SimpleLruGetBankLock(SubTransCtl, startPage);- LWLockAcquire(prevlock, LW_EXCLUSIVE);- while (startPage != endPage)+ do
{
lock = SimpleLruGetBankLock(SubTransCtl, startPage);
-- /*- * Check if we need to acquire the lock on the new bank then release- * the lock on the old bank and acquire on the new bank.- */
if (prevlock != lock)
{
- LWLockRelease(prevlock);+ if (prevlock)+ LWLockRelease(prevlock);
LWLockAcquire(lock, LW_EXCLUSIVE);
prevlock = lock;
}
@@ -346,20 +340,8 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
/* must account for wraparound */
if (startPage > TransactionIdToPage(MaxTransactionId))
startPage = 0;
- }+ } while (startPage != endPage);- lock = SimpleLruGetBankLock(SubTransCtl, startPage);-- /*- * Check if we need to acquire the lock on the new bank then release the- * lock on the old bank and acquire on the new bank.- */- if (prevlock != lock)- {- LWLockRelease(prevlock);- LWLockAcquire(lock, LW_EXCLUSIVE);- }- (void) ZeroSUBTRANSPage(startPage);
LWLockRelease(lock);
}
--
2.39.2
v2-0003-Simplify-coding-in-slru.c.patch (text/x-diff, 3.3K) download
From c3a05aae2d8acca3f216abea61a97e6a9326e699 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Mon, 4 Mar 2024 11:48:43 +0100
Subject: [PATCH v2 3/3] Simplify coding in slru.c
---
src/backend/access/transam/slru.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.cindex f774d285b7..6895266bf9 100644--- a/src/backend/access/transam/slru.c+++ b/src/backend/access/transam/slru.c@@ -489,14 +489,14 @@ SimpleLruReadPage(SlruCtl ctl, int64 pageno, bool write_ok,
TransactionId xid)
{
SlruShared shared = ctl->shared;
+ LWLock *banklock = SimpleLruGetBankLock(ctl, pageno);- Assert(LWLockHeldByMeInMode(SimpleLruGetBankLock(ctl, pageno), LW_EXCLUSIVE));+ Assert(LWLockHeldByMeInMode(banklock, LW_EXCLUSIVE));
/* Outer loop handles restart if we must wait for someone else's I/O */
for (;;)
{
int slotno;
- int bankno;
bool ok;
/* See if page already is in memory; if not, pick victim slot */
@@ -539,10 +539,9 @@ SimpleLruReadPage(SlruCtl ctl, int64 pageno, bool write_ok,
/* Acquire per-buffer lock (cannot deadlock, see notes at top) */
LWLockAcquire(&shared->buffer_locks[slotno].lock, LW_EXCLUSIVE);
- bankno = SlotGetBankNumber(slotno);
/* Release bank lock while doing I/O */
- LWLockRelease(&shared->bank_locks[bankno].lock);+ LWLockRelease(banklock);
/* Do the read */
ok = SlruPhysicalReadPage(ctl, pageno, slotno);
@@ -551,7 +550,7 @@ SimpleLruReadPage(SlruCtl ctl, int64 pageno, bool write_ok,
SimpleLruZeroLSNs(ctl, slotno);
/* Re-acquire bank control lock and update page state */
- LWLockAcquire(&shared->bank_locks[bankno].lock, LW_EXCLUSIVE);+ LWLockAcquire(banklock, LW_EXCLUSIVE);
Assert(shared->page_number[slotno] == pageno &&
shared->page_status[slotno] == SLRU_PAGE_READ_IN_PROGRESS &&
@@ -592,12 +591,13 @@ int
SimpleLruReadPage_ReadOnly(SlruCtl ctl, int64 pageno, TransactionId xid)
{
SlruShared shared = ctl->shared;
+ LWLock *banklock = SimpleLruGetBankLock(ctl, pageno);
int bankno = pageno & ctl->bank_mask;
int bankstart = bankno * SLRU_BANK_SIZE;
int bankend = bankstart + SLRU_BANK_SIZE;
/* Try to find the page while holding only shared lock */
- LWLockAcquire(&shared->bank_locks[bankno].lock, LW_SHARED);+ LWLockAcquire(banklock, LW_SHARED);
/* See if page is already in a buffer */
for (int slotno = bankstart; slotno < bankend; slotno++)
@@ -617,8 +617,8 @@ SimpleLruReadPage_ReadOnly(SlruCtl ctl, int64 pageno, TransactionId xid)
}
/* No luck, so switch to normal exclusive lock and do regular read */
- LWLockRelease(&shared->bank_locks[bankno].lock);- LWLockAcquire(&shared->bank_locks[bankno].lock, LW_EXCLUSIVE);+ LWLockRelease(banklock);+ LWLockAcquire(banklock, LW_EXCLUSIVE);
return SimpleLruReadPage(ctl, pageno, true, xid);
}
@@ -1167,7 +1167,7 @@ SlruSelectLRUPage(SlruCtl ctl, int64 pageno)
int bankstart = bankno * SLRU_BANK_SIZE;
int bankend = bankstart + SLRU_BANK_SIZE;
- Assert(LWLockHeldByMe(&shared->bank_locks[bankno].lock));+ Assert(LWLockHeldByMe(SimpleLruGetBankLock(ctl, pageno)));
/* See if page already has a buffer assigned */
for (int slotno = 0; slotno < shared->num_slots; slotno++)
--
2.39.2
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@alvh.no-ip.org, tgl@sss.pgh.pa.us, dilipbalaut@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: pgsql: Improve performance of subsystems on top of SLRU
In-Reply-To: <202403041517.3a35jw53os65@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox