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)
>From a6be7cac5de83a36e056147f3e81bea2eb1096bd 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 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.c
index 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
>From dfb077ba15399565967d2da4f1d17199bbf142d2 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@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.c
index 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
>From c3a05aae2d8acca3f216abea61a97e6a9326e699 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@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.c
index 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
