agora inbox for [email protected]
help / color / mirror / Atom feedFrom: Andres Freund <[email protected]>
Subject: [PATCH v4 5/6] bufmgr: fewer calls to BufferDescriptorGetContentLock
Date: Mon, 30 Jun 2025 16:54:39 -0400
We're planning to merge buffer content locks into BufferDesc.state. To reduce
the size of that patch, centralize BufferDescriptorGetContentLock().
The biggest part of the change is in assertions, by introducing
BufferIsLockedByMe[InMode]() (and removing BufferIsExclusiveLocked()). This
seems like an improvement even without aforementioned plans.
Additionally replace some direct calls to LWLockAcquire() with calls to
LockBuffer().
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
src/include/storage/bufmgr.h | 3 +-
src/backend/access/heap/visibilitymap.c | 3 +-
src/backend/access/transam/xloginsert.c | 3 +-
src/backend/storage/buffer/bufmgr.c | 70 +++++++++++++++++++------
4 files changed, 61 insertions(+), 18 deletions(-)
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index 47360a3d3d8..3f37b294af6 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -230,7 +230,8 @@ extern void WaitReadBuffers(ReadBuffersOperation *operation);
extern void ReleaseBuffer(Buffer buffer);
extern void UnlockReleaseBuffer(Buffer buffer);
-extern bool BufferIsExclusiveLocked(Buffer buffer);
+extern bool BufferIsLockedByMe(Buffer buffer);
+extern bool BufferIsLockedByMeInMode(Buffer buffer, int mode);
extern bool BufferIsDirty(Buffer buffer);
extern void MarkBufferDirty(Buffer buffer);
extern void IncrBufferRefCount(Buffer buffer);
diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
index 7306c16f05c..0414ce1945c 100644
--- a/src/backend/access/heap/visibilitymap.c
+++ b/src/backend/access/heap/visibilitymap.c
@@ -270,7 +270,8 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf,
if (BufferIsValid(heapBuf) && BufferGetBlockNumber(heapBuf) != heapBlk)
elog(ERROR, "wrong heap buffer passed to visibilitymap_set");
- Assert(!BufferIsValid(heapBuf) || BufferIsExclusiveLocked(heapBuf));
+ Assert(!BufferIsValid(heapBuf) ||
+ BufferIsLockedByMeInMode(heapBuf, BUFFER_LOCK_EXCLUSIVE));
/* Check that we have the right VM page pinned */
if (!BufferIsValid(vmBuf) || BufferGetBlockNumber(vmBuf) != mapBlock)
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index c7571429e8e..496e0fa4ac6 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -258,7 +258,8 @@ XLogRegisterBuffer(uint8 block_id, Buffer buffer, uint8 flags)
*/
#ifdef USE_ASSERT_CHECKING
if (!(flags & REGBUF_NO_CHANGE))
- Assert(BufferIsExclusiveLocked(buffer) && BufferIsDirty(buffer));
+ Assert(BufferIsLockedByMeInMode(buffer, BUFFER_LOCK_EXCLUSIVE) &&
+ BufferIsDirty(buffer));
#endif
if (block_id >= max_registered_block_id)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 594a8e5d512..350b35362ce 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1065,7 +1065,7 @@ ZeroAndLockBuffer(Buffer buffer, ReadBufferMode mode, bool already_valid)
* already valid.)
*/
if (!isLocalBuf)
- LWLockAcquire(BufferDescriptorGetContentLock(bufHdr), LW_EXCLUSIVE);
+ LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
/* Set BM_VALID, terminate IO, and wake up any waiters */
if (isLocalBuf)
@@ -2822,7 +2822,7 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
}
if (lock)
- LWLockAcquire(BufferDescriptorGetContentLock(buf_hdr), LW_EXCLUSIVE);
+ LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
TerminateBufferIO(buf_hdr, false, BM_VALID, true, false);
}
@@ -2835,14 +2835,14 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
}
/*
- * BufferIsExclusiveLocked
+ * BufferIsLockedByMe
*
- * Checks if buffer is exclusive-locked.
+ * Checks if this backend has the buffer locked in any mode.
*
* Buffer must be pinned.
*/
bool
-BufferIsExclusiveLocked(Buffer buffer)
+BufferIsLockedByMe(Buffer buffer)
{
BufferDesc *bufHdr;
@@ -2855,9 +2855,49 @@ BufferIsExclusiveLocked(Buffer buffer)
}
else
{
+ bufHdr = GetBufferDescriptor(buffer - 1);
+ return LWLockHeldByMe(BufferDescriptorGetContentLock(bufHdr));
+ }
+}
+
+/*
+ * BufferIsLockedByMeInMode
+ *
+ * Checks if this backend has the buffer locked in the specified mode.
+ *
+ * Buffer must be pinned.
+ */
+bool
+BufferIsLockedByMeInMode(Buffer buffer, int mode)
+{
+ BufferDesc *bufHdr;
+
+ Assert(BufferIsPinned(buffer));
+
+ if (BufferIsLocal(buffer))
+ {
+ /* Content locks are not maintained for local buffers. */
+ return true;
+ }
+ else
+ {
+ LWLockMode lw_mode;
+
+ switch (mode)
+ {
+ case BUFFER_LOCK_EXCLUSIVE:
+ lw_mode = LW_EXCLUSIVE;
+ break;
+ case BUFFER_LOCK_SHARE:
+ lw_mode = LW_SHARED;
+ break;
+ default:
+ pg_unreachable();
+ }
+
bufHdr = GetBufferDescriptor(buffer - 1);
return LWLockHeldByMeInMode(BufferDescriptorGetContentLock(bufHdr),
- LW_EXCLUSIVE);
+ lw_mode);
}
}
@@ -2886,8 +2926,7 @@ BufferIsDirty(Buffer buffer)
else
{
bufHdr = GetBufferDescriptor(buffer - 1);
- Assert(LWLockHeldByMeInMode(BufferDescriptorGetContentLock(bufHdr),
- LW_EXCLUSIVE));
+ Assert(BufferIsLockedByMeInMode(buffer, BUFFER_LOCK_EXCLUSIVE));
}
return pg_atomic_read_u32(&bufHdr->state) & BM_DIRTY;
@@ -2921,8 +2960,7 @@ MarkBufferDirty(Buffer buffer)
bufHdr = GetBufferDescriptor(buffer - 1);
Assert(BufferIsPinned(buffer));
- Assert(LWLockHeldByMeInMode(BufferDescriptorGetContentLock(bufHdr),
- LW_EXCLUSIVE));
+ Assert(BufferIsLockedByMeInMode(buffer, BUFFER_LOCK_EXCLUSIVE));
/*
* NB: We have to wait for the buffer header spinlock to be not held, as
@@ -3258,7 +3296,10 @@ UnpinBufferNoOwner(BufferDesc *buf)
*/
VALGRIND_MAKE_MEM_NOACCESS(BufHdrGetBlock(buf), BLCKSZ);
- /* I'd better not still hold the buffer content lock */
+ /*
+ * I'd better not still hold the buffer content lock. Can't use
+ * BufferIsLockedByMe(), as that asserts the buffer is pinned.
+ */
Assert(!LWLockHeldByMe(BufferDescriptorGetContentLock(buf)));
/* decrement the shared reference count */
@@ -5311,7 +5352,7 @@ FlushOneBuffer(Buffer buffer)
bufHdr = GetBufferDescriptor(buffer - 1);
- Assert(LWLockHeldByMe(BufferDescriptorGetContentLock(bufHdr)));
+ Assert(BufferIsLockedByMe(buffer));
FlushBuffer(bufHdr, NULL, IOOBJECT_RELATION, IOCONTEXT_NORMAL);
}
@@ -5402,7 +5443,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
Assert(GetPrivateRefCount(buffer) > 0);
/* here, either share or exclusive lock is OK */
- Assert(LWLockHeldByMe(BufferDescriptorGetContentLock(bufHdr)));
+ Assert(BufferIsLockedByMe(buffer));
/*
* This routine might get called many times on the same page, if we are
@@ -5894,8 +5935,7 @@ IsBufferCleanupOK(Buffer buffer)
bufHdr = GetBufferDescriptor(buffer - 1);
/* caller must hold exclusive lock on buffer */
- Assert(LWLockHeldByMeInMode(BufferDescriptorGetContentLock(bufHdr),
- LW_EXCLUSIVE));
+ Assert(BufferIsLockedByMeInMode(buffer, BUFFER_LOCK_EXCLUSIVE));
buf_state = LockBufHdr(bufHdr);
--
2.48.1.76.g4e746b1a31.dirty
--al7hvfxknuxztweg
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v4-0006-bufmgr-Introduce-FlushUnlockedBuffer.patch"
view thread (3+ messages)
reply
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: [email protected]
Cc: [email protected]
Subject: Re: [PATCH v4 5/6] bufmgr: fewer calls to BufferDescriptorGetContentLock
In-Reply-To: <no-message-id-28105@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox