agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Andres Freund <andres@anarazel.de>
To: Matthias van de Meent <boekewurm+postgres@gmail.com>
Cc: pgsql-hackers@postgresql.org, Melanie Plageman <melanieplageman@gmail.com>
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Noah Misch <noah@leadboat.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Michael Paquier <michael.paquier@gmail.com>
Subject: Re: Buffer locking is special (hints, checksums, AIO writes)
Date: Wed, 19 Nov 2025 21:47:49 -0500
Message-ID: <6rgb2nvhyvnszz4ul3wfzlf5rheb2kkwrglthnna7qhe24onwr@vw27225tkyar> (raw)
In-Reply-To: <3w7v3w6a57jnssokap4k7thoekig72flnyhd4wp3yftzdd7lm7@f6lpcfen6hr7>
References: <fvfmkr5kk4nyex56ejgxj3uzi63isfxovp2biecb4bspbjrze7@az2pljabhnff>
<yivb2evcrj7fna5ymuunw3g5u5xxttwjbjxaa4ofkfkviystjv@4dfylftqxyxh>
<6kmid26do57ykqfpvq6iieniy4djsymhrypkjccazq5g4bbe6a@2y6owwv7qpex>
<CAEze2WgGe8vjj3jiWqUugWuwLJ9cLryaGrnASjm-yJ=tEALX2A@mail.gmail.com>
<pmto7djq64mei53p7r5smfync2waittilhbuzc7j7lpflf2b3y@laz7r76y5pux>
<CAEze2WjeK9CY003S4dmCugv_H4tz9AaXgnqW+wTc=BaPDg+2xg@mail.gmail.com>
<3je3ahgf7rrmmurxo6hnlhg5d3ffwfrtjwjxd6jm5srlv5iebp@vxqk5qtgmowr>
<3w7v3w6a57jnssokap4k7thoekig72flnyhd4wp3yftzdd7lm7@f6lpcfen6hr7>
Hi,
On 2025-10-09 17:16:49 -0400, Andres Freund wrote:
> On 2025-10-09 16:35:44 -0400, Andres Freund wrote:
> > I pushed a few commits from this patchset after Matthias' review
> > (thanks!). Unfortunately in 5e899859287 I missed that the valgrind annotations
> > would not be done anymore for the buffers returned by
> > StrategyGetBuffer(). Which turned skink red.
> >
> > The attached 0001 patch centralizes the valgrind initialization in
> > TrackNewBufferPin(), which 5e899859287 had added. The nice side effect of that
> > is that there are fewer VALGRIND_MAKE_MEM_DEFINED() calls than before. The
> > naming isn't the perfect match, but it seems fine to me.
>
> Forgot to say: I'll push this patch soon, to get skink back to green. Unless
> somebody says something. We can adjust this later, if the comment and/or
> placement of VALGRIND_MAKE_MEM_DEFINED() isn't to everyones liking.
I have pushed that fix as well as the subsequent buffer header locking changes
a while ago.
Attached is a patchset that actually implements the buffer content locks in
bufmgr.c. This isn't that close to a committable shape yet, but it seemed
useful to get it out there. The first few patches seem closer, so it'll also
be useful to narrow this down.
0001: A straight-up bugfix in lwlock.c - albeit for a bug that seems currently
effectively harmless.
0002: Not really required, but seems like an improvement to me
0003: A prerequisite to 0004, pretty boring itself
0004: Use 64bit atomics for BufferDesc.state - at this point nothing uses the
additional bits yet, though. Some annoying reformatting required to avoid
long lines.
0005: There already was a wait event class for BUFFERPIN. It seems better to
make that more general than to implement them separately.
0006+0007: This is preparatory work for 0008, but also worthwhile on its
own. The private refcount stuff does show up in profiles. The reason it's
related is that without these changes the added information in 0008 makes that
worse.
0008: The main change. Implements buffer content locking independently from
lwlock.c. There's obviously a lot of similarity between lwlock.c code and
this, but I've not found a good way to reduce the duplication without giving
up too much. This patch does immediately introduce share-exclusive as a new
lock level, mostly because it was too painful to do separately.
0009+0010+0011: Preparatory work for 0012.
0012: One of the main goals of this patchset - use the new share-exclusive
lock level to only allow hint bits to be set while no IO is going on.
0013: Prototype of making UnlockReleaseBuffer() faster and of using it more
widely in nbtree.c
0014: Now that hint bits can't be done while IO is going on, we don't need to
copy pages anymore. This needs a fair bit more work, as denoted by the FIXMEs
in the code.
I've tried to add detail to the more important commit messages, at least until
0012.
I want to again emphasize that the important commits (i.e. 0008, 0012, 0014)
aren't close to being mergeable. But I think they're in a stage that they
could benefit from "lenient" high-level review.
Greetings,
Andres Freund
Attachments:
[text/x-diff] v6-0001-lwlock-Fix-currently-harmless-bug-in-LWLockWakeup.patch (1.5K, ../6rgb2nvhyvnszz4ul3wfzlf5rheb2kkwrglthnna7qhe24onwr@vw27225tkyar/2-v6-0001-lwlock-Fix-currently-harmless-bug-in-LWLockWakeup.patch)
download | inline diff:
From cf5f78299faf99d42c31acc795617fc2b9046844 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Fri, 7 Nov 2025 16:47:47 -0500
Subject: [PATCH v6 01/14] lwlock: Fix, currently harmless, bug in
LWLockWakeup()
Accidentally the code in LWLockWakeup() checked the list of to-be-woken up
processes to see if LW_FLAG_HAS_WAITERS should be unset. That means that
HAS_WAITERS would not get unset immediately, but only during the next,
unnecessary, call to LWLockWakeup().
Luckily, as the code stands, this is just a small efficiency issue.
However, if there were (as in a patch of mine) a case in which LWLockWakeup()
would not find any backend to wake, despite the wait list not being empty,
we'd wrongly unset LW_FLAG_HAS_WAITERS, leading to potentially hanging.
Discussion: https://postgr.es/m/fvfmkr5kk4nyex56ejgxj3uzi63isfxovp2biecb4bspbjrze7@az2pljabhnff
---
src/backend/storage/lmgr/lwlock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index b017880f5e4..255cfa8fa95 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -998,7 +998,7 @@ LWLockWakeup(LWLock *lock)
else
desired_state &= ~LW_FLAG_RELEASE_OK;
- if (proclist_is_empty(&wakeup))
+ if (proclist_is_empty(&lock->waiters))
desired_state &= ~LW_FLAG_HAS_WAITERS;
desired_state &= ~LW_FLAG_LOCKED; /* release lock */
--
2.48.1.76.g4e746b1a31.dirty
[text/x-diff] v6-0002-bufmgr-Turn-BUFFER_LOCK_-into-an-enum.patch (3.1K, ../6rgb2nvhyvnszz4ul3wfzlf5rheb2kkwrglthnna7qhe24onwr@vw27225tkyar/3-v6-0002-bufmgr-Turn-BUFFER_LOCK_-into-an-enum.patch)
download | inline diff:
From 70d0457b7c9527bf1304921b367a34e49d26fd6e Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Fri, 7 Nov 2025 16:51:52 -0500
Subject: [PATCH v6 02/14] bufmgr: Turn BUFFER_LOCK_* into an enum
This way we will be able to benefit from compiler-warnings for code using a
switch() over all lock modes.
Discussion: https://postgr.es/m/fvfmkr5kk4nyex56ejgxj3uzi63isfxovp2biecb4bspbjrze7@az2pljabhnff
---
src/include/storage/bufmgr.h | 13 ++++++++-----
src/backend/storage/buffer/bufmgr.c | 4 ++--
src/tools/pgindent/typedefs.list | 1 +
3 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index b5f8f3c5d42..5fc3de20abc 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -200,9 +200,12 @@ extern PGDLLIMPORT int32 *LocalRefCount;
/*
* Buffer content lock modes (mode argument for LockBuffer())
*/
-#define BUFFER_LOCK_UNLOCK 0
-#define BUFFER_LOCK_SHARE 1
-#define BUFFER_LOCK_EXCLUSIVE 2
+typedef enum BufferLockMode
+{
+ BUFFER_LOCK_UNLOCK,
+ BUFFER_LOCK_SHARE,
+ BUFFER_LOCK_EXCLUSIVE,
+} BufferLockMode;
/*
@@ -238,7 +241,7 @@ extern void WaitReadBuffers(ReadBuffersOperation *operation);
extern void ReleaseBuffer(Buffer buffer);
extern void UnlockReleaseBuffer(Buffer buffer);
extern bool BufferIsLockedByMe(Buffer buffer);
-extern bool BufferIsLockedByMeInMode(Buffer buffer, int mode);
+extern bool BufferIsLockedByMeInMode(Buffer buffer, BufferLockMode mode);
extern bool BufferIsDirty(Buffer buffer);
extern void MarkBufferDirty(Buffer buffer);
extern void IncrBufferRefCount(Buffer buffer);
@@ -299,7 +302,7 @@ extern void BufferGetTag(Buffer buffer, RelFileLocator *rlocator,
extern void MarkBufferDirtyHint(Buffer buffer, bool buffer_std);
extern void UnlockBuffers(void);
-extern void LockBuffer(Buffer buffer, int mode);
+extern void LockBuffer(Buffer buffer, BufferLockMode mode);
extern bool ConditionalLockBuffer(Buffer buffer);
extern void LockBufferForCleanup(Buffer buffer);
extern bool ConditionalLockBufferForCleanup(Buffer buffer);
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 327ddb7adc8..b682878b1fb 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -2866,7 +2866,7 @@ BufferIsLockedByMe(Buffer buffer)
* Buffer must be pinned.
*/
bool
-BufferIsLockedByMeInMode(Buffer buffer, int mode)
+BufferIsLockedByMeInMode(Buffer buffer, BufferLockMode mode)
{
BufferDesc *bufHdr;
@@ -5601,7 +5601,7 @@ UnlockBuffers(void)
* Acquire or release the content_lock for the buffer.
*/
void
-LockBuffer(Buffer buffer, int mode)
+LockBuffer(Buffer buffer, BufferLockMode mode)
{
BufferDesc *buf;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 57f2a9ccdc5..5769227e41c 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -345,6 +345,7 @@ BufferCachePagesRec
BufferDesc
BufferDescPadded
BufferHeapTupleTableSlot
+BufferLockMode
BufferLookupEnt
BufferManagerRelation
BufferStrategyControl
--
2.48.1.76.g4e746b1a31.dirty
[text/x-diff] v6-0003-Add-pg_atomic_unlocked_write_u64.patch (1.8K, ../6rgb2nvhyvnszz4ul3wfzlf5rheb2kkwrglthnna7qhe24onwr@vw27225tkyar/4-v6-0003-Add-pg_atomic_unlocked_write_u64.patch)
download | inline diff:
From e854fb8fb8737c4cb972f5b307f37a84dde32a4f Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 5 Nov 2025 19:12:37 -0500
Subject: [PATCH v6 03/14] Add pg_atomic_unlocked_write_u64
The 64bit equivalent of pg_atomic_unlocked_write_u32(), to be used in an
upcoming patch converting BufferDesc.state into a 64bit atomic.
---
src/include/port/atomics.h | 10 ++++++++++
src/include/port/atomics/generic.h | 9 +++++++++
2 files changed, 19 insertions(+)
diff --git a/src/include/port/atomics.h b/src/include/port/atomics.h
index 96f1858da97..830ea5c7c52 100644
--- a/src/include/port/atomics.h
+++ b/src/include/port/atomics.h
@@ -488,6 +488,16 @@ pg_atomic_write_u64(volatile pg_atomic_uint64 *ptr, uint64 val)
pg_atomic_write_u64_impl(ptr, val);
}
+static inline void
+pg_atomic_unlocked_write_u64(volatile pg_atomic_uint64 *ptr, uint64 val)
+{
+#ifndef PG_HAVE_ATOMIC_U64_SIMULATION
+ AssertPointerAlignment(ptr, 8);
+#endif
+
+ pg_atomic_unlocked_write_u64_impl(ptr, val);
+}
+
static inline void
pg_atomic_write_membarrier_u64(volatile pg_atomic_uint64 *ptr, uint64 val)
{
diff --git a/src/include/port/atomics/generic.h b/src/include/port/atomics/generic.h
index 6b61a7b5416..00aa152f908 100644
--- a/src/include/port/atomics/generic.h
+++ b/src/include/port/atomics/generic.h
@@ -297,6 +297,15 @@ pg_atomic_write_u64_impl(volatile pg_atomic_uint64 *ptr, uint64 val)
#endif /* PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY && !PG_HAVE_ATOMIC_U64_SIMULATION */
#endif /* PG_HAVE_ATOMIC_WRITE_U64 */
+#ifndef PG_HAVE_ATOMIC_UNLOCKED_WRITE_U64
+#define PG_HAVE_ATOMIC_UNLOCKED_WRITE_U64
+static inline void
+pg_atomic_unlocked_write_u64_impl(volatile pg_atomic_uint64 *ptr, uint64 val)
+{
+ ptr->value = val;
+}
+#endif
+
#ifndef PG_HAVE_ATOMIC_READ_U64
#define PG_HAVE_ATOMIC_READ_U64
--
2.48.1.76.g4e746b1a31.dirty
[text/x-diff] v6-0004-bufmgr-Change-BufferDesc.state-to-be-a-64bit-atom.patch (43.6K, ../6rgb2nvhyvnszz4ul3wfzlf5rheb2kkwrglthnna7qhe24onwr@vw27225tkyar/5-v6-0004-bufmgr-Change-BufferDesc.state-to-be-a-64bit-atom.patch)
download | inline diff:
From 90697d38319c839ce2533dd4425fcce7058b07ca Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 5 Nov 2025 19:22:15 -0500
Subject: [PATCH v6 04/14] bufmgr: Change BufferDesc.state to be a 64bit atomic
This is motivated by wanting to merge buffer content locks into
BufferDesc.state in a future commit, rather than having a separate lwlock (see
commit c75ebc657ff more details). As this change is rather mechanical, it
seems to make sense to split it out into a separate commit, for easier review.
Discussion: https://postgr.es/m/fvfmkr5kk4nyex56ejgxj3uzi63isfxovp2biecb4bspbjrze7@az2pljabhnff
---
src/include/storage/buf_internals.h | 88 ++++++----
src/backend/storage/buffer/buf_init.c | 2 +-
src/backend/storage/buffer/bufmgr.c | 158 +++++++++---------
src/backend/storage/buffer/freelist.c | 24 +--
src/backend/storage/buffer/localbuf.c | 72 ++++----
contrib/pg_buffercache/pg_buffercache_pages.c | 8 +-
src/test/modules/test_aio/test_aio.c | 12 +-
7 files changed, 192 insertions(+), 172 deletions(-)
diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h
index 5400c56a965..28519ad2813 100644
--- a/src/include/storage/buf_internals.h
+++ b/src/include/storage/buf_internals.h
@@ -30,7 +30,7 @@
#include "utils/resowner.h"
/*
- * Buffer state is a single 32-bit variable where following data is combined.
+ * Buffer state is a single 64-bit variable where following data is combined.
*
* - 18 bits refcount
* - 4 bits usage count
@@ -39,6 +39,9 @@
* Combining these values allows to perform some operations without locking
* the buffer header, by modifying them together with a CAS loop.
*
+ * NB: A future commit will use a significant portion of the remaining bits to
+ * implement buffer locking as part of the state variable.
+ *
* The definition of buffer state components is below.
*/
#define BUF_REFCOUNT_BITS 18
@@ -49,15 +52,21 @@ StaticAssertDecl(BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS + BUF_FLAG_BITS == 32,
"parts of buffer state space need to equal 32");
#define BUF_REFCOUNT_ONE 1
-#define BUF_REFCOUNT_MASK ((1U << BUF_REFCOUNT_BITS) - 1)
-#define BUF_USAGECOUNT_MASK (((1U << BUF_USAGECOUNT_BITS) - 1) << (BUF_REFCOUNT_BITS))
-#define BUF_USAGECOUNT_ONE (1U << BUF_REFCOUNT_BITS)
+#define BUF_REFCOUNT_MASK \
+ ((UINT64CONST(1) << BUF_REFCOUNT_BITS) - 1)
+#define BUF_USAGECOUNT_MASK \
+ (((UINT64CONST(1) << BUF_USAGECOUNT_BITS) - 1) << (BUF_REFCOUNT_BITS))
+#define BUF_USAGECOUNT_ONE \
+ (UINT64CONST(1) << BUF_REFCOUNT_BITS)
#define BUF_USAGECOUNT_SHIFT BUF_REFCOUNT_BITS
-#define BUF_FLAG_MASK (((1U << BUF_FLAG_BITS) - 1) << (BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS))
+#define BUF_FLAG_MASK \
+ (((UINT64CONST(1) << BUF_FLAG_BITS) - 1) << (BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS))
/* Get refcount and usagecount from buffer state */
-#define BUF_STATE_GET_REFCOUNT(state) ((state) & BUF_REFCOUNT_MASK)
-#define BUF_STATE_GET_USAGECOUNT(state) (((state) & BUF_USAGECOUNT_MASK) >> BUF_USAGECOUNT_SHIFT)
+#define BUF_STATE_GET_REFCOUNT(state) \
+ ((uint32)((state) & BUF_REFCOUNT_MASK))
+#define BUF_STATE_GET_USAGECOUNT(state) \
+ ((uint32)(((state) & BUF_USAGECOUNT_MASK) >> BUF_USAGECOUNT_SHIFT))
/*
* Flags for buffer descriptors
@@ -65,17 +74,28 @@ StaticAssertDecl(BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS + BUF_FLAG_BITS == 32,
* Note: BM_TAG_VALID essentially means that there is a buffer hashtable
* entry associated with the buffer's tag.
*/
-#define BM_LOCKED (1U << 22) /* buffer header is locked */
-#define BM_DIRTY (1U << 23) /* data needs writing */
-#define BM_VALID (1U << 24) /* data is valid */
-#define BM_TAG_VALID (1U << 25) /* tag is assigned */
-#define BM_IO_IN_PROGRESS (1U << 26) /* read or write in progress */
-#define BM_IO_ERROR (1U << 27) /* previous I/O failed */
-#define BM_JUST_DIRTIED (1U << 28) /* dirtied since write started */
-#define BM_PIN_COUNT_WAITER (1U << 29) /* have waiter for sole pin */
-#define BM_CHECKPOINT_NEEDED (1U << 30) /* must write for checkpoint */
-#define BM_PERMANENT (1U << 31) /* permanent buffer (not unlogged,
- * or init fork) */
+
+/* buffer header is locked */
+#define BM_LOCKED (UINT64CONST(1) << 22)
+/* data needs writing */
+#define BM_DIRTY (UINT64CONST(1) << 23)
+/* data is valid */
+#define BM_VALID (UINT64CONST(1) << 24)
+/* tag is assigned */
+#define BM_TAG_VALID (UINT64CONST(1) << 25)
+/* read or write in progress */
+#define BM_IO_IN_PROGRESS (UINT64CONST(1) << 26)
+/* previous I/O failed */
+#define BM_IO_ERROR (UINT64CONST(1) << 27)
+/* dirtied since write started */
+#define BM_JUST_DIRTIED (UINT64CONST(1) << 28)
+/* have waiter for sole pin */
+#define BM_PIN_COUNT_WAITER (UINT64CONST(1) << 29)
+/* must write for checkpoint */
+#define BM_CHECKPOINT_NEEDED (UINT64CONST(1) << 30)
+/* permanent buffer (not unlogged, or init fork) */
+#define BM_PERMANENT (UINT64CONST(1) << 31)
+
/*
* The maximum allowed value of usage_count represents a tradeoff between
* accuracy and speed of the clock-sweep buffer management algorithm. A
@@ -86,7 +106,7 @@ StaticAssertDecl(BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS + BUF_FLAG_BITS == 32,
*/
#define BM_MAX_USAGE_COUNT 5
-StaticAssertDecl(BM_MAX_USAGE_COUNT < (1 << BUF_USAGECOUNT_BITS),
+StaticAssertDecl(BM_MAX_USAGE_COUNT < (UINT64CONST(1) << BUF_USAGECOUNT_BITS),
"BM_MAX_USAGE_COUNT doesn't fit in BUF_USAGECOUNT_BITS bits");
StaticAssertDecl(MAX_BACKENDS_BITS <= BUF_REFCOUNT_BITS,
"MAX_BACKENDS_BITS needs to be <= BUF_REFCOUNT_BITS");
@@ -251,8 +271,8 @@ BufMappingPartitionLockByIndex(uint32 index)
* We use this same struct for local buffer headers, but the locks are not
* used and not all of the flag bits are useful either. To avoid unnecessary
* overhead, manipulations of the state field should be done without actual
- * atomic operations (i.e. only pg_atomic_read_u32() and
- * pg_atomic_unlocked_write_u32()).
+ * atomic operations (i.e. only pg_atomic_read_u64() and
+ * pg_atomic_unlocked_write_u64()).
*
* Be careful to avoid increasing the size of the struct when adding or
* reordering members. Keeping it below 64 bytes (the most common CPU
@@ -280,7 +300,7 @@ typedef struct BufferDesc
* State of the buffer, containing flags, refcount and usagecount. See
* BUF_* and BM_* defines at the top of this file.
*/
- pg_atomic_uint32 state;
+ pg_atomic_uint64 state;
/*
* Backend of pin-count waiter. The buffer header spinlock needs to be
@@ -386,7 +406,7 @@ BufferDescriptorGetContentLock(const BufferDesc *bdesc)
* Functions for acquiring/releasing a shared buffer header's spinlock. Do
* not apply these to local buffers!
*/
-extern uint32 LockBufHdr(BufferDesc *desc);
+extern uint64 LockBufHdr(BufferDesc *desc);
/*
* Unlock the buffer header.
@@ -397,9 +417,9 @@ extern uint32 LockBufHdr(BufferDesc *desc);
static inline void
UnlockBufHdr(BufferDesc *desc)
{
- Assert(pg_atomic_read_u32(&desc->state) & BM_LOCKED);
+ Assert(pg_atomic_read_u64(&desc->state) & BM_LOCKED);
- pg_atomic_fetch_sub_u32(&desc->state, BM_LOCKED);
+ pg_atomic_fetch_sub_u64(&desc->state, BM_LOCKED);
}
/*
@@ -410,14 +430,14 @@ UnlockBufHdr(BufferDesc *desc)
* Note that this approach would not work for usagecount, since we need to cap
* the usagecount at BM_MAX_USAGE_COUNT.
*/
-static inline uint32
-UnlockBufHdrExt(BufferDesc *desc, uint32 old_buf_state,
- uint32 set_bits, uint32 unset_bits,
+static inline uint64
+UnlockBufHdrExt(BufferDesc *desc, uint64 old_buf_state,
+ uint64 set_bits, uint64 unset_bits,
int refcount_change)
{
for (;;)
{
- uint32 buf_state = old_buf_state;
+ uint64 buf_state = old_buf_state;
Assert(buf_state & BM_LOCKED);
@@ -428,7 +448,7 @@ UnlockBufHdrExt(BufferDesc *desc, uint32 old_buf_state,
if (refcount_change != 0)
buf_state += BUF_REFCOUNT_ONE * refcount_change;
- if (pg_atomic_compare_exchange_u32(&desc->state, &old_buf_state,
+ if (pg_atomic_compare_exchange_u64(&desc->state, &old_buf_state,
buf_state))
{
return old_buf_state;
@@ -436,7 +456,7 @@ UnlockBufHdrExt(BufferDesc *desc, uint32 old_buf_state,
}
}
-extern uint32 WaitBufHdrUnlocked(BufferDesc *buf);
+extern uint64 WaitBufHdrUnlocked(BufferDesc *buf);
/* in bufmgr.c */
@@ -496,14 +516,14 @@ extern void TrackNewBufferPin(Buffer buf);
/* solely to make it easier to write tests */
extern bool StartBufferIO(BufferDesc *buf, bool forInput, bool nowait);
-extern void TerminateBufferIO(BufferDesc *buf, bool clear_dirty, uint32 set_flag_bits,
+extern void TerminateBufferIO(BufferDesc *buf, bool clear_dirty, uint64 set_flag_bits,
bool forget_owner, bool release_aio);
/* freelist.c */
extern IOContext IOContextForStrategy(BufferAccessStrategy strategy);
extern BufferDesc *StrategyGetBuffer(BufferAccessStrategy strategy,
- uint32 *buf_state, bool *from_ring);
+ uint64 *buf_state, bool *from_ring);
extern bool StrategyRejectBuffer(BufferAccessStrategy strategy,
BufferDesc *buf, bool from_ring);
@@ -539,7 +559,7 @@ extern BlockNumber ExtendBufferedRelLocal(BufferManagerRelation bmr,
uint32 *extended_by);
extern void MarkLocalBufferDirty(Buffer buffer);
extern void TerminateLocalBufferIO(BufferDesc *bufHdr, bool clear_dirty,
- uint32 set_flag_bits, bool release_aio);
+ uint64 set_flag_bits, bool release_aio);
extern bool StartLocalBufferIO(BufferDesc *bufHdr, bool forInput, bool nowait);
extern void FlushLocalBuffer(BufferDesc *bufHdr, SMgrRelation reln);
extern void InvalidateLocalBuffer(BufferDesc *bufHdr, bool check_unreferenced);
diff --git a/src/backend/storage/buffer/buf_init.c b/src/backend/storage/buffer/buf_init.c
index 6fd3a6bbac5..25f71191ec3 100644
--- a/src/backend/storage/buffer/buf_init.c
+++ b/src/backend/storage/buffer/buf_init.c
@@ -121,7 +121,7 @@ BufferManagerShmemInit(void)
ClearBufferTag(&buf->tag);
- pg_atomic_init_u32(&buf->state, 0);
+ pg_atomic_init_u64(&buf->state, 0);
buf->wait_backend_pgprocno = INVALID_PROC_NUMBER;
buf->buf_id = i;
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index b682878b1fb..e33fa0cbfec 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -686,7 +686,7 @@ ReadRecentBuffer(RelFileLocator rlocator, ForkNumber forkNum, BlockNumber blockN
{
BufferDesc *bufHdr;
BufferTag tag;
- uint32 buf_state;
+ uint64 buf_state;
Assert(BufferIsValid(recent_buffer));
@@ -699,7 +699,7 @@ ReadRecentBuffer(RelFileLocator rlocator, ForkNumber forkNum, BlockNumber blockN
int b = -recent_buffer - 1;
bufHdr = GetLocalBufferDescriptor(b);
- buf_state = pg_atomic_read_u32(&bufHdr->state);
+ buf_state = pg_atomic_read_u64(&bufHdr->state);
/* Is it still valid and holding the right tag? */
if ((buf_state & BM_VALID) && BufferTagsEqual(&tag, &bufHdr->tag))
@@ -1292,8 +1292,8 @@ StartReadBuffersImpl(ReadBuffersOperation *operation,
bufHdr = GetLocalBufferDescriptor(-buffers[i] - 1);
else
bufHdr = GetBufferDescriptor(buffers[i] - 1);
- Assert(pg_atomic_read_u32(&bufHdr->state) & BM_TAG_VALID);
- found = pg_atomic_read_u32(&bufHdr->state) & BM_VALID;
+ Assert(pg_atomic_read_u64(&bufHdr->state) & BM_TAG_VALID);
+ found = pg_atomic_read_u64(&bufHdr->state) & BM_VALID;
}
else
{
@@ -1519,10 +1519,10 @@ CheckReadBuffersOperation(ReadBuffersOperation *operation, bool is_complete)
GetBufferDescriptor(buffer - 1);
Assert(BufferGetBlockNumber(buffer) == operation->blocknum + i);
- Assert(pg_atomic_read_u32(&buf_hdr->state) & BM_TAG_VALID);
+ Assert(pg_atomic_read_u64(&buf_hdr->state) & BM_TAG_VALID);
if (i < operation->nblocks_done)
- Assert(pg_atomic_read_u32(&buf_hdr->state) & BM_VALID);
+ Assert(pg_atomic_read_u64(&buf_hdr->state) & BM_VALID);
}
#endif
}
@@ -1989,8 +1989,8 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
int existing_buf_id;
Buffer victim_buffer;
BufferDesc *victim_buf_hdr;
- uint32 victim_buf_state;
- uint32 set_bits = 0;
+ uint64 victim_buf_state;
+ uint64 set_bits = 0;
/* Make sure we will have room to remember the buffer pin */
ResourceOwnerEnlarge(CurrentResourceOwner);
@@ -2157,7 +2157,7 @@ InvalidateBuffer(BufferDesc *buf)
uint32 oldHash; /* hash value for oldTag */
LWLock *oldPartitionLock; /* buffer partition lock for it */
uint32 oldFlags;
- uint32 buf_state;
+ uint64 buf_state;
/* Save the original buffer tag before dropping the spinlock */
oldTag = buf->tag;
@@ -2248,7 +2248,7 @@ retry:
static bool
InvalidateVictimBuffer(BufferDesc *buf_hdr)
{
- uint32 buf_state;
+ uint64 buf_state;
uint32 hash;
LWLock *partition_lock;
BufferTag tag;
@@ -2308,10 +2308,10 @@ InvalidateVictimBuffer(BufferDesc *buf_hdr)
LWLockRelease(partition_lock);
- buf_state = pg_atomic_read_u32(&buf_hdr->state);
+ buf_state = pg_atomic_read_u64(&buf_hdr->state);
Assert(!(buf_state & (BM_DIRTY | BM_VALID | BM_TAG_VALID)));
Assert(BUF_STATE_GET_REFCOUNT(buf_state) > 0);
- Assert(BUF_STATE_GET_REFCOUNT(pg_atomic_read_u32(&buf_hdr->state)) > 0);
+ Assert(BUF_STATE_GET_REFCOUNT(pg_atomic_read_u64(&buf_hdr->state)) > 0);
return true;
}
@@ -2321,7 +2321,7 @@ GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context)
{
BufferDesc *buf_hdr;
Buffer buf;
- uint32 buf_state;
+ uint64 buf_state;
bool from_ring;
/*
@@ -2454,7 +2454,7 @@ again:
/* a final set of sanity checks */
#ifdef USE_ASSERT_CHECKING
- buf_state = pg_atomic_read_u32(&buf_hdr->state);
+ buf_state = pg_atomic_read_u64(&buf_hdr->state);
Assert(BUF_STATE_GET_REFCOUNT(buf_state) == 1);
Assert(!(buf_state & (BM_TAG_VALID | BM_VALID | BM_DIRTY)));
@@ -2745,13 +2745,13 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
*/
do
{
- pg_atomic_fetch_and_u32(&existing_hdr->state, ~BM_VALID);
+ pg_atomic_fetch_and_u64(&existing_hdr->state, ~BM_VALID);
} while (!StartBufferIO(existing_hdr, true, false));
}
else
{
- uint32 buf_state;
- uint32 set_bits = 0;
+ uint64 buf_state;
+ uint64 set_bits = 0;
buf_state = LockBufHdr(victim_buf_hdr);
@@ -2927,7 +2927,7 @@ BufferIsDirty(Buffer buffer)
Assert(BufferIsLockedByMeInMode(buffer, BUFFER_LOCK_EXCLUSIVE));
}
- return pg_atomic_read_u32(&bufHdr->state) & BM_DIRTY;
+ return pg_atomic_read_u64(&bufHdr->state) & BM_DIRTY;
}
/*
@@ -2943,8 +2943,8 @@ void
MarkBufferDirty(Buffer buffer)
{
BufferDesc *bufHdr;
- uint32 buf_state;
- uint32 old_buf_state;
+ uint64 buf_state;
+ uint64 old_buf_state;
if (!BufferIsValid(buffer))
elog(ERROR, "bad buffer ID: %d", buffer);
@@ -2964,7 +2964,7 @@ MarkBufferDirty(Buffer buffer)
* NB: We have to wait for the buffer header spinlock to be not held, as
* TerminateBufferIO() relies on the spinlock.
*/
- old_buf_state = pg_atomic_read_u32(&bufHdr->state);
+ old_buf_state = pg_atomic_read_u64(&bufHdr->state);
for (;;)
{
if (old_buf_state & BM_LOCKED)
@@ -2975,7 +2975,7 @@ MarkBufferDirty(Buffer buffer)
Assert(BUF_STATE_GET_REFCOUNT(buf_state) > 0);
buf_state |= BM_DIRTY | BM_JUST_DIRTIED;
- if (pg_atomic_compare_exchange_u32(&bufHdr->state, &old_buf_state,
+ if (pg_atomic_compare_exchange_u64(&bufHdr->state, &old_buf_state,
buf_state))
break;
}
@@ -3079,10 +3079,10 @@ PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy,
if (ref == NULL)
{
- uint32 buf_state;
- uint32 old_buf_state;
+ uint64 buf_state;
+ uint64 old_buf_state;
- old_buf_state = pg_atomic_read_u32(&buf->state);
+ old_buf_state = pg_atomic_read_u64(&buf->state);
for (;;)
{
if (unlikely(skip_if_not_valid && !(old_buf_state & BM_VALID)))
@@ -3116,7 +3116,7 @@ PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy,
buf_state += BUF_USAGECOUNT_ONE;
}
- if (pg_atomic_compare_exchange_u32(&buf->state, &old_buf_state,
+ if (pg_atomic_compare_exchange_u64(&buf->state, &old_buf_state,
buf_state))
{
result = (buf_state & BM_VALID) != 0;
@@ -3143,7 +3143,7 @@ PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy,
* that the buffer page is legitimately non-accessible here. We
* cannot meddle with that.
*/
- result = (pg_atomic_read_u32(&buf->state) & BM_VALID) != 0;
+ result = (pg_atomic_read_u64(&buf->state) & BM_VALID) != 0;
Assert(ref->refcount > 0);
ref->refcount++;
@@ -3178,7 +3178,7 @@ PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy,
static void
PinBuffer_Locked(BufferDesc *buf)
{
- uint32 old_buf_state;
+ uint64 old_buf_state;
/*
* As explained, We don't expect any preexisting pins. That allows us to
@@ -3190,7 +3190,7 @@ PinBuffer_Locked(BufferDesc *buf)
* Since we hold the buffer spinlock, we can update the buffer state and
* release the lock in one operation.
*/
- old_buf_state = pg_atomic_read_u32(&buf->state);
+ old_buf_state = pg_atomic_read_u64(&buf->state);
UnlockBufHdrExt(buf, old_buf_state,
0, 0, 1);
@@ -3220,7 +3220,7 @@ WakePinCountWaiter(BufferDesc *buf)
* BM_PIN_COUNT_WAITER if it stops waiting for a reason other than this
* backend waking it up.
*/
- uint32 buf_state = LockBufHdr(buf);
+ uint64 buf_state = LockBufHdr(buf);
if ((buf_state & BM_PIN_COUNT_WAITER) &&
BUF_STATE_GET_REFCOUNT(buf_state) == 1)
@@ -3267,7 +3267,7 @@ UnpinBufferNoOwner(BufferDesc *buf)
ref->refcount--;
if (ref->refcount == 0)
{
- uint32 old_buf_state;
+ uint64 old_buf_state;
/*
* Mark buffer non-accessible to Valgrind.
@@ -3285,7 +3285,7 @@ UnpinBufferNoOwner(BufferDesc *buf)
Assert(!LWLockHeldByMe(BufferDescriptorGetContentLock(buf)));
/* decrement the shared reference count */
- old_buf_state = pg_atomic_fetch_sub_u32(&buf->state, BUF_REFCOUNT_ONE);
+ old_buf_state = pg_atomic_fetch_sub_u64(&buf->state, BUF_REFCOUNT_ONE);
/* Support LockBufferForCleanup() */
if (old_buf_state & BM_PIN_COUNT_WAITER)
@@ -3342,7 +3342,7 @@ TrackNewBufferPin(Buffer buf)
static void
BufferSync(int flags)
{
- uint32 buf_state;
+ uint64 buf_state;
int buf_id;
int num_to_scan;
int num_spaces;
@@ -3352,7 +3352,7 @@ BufferSync(int flags)
Oid last_tsid;
binaryheap *ts_heap;
int i;
- uint32 mask = BM_DIRTY;
+ uint64 mask = BM_DIRTY;
WritebackContext wb_context;
/*
@@ -3384,7 +3384,7 @@ BufferSync(int flags)
for (buf_id = 0; buf_id < NBuffers; buf_id++)
{
BufferDesc *bufHdr = GetBufferDescriptor(buf_id);
- uint32 set_bits = 0;
+ uint64 set_bits = 0;
/*
* Header spinlock is enough to examine BM_DIRTY, see comment in
@@ -3551,7 +3551,7 @@ BufferSync(int flags)
* write the buffer though we didn't need to. It doesn't seem worth
* guarding against this, though.
*/
- if (pg_atomic_read_u32(&bufHdr->state) & BM_CHECKPOINT_NEEDED)
+ if (pg_atomic_read_u64(&bufHdr->state) & BM_CHECKPOINT_NEEDED)
{
if (SyncOneBuffer(buf_id, false, &wb_context) & BUF_WRITTEN)
{
@@ -3921,7 +3921,7 @@ SyncOneBuffer(int buf_id, bool skip_recently_used, WritebackContext *wb_context)
{
BufferDesc *bufHdr = GetBufferDescriptor(buf_id);
int result = 0;
- uint32 buf_state;
+ uint64 buf_state;
BufferTag tag;
/* Make sure we can handle the pin */
@@ -4169,7 +4169,7 @@ DebugPrintBufferRefcount(Buffer buffer)
int32 loccount;
char *result;
ProcNumber backend;
- uint32 buf_state;
+ uint64 buf_state;
Assert(BufferIsValid(buffer));
if (BufferIsLocal(buffer))
@@ -4186,9 +4186,9 @@ DebugPrintBufferRefcount(Buffer buffer)
}
/* theoretically we should lock the bufhdr here */
- buf_state = pg_atomic_read_u32(&buf->state);
+ buf_state = pg_atomic_read_u64(&buf->state);
- result = psprintf("[%03d] (rel=%s, blockNum=%u, flags=0x%x, refcount=%u %d)",
+ result = psprintf("[%03d] (rel=%s, blockNum=%u, flags=0x%" PRIx64 ", refcount=%u %d)",
buffer,
relpathbackend(BufTagGetRelFileLocator(&buf->tag), backend,
BufTagGetForkNum(&buf->tag)).str,
@@ -4288,7 +4288,7 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object,
instr_time io_start;
Block bufBlock;
char *bufToWrite;
- uint32 buf_state;
+ uint64 buf_state;
/*
* Try to start an I/O operation. If StartBufferIO returns false, then
@@ -4486,7 +4486,7 @@ BufferIsPermanent(Buffer buffer)
* not random garbage.
*/
bufHdr = GetBufferDescriptor(buffer - 1);
- return (pg_atomic_read_u32(&bufHdr->state) & BM_PERMANENT) != 0;
+ return (pg_atomic_read_u64(&bufHdr->state) & BM_PERMANENT) != 0;
}
/*
@@ -4949,11 +4949,11 @@ FlushRelationBuffers(Relation rel)
{
for (i = 0; i < NLocBuffer; i++)
{
- uint32 buf_state;
+ uint64 buf_state;
bufHdr = GetLocalBufferDescriptor(i);
if (BufTagMatchesRelFileLocator(&bufHdr->tag, &rel->rd_locator) &&
- ((buf_state = pg_atomic_read_u32(&bufHdr->state)) &
+ ((buf_state = pg_atomic_read_u64(&bufHdr->state)) &
(BM_VALID | BM_DIRTY)) == (BM_VALID | BM_DIRTY))
{
ErrorContextCallback errcallback;
@@ -4989,7 +4989,7 @@ FlushRelationBuffers(Relation rel)
for (i = 0; i < NBuffers; i++)
{
- uint32 buf_state;
+ uint64 buf_state;
bufHdr = GetBufferDescriptor(i);
@@ -5061,7 +5061,7 @@ FlushRelationsAllBuffers(SMgrRelation *smgrs, int nrels)
{
SMgrSortArray *srelent = NULL;
BufferDesc *bufHdr = GetBufferDescriptor(i);
- uint32 buf_state;
+ uint64 buf_state;
/*
* As in DropRelationBuffers, an unlocked precheck should be safe and
@@ -5310,7 +5310,7 @@ FlushDatabaseBuffers(Oid dbid)
for (i = 0; i < NBuffers; i++)
{
- uint32 buf_state;
+ uint64 buf_state;
bufHdr = GetBufferDescriptor(i);
@@ -5458,13 +5458,13 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
* is only intended to be used in cases where failing to write out the
* data would be harmless anyway, it doesn't really matter.
*/
- if ((pg_atomic_read_u32(&bufHdr->state) & (BM_DIRTY | BM_JUST_DIRTIED)) !=
+ if ((pg_atomic_read_u64(&bufHdr->state) & (BM_DIRTY | BM_JUST_DIRTIED)) !=
(BM_DIRTY | BM_JUST_DIRTIED))
{
XLogRecPtr lsn = InvalidXLogRecPtr;
bool dirtied = false;
bool delayChkptFlags = false;
- uint32 buf_state;
+ uint64 buf_state;
/*
* If we need to protect hint bit updates from torn writes, WAL-log a
@@ -5476,7 +5476,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
* when we call XLogInsert() since the value changes dynamically.
*/
if (XLogHintBitIsNeeded() &&
- (pg_atomic_read_u32(&bufHdr->state) & BM_PERMANENT))
+ (pg_atomic_read_u64(&bufHdr->state) & BM_PERMANENT))
{
/*
* If we must not write WAL, due to a relfilelocator-specific
@@ -5576,8 +5576,8 @@ UnlockBuffers(void)
if (buf)
{
- uint32 buf_state;
- uint32 unset_bits = 0;
+ uint64 buf_state;
+ uint64 unset_bits = 0;
buf_state = LockBufHdr(buf);
@@ -5708,8 +5708,8 @@ LockBufferForCleanup(Buffer buffer)
for (;;)
{
- uint32 buf_state;
- uint32 unset_bits = 0;
+ uint64 buf_state;
+ uint64 unset_bits = 0;
/* Try to acquire lock */
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
@@ -5857,7 +5857,7 @@ bool
ConditionalLockBufferForCleanup(Buffer buffer)
{
BufferDesc *bufHdr;
- uint32 buf_state,
+ uint64 buf_state,
refcount;
Assert(BufferIsValid(buffer));
@@ -5915,7 +5915,7 @@ bool
IsBufferCleanupOK(Buffer buffer)
{
BufferDesc *bufHdr;
- uint32 buf_state;
+ uint64 buf_state;
Assert(BufferIsValid(buffer));
@@ -5971,7 +5971,7 @@ WaitIO(BufferDesc *buf)
ConditionVariablePrepareToSleep(cv);
for (;;)
{
- uint32 buf_state;
+ uint64 buf_state;
PgAioWaitRef iow;
/*
@@ -6045,7 +6045,7 @@ WaitIO(BufferDesc *buf)
bool
StartBufferIO(BufferDesc *buf, bool forInput, bool nowait)
{
- uint32 buf_state;
+ uint64 buf_state;
ResourceOwnerEnlarge(CurrentResourceOwner);
@@ -6101,11 +6101,11 @@ StartBufferIO(BufferDesc *buf, bool forInput, bool nowait)
* is being released)
*/
void
-TerminateBufferIO(BufferDesc *buf, bool clear_dirty, uint32 set_flag_bits,
+TerminateBufferIO(BufferDesc *buf, bool clear_dirty, uint64 set_flag_bits,
bool forget_owner, bool release_aio)
{
- uint32 buf_state;
- uint32 unset_flag_bits = 0;
+ uint64 buf_state;
+ uint64 unset_flag_bits = 0;
int refcount_change = 0;
buf_state = LockBufHdr(buf);
@@ -6166,7 +6166,7 @@ static void
AbortBufferIO(Buffer buffer)
{
BufferDesc *buf_hdr = GetBufferDescriptor(buffer - 1);
- uint32 buf_state;
+ uint64 buf_state;
buf_state = LockBufHdr(buf_hdr);
Assert(buf_state & (BM_IO_IN_PROGRESS | BM_TAG_VALID));
@@ -6260,11 +6260,11 @@ rlocator_comparator(const void *p1, const void *p2)
/*
* Lock buffer header - set BM_LOCKED in buffer state.
*/
-uint32
+uint64
LockBufHdr(BufferDesc *desc)
{
SpinDelayStatus delayStatus;
- uint32 old_buf_state;
+ uint64 old_buf_state;
Assert(!BufferIsLocal(BufferDescriptorGetBuffer(desc)));
@@ -6273,7 +6273,7 @@ LockBufHdr(BufferDesc *desc)
while (true)
{
/* set BM_LOCKED flag */
- old_buf_state = pg_atomic_fetch_or_u32(&desc->state, BM_LOCKED);
+ old_buf_state = pg_atomic_fetch_or_u64(&desc->state, BM_LOCKED);
/* if it wasn't set before we're OK */
if (!(old_buf_state & BM_LOCKED))
break;
@@ -6290,20 +6290,20 @@ LockBufHdr(BufferDesc *desc)
* Obviously the buffer could be locked by the time the value is returned, so
* this is primarily useful in CAS style loops.
*/
-pg_noinline uint32
+pg_noinline uint64
WaitBufHdrUnlocked(BufferDesc *buf)
{
SpinDelayStatus delayStatus;
- uint32 buf_state;
+ uint64 buf_state;
init_local_spin_delay(&delayStatus);
- buf_state = pg_atomic_read_u32(&buf->state);
+ buf_state = pg_atomic_read_u64(&buf->state);
while (buf_state & BM_LOCKED)
{
perform_spin_delay(&delayStatus);
- buf_state = pg_atomic_read_u32(&buf->state);
+ buf_state = pg_atomic_read_u64(&buf->state);
}
finish_spin_delay(&delayStatus);
@@ -6591,12 +6591,12 @@ ResOwnerPrintBufferPin(Datum res)
static bool
EvictUnpinnedBufferInternal(BufferDesc *desc, bool *buffer_flushed)
{
- uint32 buf_state;
+ uint64 buf_state;
bool result;
*buffer_flushed = false;
- buf_state = pg_atomic_read_u32(&(desc->state));
+ buf_state = pg_atomic_read_u64(&(desc->state));
Assert(buf_state & BM_LOCKED);
if ((buf_state & BM_VALID) == 0)
@@ -6690,12 +6690,12 @@ EvictAllUnpinnedBuffers(int32 *buffers_evicted, int32 *buffers_flushed,
for (int buf = 1; buf <= NBuffers; buf++)
{
BufferDesc *desc = GetBufferDescriptor(buf - 1);
- uint32 buf_state;
+ uint64 buf_state;
bool buffer_flushed;
CHECK_FOR_INTERRUPTS();
- buf_state = pg_atomic_read_u32(&desc->state);
+ buf_state = pg_atomic_read_u64(&desc->state);
if (!(buf_state & BM_VALID))
continue;
@@ -6742,7 +6742,7 @@ EvictRelUnpinnedBuffers(Relation rel, int32 *buffers_evicted,
for (int buf = 1; buf <= NBuffers; buf++)
{
BufferDesc *desc = GetBufferDescriptor(buf - 1);
- uint32 buf_state = pg_atomic_read_u32(&(desc->state));
+ uint64 buf_state = pg_atomic_read_u64(&(desc->state));
bool buffer_flushed;
CHECK_FOR_INTERRUPTS();
@@ -6809,7 +6809,7 @@ buffer_stage_common(PgAioHandle *ioh, bool is_write, bool is_temp)
BufferDesc *buf_hdr = is_temp ?
GetLocalBufferDescriptor(-buffer - 1)
: GetBufferDescriptor(buffer - 1);
- uint32 buf_state;
+ uint64 buf_state;
/*
* Check that all the buffers are actually ones that could conceivably
@@ -6827,7 +6827,7 @@ buffer_stage_common(PgAioHandle *ioh, bool is_write, bool is_temp)
}
if (is_temp)
- buf_state = pg_atomic_read_u32(&buf_hdr->state);
+ buf_state = pg_atomic_read_u64(&buf_hdr->state);
else
buf_state = LockBufHdr(buf_hdr);
@@ -6865,7 +6865,7 @@ buffer_stage_common(PgAioHandle *ioh, bool is_write, bool is_temp)
if (is_temp)
{
buf_state += BUF_REFCOUNT_ONE;
- pg_atomic_unlocked_write_u32(&buf_hdr->state, buf_state);
+ pg_atomic_unlocked_write_u64(&buf_hdr->state, buf_state);
}
else
UnlockBufHdrExt(buf_hdr, buf_state, 0, 0, 1);
@@ -7051,13 +7051,13 @@ buffer_readv_complete_one(PgAioTargetData *td, uint8 buf_off, Buffer buffer,
: GetBufferDescriptor(buffer - 1);
BufferTag tag = buf_hdr->tag;
char *bufdata = BufferGetBlock(buffer);
- uint32 set_flag_bits;
+ uint64 set_flag_bits;
int piv_flags;
/* check that the buffer is in the expected state for a read */
#ifdef USE_ASSERT_CHECKING
{
- uint32 buf_state = pg_atomic_read_u32(&buf_hdr->state);
+ uint64 buf_state = pg_atomic_read_u64(&buf_hdr->state);
Assert(buf_state & BM_TAG_VALID);
Assert(!(buf_state & BM_VALID));
diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c
index 28d952b3534..1d4f19a9afd 100644
--- a/src/backend/storage/buffer/freelist.c
+++ b/src/backend/storage/buffer/freelist.c
@@ -86,7 +86,7 @@ typedef struct BufferAccessStrategyData
/* Prototypes for internal functions */
static BufferDesc *GetBufferFromRing(BufferAccessStrategy strategy,
- uint32 *buf_state);
+ uint64 *buf_state);
static void AddBufferToRing(BufferAccessStrategy strategy,
BufferDesc *buf);
@@ -171,7 +171,7 @@ ClockSweepTick(void)
* before returning.
*/
BufferDesc *
-StrategyGetBuffer(BufferAccessStrategy strategy, uint32 *buf_state, bool *from_ring)
+StrategyGetBuffer(BufferAccessStrategy strategy, uint64 *buf_state, bool *from_ring)
{
BufferDesc *buf;
int bgwprocno;
@@ -230,8 +230,8 @@ StrategyGetBuffer(BufferAccessStrategy strategy, uint32 *buf_state, bool *from_r
trycounter = NBuffers;
for (;;)
{
- uint32 old_buf_state;
- uint32 local_buf_state;
+ uint64 old_buf_state;
+ uint64 local_buf_state;
buf = GetBufferDescriptor(ClockSweepTick());
@@ -239,7 +239,7 @@ StrategyGetBuffer(BufferAccessStrategy strategy, uint32 *buf_state, bool *from_r
* Check whether the buffer can be used and pin it if so. Do this
* using a CAS loop, to avoid having to lock the buffer header.
*/
- old_buf_state = pg_atomic_read_u32(&buf->state);
+ old_buf_state = pg_atomic_read_u64(&buf->state);
for (;;)
{
local_buf_state = old_buf_state;
@@ -277,7 +277,7 @@ StrategyGetBuffer(BufferAccessStrategy strategy, uint32 *buf_state, bool *from_r
{
local_buf_state -= BUF_USAGECOUNT_ONE;
- if (pg_atomic_compare_exchange_u32(&buf->state, &old_buf_state,
+ if (pg_atomic_compare_exchange_u64(&buf->state, &old_buf_state,
local_buf_state))
{
trycounter = NBuffers;
@@ -289,7 +289,7 @@ StrategyGetBuffer(BufferAccessStrategy strategy, uint32 *buf_state, bool *from_r
/* pin the buffer if the CAS succeeds */
local_buf_state += BUF_REFCOUNT_ONE;
- if (pg_atomic_compare_exchange_u32(&buf->state, &old_buf_state,
+ if (pg_atomic_compare_exchange_u64(&buf->state, &old_buf_state,
local_buf_state))
{
/* Found a usable buffer */
@@ -655,12 +655,12 @@ FreeAccessStrategy(BufferAccessStrategy strategy)
* returning.
*/
static BufferDesc *
-GetBufferFromRing(BufferAccessStrategy strategy, uint32 *buf_state)
+GetBufferFromRing(BufferAccessStrategy strategy, uint64 *buf_state)
{
BufferDesc *buf;
Buffer bufnum;
- uint32 old_buf_state;
- uint32 local_buf_state; /* to avoid repeated (de-)referencing */
+ uint64 old_buf_state;
+ uint64 local_buf_state; /* to avoid repeated (de-)referencing */
/* Advance to next ring slot */
@@ -682,7 +682,7 @@ GetBufferFromRing(BufferAccessStrategy strategy, uint32 *buf_state)
* Check whether the buffer can be used and pin it if so. Do this using a
* CAS loop, to avoid having to lock the buffer header.
*/
- old_buf_state = pg_atomic_read_u32(&buf->state);
+ old_buf_state = pg_atomic_read_u64(&buf->state);
for (;;)
{
local_buf_state = old_buf_state;
@@ -710,7 +710,7 @@ GetBufferFromRing(BufferAccessStrategy strategy, uint32 *buf_state)
/* pin the buffer if the CAS succeeds */
local_buf_state += BUF_REFCOUNT_ONE;
- if (pg_atomic_compare_exchange_u32(&buf->state, &old_buf_state,
+ if (pg_atomic_compare_exchange_u64(&buf->state, &old_buf_state,
local_buf_state))
{
*buf_state = local_buf_state;
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index 15aac7d1c9f..a41a5facd3a 100644
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -148,7 +148,7 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
}
else
{
- uint32 buf_state;
+ uint64 buf_state;
victim_buffer = GetLocalVictimBuffer();
bufid = -victim_buffer - 1;
@@ -165,10 +165,10 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
*/
bufHdr->tag = newTag;
- buf_state = pg_atomic_read_u32(&bufHdr->state);
+ buf_state = pg_atomic_read_u64(&bufHdr->state);
buf_state &= ~(BUF_FLAG_MASK | BUF_USAGECOUNT_MASK);
buf_state |= BM_TAG_VALID | BUF_USAGECOUNT_ONE;
- pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
+ pg_atomic_unlocked_write_u64(&bufHdr->state, buf_state);
*foundPtr = false;
}
@@ -245,12 +245,12 @@ GetLocalVictimBuffer(void)
if (LocalRefCount[victim_bufid] == 0)
{
- uint32 buf_state = pg_atomic_read_u32(&bufHdr->state);
+ uint64 buf_state = pg_atomic_read_u64(&bufHdr->state);
if (BUF_STATE_GET_USAGECOUNT(buf_state) > 0)
{
buf_state -= BUF_USAGECOUNT_ONE;
- pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
+ pg_atomic_unlocked_write_u64(&bufHdr->state, buf_state);
trycounter = NLocBuffer;
}
else if (BUF_STATE_GET_REFCOUNT(buf_state) > 0)
@@ -286,13 +286,13 @@ GetLocalVictimBuffer(void)
* this buffer is not referenced but it might still be dirty. if that's
* the case, write it out before reusing it!
*/
- if (pg_atomic_read_u32(&bufHdr->state) & BM_DIRTY)
+ if (pg_atomic_read_u64(&bufHdr->state) & BM_DIRTY)
FlushLocalBuffer(bufHdr, NULL);
/*
* Remove the victim buffer from the hashtable and mark as invalid.
*/
- if (pg_atomic_read_u32(&bufHdr->state) & BM_TAG_VALID)
+ if (pg_atomic_read_u64(&bufHdr->state) & BM_TAG_VALID)
{
InvalidateLocalBuffer(bufHdr, false);
@@ -417,7 +417,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
if (found)
{
BufferDesc *existing_hdr;
- uint32 buf_state;
+ uint64 buf_state;
UnpinLocalBuffer(BufferDescriptorGetBuffer(victim_buf_hdr));
@@ -428,18 +428,18 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
/*
* Clear the BM_VALID bit, do StartLocalBufferIO() and proceed.
*/
- buf_state = pg_atomic_read_u32(&existing_hdr->state);
+ buf_state = pg_atomic_read_u64(&existing_hdr->state);
Assert(buf_state & BM_TAG_VALID);
Assert(!(buf_state & BM_DIRTY));
buf_state &= ~BM_VALID;
- pg_atomic_unlocked_write_u32(&existing_hdr->state, buf_state);
+ pg_atomic_unlocked_write_u64(&existing_hdr->state, buf_state);
/* no need to loop for local buffers */
StartLocalBufferIO(existing_hdr, true, false);
}
else
{
- uint32 buf_state = pg_atomic_read_u32(&victim_buf_hdr->state);
+ uint64 buf_state = pg_atomic_read_u64(&victim_buf_hdr->state);
Assert(!(buf_state & (BM_VALID | BM_TAG_VALID | BM_DIRTY | BM_JUST_DIRTIED)));
@@ -447,7 +447,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
buf_state |= BM_TAG_VALID | BUF_USAGECOUNT_ONE;
- pg_atomic_unlocked_write_u32(&victim_buf_hdr->state, buf_state);
+ pg_atomic_unlocked_write_u64(&victim_buf_hdr->state, buf_state);
hresult->id = victim_buf_id;
@@ -467,13 +467,13 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
{
Buffer buf = buffers[i];
BufferDesc *buf_hdr;
- uint32 buf_state;
+ uint64 buf_state;
buf_hdr = GetLocalBufferDescriptor(-buf - 1);
- buf_state = pg_atomic_read_u32(&buf_hdr->state);
+ buf_state = pg_atomic_read_u64(&buf_hdr->state);
buf_state |= BM_VALID;
- pg_atomic_unlocked_write_u32(&buf_hdr->state, buf_state);
+ pg_atomic_unlocked_write_u64(&buf_hdr->state, buf_state);
}
*extended_by = extend_by;
@@ -492,7 +492,7 @@ MarkLocalBufferDirty(Buffer buffer)
{
int bufid;
BufferDesc *bufHdr;
- uint32 buf_state;
+ uint64 buf_state;
Assert(BufferIsLocal(buffer));
@@ -506,14 +506,14 @@ MarkLocalBufferDirty(Buffer buffer)
bufHdr = GetLocalBufferDescriptor(bufid);
- buf_state = pg_atomic_read_u32(&bufHdr->state);
+ buf_state = pg_atomic_read_u64(&bufHdr->state);
if (!(buf_state & BM_DIRTY))
pgBufferUsage.local_blks_dirtied++;
buf_state |= BM_DIRTY;
- pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
+ pg_atomic_unlocked_write_u64(&bufHdr->state, buf_state);
}
/*
@@ -522,7 +522,7 @@ MarkLocalBufferDirty(Buffer buffer)
bool
StartLocalBufferIO(BufferDesc *bufHdr, bool forInput, bool nowait)
{
- uint32 buf_state;
+ uint64 buf_state;
/*
* With AIO the buffer could have IO in progress, e.g. when there are two
@@ -542,7 +542,7 @@ StartLocalBufferIO(BufferDesc *bufHdr, bool forInput, bool nowait)
/* Once we get here, there is definitely no I/O active on this buffer */
/* Check if someone else already did the I/O */
- buf_state = pg_atomic_read_u32(&bufHdr->state);
+ buf_state = pg_atomic_read_u64(&bufHdr->state);
if (forInput ? (buf_state & BM_VALID) : !(buf_state & BM_DIRTY))
{
return false;
@@ -559,11 +559,11 @@ StartLocalBufferIO(BufferDesc *bufHdr, bool forInput, bool nowait)
* Like TerminateBufferIO, but for local buffers
*/
void
-TerminateLocalBufferIO(BufferDesc *bufHdr, bool clear_dirty, uint32 set_flag_bits,
+TerminateLocalBufferIO(BufferDesc *bufHdr, bool clear_dirty, uint64 set_flag_bits,
bool release_aio)
{
/* Only need to adjust flags */
- uint32 buf_state = pg_atomic_read_u32(&bufHdr->state);
+ uint64 buf_state = pg_atomic_read_u64(&bufHdr->state);
/* BM_IO_IN_PROGRESS isn't currently used for local buffers */
@@ -582,7 +582,7 @@ TerminateLocalBufferIO(BufferDesc *bufHdr, bool clear_dirty, uint32 set_flag_bit
}
buf_state |= set_flag_bits;
- pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
+ pg_atomic_unlocked_write_u64(&bufHdr->state, buf_state);
/* local buffers don't track IO using resowners */
@@ -606,7 +606,7 @@ InvalidateLocalBuffer(BufferDesc *bufHdr, bool check_unreferenced)
{
Buffer buffer = BufferDescriptorGetBuffer(bufHdr);
int bufid = -buffer - 1;
- uint32 buf_state;
+ uint64 buf_state;
LocalBufferLookupEnt *hresult;
/*
@@ -622,7 +622,7 @@ InvalidateLocalBuffer(BufferDesc *bufHdr, bool check_unreferenced)
Assert(!pgaio_wref_valid(&bufHdr->io_wref));
}
- buf_state = pg_atomic_read_u32(&bufHdr->state);
+ buf_state = pg_atomic_read_u64(&bufHdr->state);
/*
* We need to test not just LocalRefCount[bufid] but also the BufferDesc
@@ -647,7 +647,7 @@ InvalidateLocalBuffer(BufferDesc *bufHdr, bool check_unreferenced)
ClearBufferTag(&bufHdr->tag);
buf_state &= ~BUF_FLAG_MASK;
buf_state &= ~BUF_USAGECOUNT_MASK;
- pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
+ pg_atomic_unlocked_write_u64(&bufHdr->state, buf_state);
}
/*
@@ -671,9 +671,9 @@ DropRelationLocalBuffers(RelFileLocator rlocator, ForkNumber *forkNum,
for (i = 0; i < NLocBuffer; i++)
{
BufferDesc *bufHdr = GetLocalBufferDescriptor(i);
- uint32 buf_state;
+ uint64 buf_state;
- buf_state = pg_atomic_read_u32(&bufHdr->state);
+ buf_state = pg_atomic_read_u64(&bufHdr->state);
if (!(buf_state & BM_TAG_VALID) ||
!BufTagMatchesRelFileLocator(&bufHdr->tag, &rlocator))
@@ -706,9 +706,9 @@ DropRelationAllLocalBuffers(RelFileLocator rlocator)
for (i = 0; i < NLocBuffer; i++)
{
BufferDesc *bufHdr = GetLocalBufferDescriptor(i);
- uint32 buf_state;
+ uint64 buf_state;
- buf_state = pg_atomic_read_u32(&bufHdr->state);
+ buf_state = pg_atomic_read_u64(&bufHdr->state);
if ((buf_state & BM_TAG_VALID) &&
BufTagMatchesRelFileLocator(&bufHdr->tag, &rlocator))
@@ -804,11 +804,11 @@ InitLocalBuffers(void)
bool
PinLocalBuffer(BufferDesc *buf_hdr, bool adjust_usagecount)
{
- uint32 buf_state;
+ uint64 buf_state;
Buffer buffer = BufferDescriptorGetBuffer(buf_hdr);
int bufid = -buffer - 1;
- buf_state = pg_atomic_read_u32(&buf_hdr->state);
+ buf_state = pg_atomic_read_u64(&buf_hdr->state);
if (LocalRefCount[bufid] == 0)
{
@@ -819,7 +819,7 @@ PinLocalBuffer(BufferDesc *buf_hdr, bool adjust_usagecount)
{
buf_state += BUF_USAGECOUNT_ONE;
}
- pg_atomic_unlocked_write_u32(&buf_hdr->state, buf_state);
+ pg_atomic_unlocked_write_u64(&buf_hdr->state, buf_state);
/*
* See comment in PinBuffer().
@@ -856,14 +856,14 @@ UnpinLocalBufferNoOwner(Buffer buffer)
if (--LocalRefCount[buffid] == 0)
{
BufferDesc *buf_hdr = GetLocalBufferDescriptor(buffid);
- uint32 buf_state;
+ uint64 buf_state;
NLocalPinnedBuffers--;
- buf_state = pg_atomic_read_u32(&buf_hdr->state);
+ buf_state = pg_atomic_read_u64(&buf_hdr->state);
Assert(BUF_STATE_GET_REFCOUNT(buf_state) > 0);
buf_state -= BUF_REFCOUNT_ONE;
- pg_atomic_unlocked_write_u32(&buf_hdr->state, buf_state);
+ pg_atomic_unlocked_write_u64(&buf_hdr->state, buf_state);
/* see comment in UnpinBufferNoOwner */
VALGRIND_MAKE_MEM_NOACCESS(LocalBufHdrGetBlock(buf_hdr), BLCKSZ);
diff --git a/contrib/pg_buffercache/pg_buffercache_pages.c b/contrib/pg_buffercache/pg_buffercache_pages.c
index c29b784dfa1..32bd8aa784a 100644
--- a/contrib/pg_buffercache/pg_buffercache_pages.c
+++ b/contrib/pg_buffercache/pg_buffercache_pages.c
@@ -192,7 +192,7 @@ pg_buffercache_pages(PG_FUNCTION_ARGS)
for (i = 0; i < NBuffers; i++)
{
BufferDesc *bufHdr;
- uint32 buf_state;
+ uint64 buf_state;
CHECK_FOR_INTERRUPTS();
@@ -559,7 +559,7 @@ pg_buffercache_summary(PG_FUNCTION_ARGS)
for (int i = 0; i < NBuffers; i++)
{
BufferDesc *bufHdr;
- uint32 buf_state;
+ uint64 buf_state;
CHECK_FOR_INTERRUPTS();
@@ -570,7 +570,7 @@ pg_buffercache_summary(PG_FUNCTION_ARGS)
* noticeably increase the cost of the function.
*/
bufHdr = GetBufferDescriptor(i);
- buf_state = pg_atomic_read_u32(&bufHdr->state);
+ buf_state = pg_atomic_read_u64(&bufHdr->state);
if (buf_state & BM_VALID)
{
@@ -620,7 +620,7 @@ pg_buffercache_usage_counts(PG_FUNCTION_ARGS)
for (int i = 0; i < NBuffers; i++)
{
BufferDesc *bufHdr = GetBufferDescriptor(i);
- uint32 buf_state = pg_atomic_read_u32(&bufHdr->state);
+ uint64 buf_state = pg_atomic_read_u64(&bufHdr->state);
int usage_count;
CHECK_FOR_INTERRUPTS();
diff --git a/src/test/modules/test_aio/test_aio.c b/src/test/modules/test_aio/test_aio.c
index d7eadeab256..488d98e7e66 100644
--- a/src/test/modules/test_aio/test_aio.c
+++ b/src/test/modules/test_aio/test_aio.c
@@ -308,9 +308,9 @@ create_toy_buffer(Relation rel, BlockNumber blkno)
{
Buffer buf;
BufferDesc *buf_hdr;
- uint32 buf_state;
+ uint64 buf_state;
bool was_pinned = false;
- uint32 unset_bits = 0;
+ uint64 unset_bits = 0;
/* place buffer in shared buffers without erroring out */
buf = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_ZERO_AND_LOCK, NULL);
@@ -319,7 +319,7 @@ create_toy_buffer(Relation rel, BlockNumber blkno)
if (RelationUsesLocalBuffers(rel))
{
buf_hdr = GetLocalBufferDescriptor(-buf - 1);
- buf_state = pg_atomic_read_u32(&buf_hdr->state);
+ buf_state = pg_atomic_read_u64(&buf_hdr->state);
}
else
{
@@ -340,7 +340,7 @@ create_toy_buffer(Relation rel, BlockNumber blkno)
if (RelationUsesLocalBuffers(rel))
{
buf_state &= ~unset_bits;
- pg_atomic_unlocked_write_u32(&buf_hdr->state, buf_state);
+ pg_atomic_unlocked_write_u64(&buf_hdr->state, buf_state);
}
else
{
@@ -489,7 +489,7 @@ invalidate_rel_block(PG_FUNCTION_ARGS)
LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
- if (pg_atomic_read_u32(&buf_hdr->state) & BM_DIRTY)
+ if (pg_atomic_read_u64(&buf_hdr->state) & BM_DIRTY)
{
if (BufferIsLocal(buf))
FlushLocalBuffer(buf_hdr, NULL);
@@ -572,7 +572,7 @@ buffer_call_terminate_io(PG_FUNCTION_ARGS)
bool io_error = PG_GETARG_BOOL(3);
bool release_aio = PG_GETARG_BOOL(4);
bool clear_dirty = false;
- uint32 set_flag_bits = 0;
+ uint64 set_flag_bits = 0;
if (io_error)
set_flag_bits |= BM_IO_ERROR;
--
2.48.1.76.g4e746b1a31.dirty
[text/x-diff] v6-0005-Rename-BUFFERPIN-wait-event-class-to-BUFFER.patch (6.5K, ../6rgb2nvhyvnszz4ul3wfzlf5rheb2kkwrglthnna7qhe24onwr@vw27225tkyar/6-v6-0005-Rename-BUFFERPIN-wait-event-class-to-BUFFER.patch)
download | inline diff:
From 38bf7740795793403d5fba883be29a74d05ac91e Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Thu, 6 Nov 2025 09:15:18 -0500
Subject: [PATCH v6 05/14] Rename BUFFERPIN wait event class to BUFFER
In an upcoming patch more wait events will be added to the wait event
class (for buffer locking), making the current name too
specific. Alternatively we could introduce a dedicated wait event class for
those, but it seems somewhat confusing to have a BUFFERPIN and a BUFFER wait
event class.
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
src/include/utils/wait_classes.h | 2 +-
src/backend/storage/buffer/bufmgr.c | 2 +-
src/backend/storage/ipc/standby.c | 2 +-
src/backend/utils/activity/wait_event.c | 12 ++++++------
src/backend/utils/activity/wait_event_names.txt | 6 +++---
doc/src/sgml/monitoring.sgml | 8 +++-----
src/test/recovery/t/048_vacuum_horizon_floor.pl | 2 +-
src/test/regress/expected/sysviews.out | 2 +-
8 files changed, 17 insertions(+), 19 deletions(-)
diff --git a/src/include/utils/wait_classes.h b/src/include/utils/wait_classes.h
index 51ee68397d5..57888aa62f7 100644
--- a/src/include/utils/wait_classes.h
+++ b/src/include/utils/wait_classes.h
@@ -17,7 +17,7 @@
*/
#define PG_WAIT_LWLOCK 0x01000000U
#define PG_WAIT_LOCK 0x03000000U
-#define PG_WAIT_BUFFERPIN 0x04000000U
+#define PG_WAIT_BUFFER 0x04000000U
#define PG_WAIT_ACTIVITY 0x05000000U
#define PG_WAIT_CLIENT 0x06000000U
#define PG_WAIT_EXTENSION 0x07000000U
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index e33fa0cbfec..d4235ca7939 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -5799,7 +5799,7 @@ LockBufferForCleanup(Buffer buffer)
SetStartupBufferPinWaitBufId(-1);
}
else
- ProcWaitForSignal(WAIT_EVENT_BUFFER_PIN);
+ ProcWaitForSignal(WAIT_EVENT_BUFFER_CLEANUP);
/*
* Remove flag marking us as waiter. Normally this will not be set
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index 4222bdab078..fc45d72c79b 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -840,7 +840,7 @@ ResolveRecoveryConflictWithBufferPin(void)
* SIGHUP signal handler, etc cannot do that because it uses the different
* latch from that ProcWaitForSignal() waits on.
*/
- ProcWaitForSignal(WAIT_EVENT_BUFFER_PIN);
+ ProcWaitForSignal(WAIT_EVENT_BUFFER_CLEANUP);
if (got_standby_delay_timeout)
SendRecoveryConflictWithBufferPin(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN);
diff --git a/src/backend/utils/activity/wait_event.c b/src/backend/utils/activity/wait_event.c
index d9b8f34a355..96d61f77f6e 100644
--- a/src/backend/utils/activity/wait_event.c
+++ b/src/backend/utils/activity/wait_event.c
@@ -29,7 +29,7 @@
static const char *pgstat_get_wait_activity(WaitEventActivity w);
-static const char *pgstat_get_wait_bufferpin(WaitEventBufferPin w);
+static const char *pgstat_get_wait_buffer(WaitEventBuffer w);
static const char *pgstat_get_wait_client(WaitEventClient w);
static const char *pgstat_get_wait_ipc(WaitEventIPC w);
static const char *pgstat_get_wait_timeout(WaitEventTimeout w);
@@ -389,8 +389,8 @@ pgstat_get_wait_event_type(uint32 wait_event_info)
case PG_WAIT_LOCK:
event_type = "Lock";
break;
- case PG_WAIT_BUFFERPIN:
- event_type = "BufferPin";
+ case PG_WAIT_BUFFER:
+ event_type = "Buffer";
break;
case PG_WAIT_ACTIVITY:
event_type = "Activity";
@@ -453,11 +453,11 @@ pgstat_get_wait_event(uint32 wait_event_info)
case PG_WAIT_INJECTIONPOINT:
event_name = GetWaitEventCustomIdentifier(wait_event_info);
break;
- case PG_WAIT_BUFFERPIN:
+ case PG_WAIT_BUFFER:
{
- WaitEventBufferPin w = (WaitEventBufferPin) wait_event_info;
+ WaitEventBuffer w = (WaitEventBuffer) wait_event_info;
- event_name = pgstat_get_wait_bufferpin(w);
+ event_name = pgstat_get_wait_buffer(w);
break;
}
case PG_WAIT_ACTIVITY:
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index c1ac71ff7f2..1e5e368a5dc 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -279,12 +279,12 @@ WAL_WRITE "Waiting for a write to a WAL file."
ABI_compatibility:
#
-# Wait Events - Buffer Pin
+# Wait Events - Buffer
#
-Section: ClassName - WaitEventBufferPin
+Section: ClassName - WaitEventBuffer
-BUFFER_PIN "Waiting to acquire an exclusive pin on a buffer."
+BUFFER_CLEANUP "Waiting to acquire an exclusive pin on a buffer. Buffer pin waits can be protracted if another process holds an open cursor that last read data from the buffer in question."
ABI_compatibility:
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 436ef0e8bd0..97ae34a92aa 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1053,11 +1053,9 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
</entry>
</row>
<row>
- <entry><literal>BufferPin</literal></entry>
- <entry>The server process is waiting for exclusive access to
- a data buffer. Buffer pin waits can be protracted if
- another process holds an open cursor that last read data from the
- buffer in question. See <xref linkend="wait-event-bufferpin-table"/>.
+ <entry><literal>Buffer</literal></entry>
+ <entry>The server process is waiting for access to a data buffer.
+ See <xref linkend="wait-event-buffer-table"/>.
</entry>
</row>
<row>
diff --git a/src/test/recovery/t/048_vacuum_horizon_floor.pl b/src/test/recovery/t/048_vacuum_horizon_floor.pl
index 668eedd71b2..9cdf6cee8a7 100644
--- a/src/test/recovery/t/048_vacuum_horizon_floor.pl
+++ b/src/test/recovery/t/048_vacuum_horizon_floor.pl
@@ -194,7 +194,7 @@ $node_primary->poll_query_until(
qq[
SELECT count(*) >= 1 FROM pg_stat_activity
WHERE pid = $vacuum_pid
- AND wait_event = 'BufferPin';
+ AND wait_event = 'BufferCleanup';
],
't');
diff --git a/src/test/regress/expected/sysviews.out b/src/test/regress/expected/sysviews.out
index 3b37fafa65b..0411db832f1 100644
--- a/src/test/regress/expected/sysviews.out
+++ b/src/test/regress/expected/sysviews.out
@@ -182,7 +182,7 @@ select type, count(*) > 0 as ok FROM pg_wait_events
type | ok
-----------+----
Activity | t
- BufferPin | t
+ Buffer | t
Client | t
Extension | t
IO | t
--
2.48.1.76.g4e746b1a31.dirty
[text/x-diff] v6-0006-bufmgr-Separate-keys-for-private-refcount-infrast.patch (10.0K, ../6rgb2nvhyvnszz4ul3wfzlf5rheb2kkwrglthnna7qhe24onwr@vw27225tkyar/7-v6-0006-bufmgr-Separate-keys-for-private-refcount-infrast.patch)
download | inline diff:
From f2e8d9de5bd2dae9b16264cb77d92bcc8e5ec8df Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 12 Nov 2025 12:50:52 -0500
Subject: [PATCH v6 06/14] bufmgr: Separate keys for private refcount
infrastructure
This makes lookups faster, due to allowing auto-vectorized lookups. It is also
beneficial for an upcoming patch, independent of auto-vectorization, as the
upcoming patch wants to track more information for each pinned buffer, making
the existing loop, iterating over an array of PrivateRefCountEntry, more
expensive due to increasing its size.
Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
src/backend/storage/buffer/bufmgr.c | 123 ++++++++++++++++++----------
src/tools/pgindent/typedefs.list | 1 +
2 files changed, 79 insertions(+), 45 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index d4235ca7939..972eae1fc7b 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -90,10 +90,18 @@
*/
#define BUF_DROP_FULL_SCAN_THRESHOLD (uint64) (NBuffers / 32)
-typedef struct PrivateRefCountEntry
+typedef struct PrivateRefCountData
{
- Buffer buffer;
+ /*
+ * How many times has the buffer been pinned by this backend.
+ */
int32 refcount;
+} PrivateRefCountData;
+
+typedef struct PrivateRefCountEntry
+{
+ Buffer buffer;
+ PrivateRefCountData data;
} PrivateRefCountEntry;
/* 64 bytes, about the size of a cache line on common systems */
@@ -212,11 +220,12 @@ static BufferDesc *PinCountWaitBuf = NULL;
* memory allocations in NewPrivateRefCountEntry() which can be important
* because in some scenarios it's called with a spinlock held...
*/
+static Buffer PrivateRefCountArrayKeys[REFCOUNT_ARRAY_ENTRIES];
static struct PrivateRefCountEntry PrivateRefCountArray[REFCOUNT_ARRAY_ENTRIES];
static HTAB *PrivateRefCountHash = NULL;
static int32 PrivateRefCountOverflowed = 0;
static uint32 PrivateRefCountClock = 0;
-static PrivateRefCountEntry *ReservedRefCountEntry = NULL;
+static int ReservedRefCountSlot = -1;
static uint32 MaxProportionalPins;
@@ -259,7 +268,7 @@ static void
ReservePrivateRefCountEntry(void)
{
/* Already reserved (or freed), nothing to do */
- if (ReservedRefCountEntry != NULL)
+ if (ReservedRefCountSlot != -1)
return;
/*
@@ -271,16 +280,19 @@ ReservePrivateRefCountEntry(void)
for (i = 0; i < REFCOUNT_ARRAY_ENTRIES; i++)
{
- PrivateRefCountEntry *res;
-
- res = &PrivateRefCountArray[i];
-
- if (res->buffer == InvalidBuffer)
+ if (PrivateRefCountArrayKeys[i] == InvalidBuffer)
{
- ReservedRefCountEntry = res;
- return;
+ ReservedRefCountSlot = i;
+
+ /*
+ * We could return immediately, but iterating till the end of
+ * the array allows compiler-autovectorization.
+ */
}
}
+
+ if (ReservedRefCountSlot != -1)
+ return;
}
/*
@@ -292,27 +304,34 @@ ReservePrivateRefCountEntry(void)
* Move entry from the current clock position in the array into the
* hashtable. Use that slot.
*/
+ int victim_slot;
+ PrivateRefCountEntry *victim_entry;
PrivateRefCountEntry *hashent;
bool found;
/* select victim slot */
- ReservedRefCountEntry =
- &PrivateRefCountArray[PrivateRefCountClock++ % REFCOUNT_ARRAY_ENTRIES];
+ victim_slot = PrivateRefCountClock++ % REFCOUNT_ARRAY_ENTRIES;
+ victim_entry = &PrivateRefCountArray[victim_slot];
+ ReservedRefCountSlot = victim_slot;
/* Better be used, otherwise we shouldn't get here. */
- Assert(ReservedRefCountEntry->buffer != InvalidBuffer);
+ Assert(PrivateRefCountArrayKeys[victim_slot] != InvalidBuffer);
+ Assert(PrivateRefCountArray[victim_slot].buffer != InvalidBuffer);
+ Assert(PrivateRefCountArrayKeys[victim_slot] == PrivateRefCountArray[victim_slot].buffer);
/* enter victim array entry into hashtable */
hashent = hash_search(PrivateRefCountHash,
- &(ReservedRefCountEntry->buffer),
+ &PrivateRefCountArrayKeys[victim_slot],
HASH_ENTER,
&found);
Assert(!found);
- hashent->refcount = ReservedRefCountEntry->refcount;
+ hashent->data = victim_entry->data;
/* clear the now free array slot */
- ReservedRefCountEntry->buffer = InvalidBuffer;
- ReservedRefCountEntry->refcount = 0;
+ PrivateRefCountArrayKeys[victim_slot] = InvalidBuffer;
+ victim_entry->buffer = InvalidBuffer;
+ memset(&victim_entry->data, 0, sizeof(victim_entry->data));
+ victim_entry->data.refcount = 0;
PrivateRefCountOverflowed++;
}
@@ -327,15 +346,17 @@ NewPrivateRefCountEntry(Buffer buffer)
PrivateRefCountEntry *res;
/* only allowed to be called when a reservation has been made */
- Assert(ReservedRefCountEntry != NULL);
+ Assert(ReservedRefCountSlot != -1);
/* use up the reserved entry */
- res = ReservedRefCountEntry;
- ReservedRefCountEntry = NULL;
+ res = &PrivateRefCountArray[ReservedRefCountSlot];
/* and fill it */
+ PrivateRefCountArrayKeys[ReservedRefCountSlot] = buffer;
res->buffer = buffer;
- res->refcount = 0;
+ res->data.refcount = 0;
+
+ ReservedRefCountSlot = -1;
return res;
}
@@ -347,10 +368,11 @@ NewPrivateRefCountEntry(Buffer buffer)
* do_move is true, and the entry resides in the hashtable the entry is
* optimized for frequent access by moving it to the array.
*/
-static PrivateRefCountEntry *
+static inline PrivateRefCountEntry *
GetPrivateRefCountEntry(Buffer buffer, bool do_move)
{
PrivateRefCountEntry *res;
+ int match = -1;
int i;
Assert(BufferIsValid(buffer));
@@ -362,12 +384,16 @@ GetPrivateRefCountEntry(Buffer buffer, bool do_move)
*/
for (i = 0; i < REFCOUNT_ARRAY_ENTRIES; i++)
{
- res = &PrivateRefCountArray[i];
-
- if (res->buffer == buffer)
- return res;
+ if (PrivateRefCountArrayKeys[i] == buffer)
+ {
+ match = i;
+ /* see ReservePrivateRefCountEntry() for why we don't return */
+ }
}
+ if (match != -1)
+ return &PrivateRefCountArray[match];
+
/*
* By here we know that the buffer, if already pinned, isn't residing in
* the array.
@@ -397,14 +423,18 @@ GetPrivateRefCountEntry(Buffer buffer, bool do_move)
ReservePrivateRefCountEntry();
/* Use up the reserved slot */
- Assert(ReservedRefCountEntry != NULL);
- free = ReservedRefCountEntry;
- ReservedRefCountEntry = NULL;
+ Assert(ReservedRefCountSlot != -1);
+ free = &PrivateRefCountArray[ReservedRefCountSlot];
+ Assert(PrivateRefCountArrayKeys[ReservedRefCountSlot] == free->buffer);
Assert(free->buffer == InvalidBuffer);
/* and fill it */
free->buffer = buffer;
- free->refcount = res->refcount;
+ free->data = res->data;
+ PrivateRefCountArrayKeys[ReservedRefCountSlot] = buffer;
+
+ ReservedRefCountSlot = -1;
+
/* delete from hashtable */
hash_search(PrivateRefCountHash, &buffer, HASH_REMOVE, &found);
@@ -437,7 +467,7 @@ GetPrivateRefCount(Buffer buffer)
if (ref == NULL)
return 0;
- return ref->refcount;
+ return ref->data.refcount;
}
/*
@@ -447,19 +477,21 @@ GetPrivateRefCount(Buffer buffer)
static void
ForgetPrivateRefCountEntry(PrivateRefCountEntry *ref)
{
- Assert(ref->refcount == 0);
+ Assert(ref->data.refcount == 0);
if (ref >= &PrivateRefCountArray[0] &&
ref < &PrivateRefCountArray[REFCOUNT_ARRAY_ENTRIES])
{
ref->buffer = InvalidBuffer;
+ PrivateRefCountArrayKeys[ref - PrivateRefCountArray] = InvalidBuffer;
+
/*
* Mark the just used entry as reserved - in many scenarios that
* allows us to avoid ever having to search the array/hash for free
* entries.
*/
- ReservedRefCountEntry = ref;
+ ReservedRefCountSlot = ref - PrivateRefCountArray;
}
else
{
@@ -3073,7 +3105,7 @@ PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy,
PrivateRefCountEntry *ref;
Assert(!BufferIsLocal(b));
- Assert(ReservedRefCountEntry != NULL);
+ Assert(ReservedRefCountSlot != -1);
ref = GetPrivateRefCountEntry(b, true);
@@ -3145,8 +3177,8 @@ PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy,
*/
result = (pg_atomic_read_u64(&buf->state) & BM_VALID) != 0;
- Assert(ref->refcount > 0);
- ref->refcount++;
+ Assert(ref->data.refcount > 0);
+ ref->data.refcount++;
ResourceOwnerRememberBuffer(CurrentResourceOwner, b);
}
@@ -3263,9 +3295,9 @@ UnpinBufferNoOwner(BufferDesc *buf)
/* not moving as we're likely deleting it soon anyway */
ref = GetPrivateRefCountEntry(b, false);
Assert(ref != NULL);
- Assert(ref->refcount > 0);
- ref->refcount--;
- if (ref->refcount == 0)
+ Assert(ref->data.refcount > 0);
+ ref->data.refcount--;
+ if (ref->data.refcount == 0)
{
uint64 old_buf_state;
@@ -3305,7 +3337,7 @@ TrackNewBufferPin(Buffer buf)
PrivateRefCountEntry *ref;
ref = NewPrivateRefCountEntry(buf);
- ref->refcount++;
+ ref->data.refcount++;
ResourceOwnerRememberBuffer(CurrentResourceOwner, buf);
@@ -4018,6 +4050,7 @@ InitBufferManagerAccess(void)
MaxProportionalPins = NBuffers / (MaxBackends + NUM_AUXILIARY_PROCS);
memset(&PrivateRefCountArray, 0, sizeof(PrivateRefCountArray));
+ memset(&PrivateRefCountArrayKeys, 0, sizeof(Buffer));
hash_ctl.keysize = sizeof(int32);
hash_ctl.entrysize = sizeof(PrivateRefCountEntry);
@@ -4067,10 +4100,10 @@ CheckForBufferLeaks(void)
/* check the array */
for (i = 0; i < REFCOUNT_ARRAY_ENTRIES; i++)
{
- res = &PrivateRefCountArray[i];
-
- if (res->buffer != InvalidBuffer)
+ if (PrivateRefCountArrayKeys[i] != InvalidBuffer)
{
+ res = &PrivateRefCountArray[i];
+
s = DebugPrintBufferRefcount(res->buffer);
elog(WARNING, "buffer refcount leak: %s", s);
pfree(s);
@@ -5407,7 +5440,7 @@ IncrBufferRefCount(Buffer buffer)
ref = GetPrivateRefCountEntry(buffer, true);
Assert(ref != NULL);
- ref->refcount++;
+ ref->data.refcount++;
}
ResourceOwnerRememberBuffer(CurrentResourceOwner, buffer);
}
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 5769227e41c..9a89e68c59c 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2327,6 +2327,7 @@ PrintfArgValue
PrintfTarget
PrinttupAttrInfo
PrivTarget
+PrivateRefCountData
PrivateRefCountEntry
ProcArrayStruct
ProcLangInfo
--
2.48.1.76.g4e746b1a31.dirty
[text/x-diff] v6-0007-bufmgr-Add-one-entry-cache-for-private-refcount.patch (1.9K, ../6rgb2nvhyvnszz4ul3wfzlf5rheb2kkwrglthnna7qhe24onwr@vw27225tkyar/8-v6-0007-bufmgr-Add-one-entry-cache-for-private-refcount.patch)
download | inline diff:
From b503f94124620433d1e8276341079c70d037d360 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 18 Nov 2025 09:39:59 -0500
Subject: [PATCH v6 07/14] bufmgr: Add one-entry cache for private refcount
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
src/backend/storage/buffer/bufmgr.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 972eae1fc7b..b7ce4bafdea 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -226,6 +226,8 @@ static HTAB *PrivateRefCountHash = NULL;
static int32 PrivateRefCountOverflowed = 0;
static uint32 PrivateRefCountClock = 0;
static int ReservedRefCountSlot = -1;
+static int PrivateRefcountEntryLast = -1;
+
static uint32 MaxProportionalPins;
@@ -356,6 +358,8 @@ NewPrivateRefCountEntry(Buffer buffer)
res->buffer = buffer;
res->data.refcount = 0;
+ PrivateRefcountEntryLast = ReservedRefCountSlot;
+
ReservedRefCountSlot = -1;
return res;
@@ -378,6 +382,16 @@ GetPrivateRefCountEntry(Buffer buffer, bool do_move)
Assert(BufferIsValid(buffer));
Assert(!BufferIsLocal(buffer));
+ /*
+ * It's very common to look up the same buffer repeatedly. To make that
+ * fast, we have a one-entry cache.
+ */
+ if (likely(PrivateRefcountEntryLast != -1) &&
+ likely(PrivateRefCountArrayKeys[PrivateRefcountEntryLast] == buffer))
+ {
+ return &PrivateRefCountArray[PrivateRefcountEntryLast];
+ }
+
/*
* First search for references in the array, that'll be sufficient in the
* majority of cases.
@@ -392,7 +406,10 @@ GetPrivateRefCountEntry(Buffer buffer, bool do_move)
}
if (match != -1)
+ {
+ PrivateRefcountEntryLast = match;
return &PrivateRefCountArray[match];
+ }
/*
* By here we know that the buffer, if already pinned, isn't residing in
--
2.48.1.76.g4e746b1a31.dirty
[text/x-diff] v6-0008-bufmgr-Implement-buffer-content-locks-independent.patch (39.9K, ../6rgb2nvhyvnszz4ul3wfzlf5rheb2kkwrglthnna7qhe24onwr@vw27225tkyar/9-v6-0008-bufmgr-Implement-buffer-content-locks-independent.patch)
download | inline diff:
From 905c67a8387286bbd39ebca49c2b127403de4e80 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 19 Nov 2025 16:37:26 -0500
Subject: [PATCH v6 08/14] bufmgr: Implement buffer content locks independently
of lwlocks
Until now buffer content locks were implemented using lwlocks. That has the
obvious advantage of not needing a separate efficient implementation of
locks. However, the time for a dedicated buffer content lock implementation
has come:
1) Hint bits are currently set while holding only a share lock. This leads to
having to copy pages while they are being written out if checksums are
enabled, which is not cheap. We would like to add AIO writes, however once
many buffers can be written out at the same time, it gets a lot more
expensive to copy them, particularly because that copy needs to reside in
shared buffers (for worker mode to have access to the buffer).
In addition, modifying buffers while they are being written out can cause
issues with unbuffered/direct-IO, as some filesystems (like btrfs) do not
like that, due to filesystem internal checksums getting corrupted.
The solution to this is to require a new share-exclusive lock-level to set
hint bits and to write out buffers, making those operations mutually
exclusive. We could introduce such a lock level into the generic lwlock
implementation, however it does not look like there would be other users,
and it does add some overhead into important codepaths.
2) For AIO writes we need to be able to race-freely check whether a buffer is
undergoing IO and whether an exclusive lock on the page can be acquired. That
is rather hard to do efficiently when the buffer state and the lock state
are separate atomic variables. This is a major hindrance to allowing writes
to be done asynchronously.
3) Buffer locks are by far the most frequently taken locks. Optimizing them
specifically for their use case is worth the effort. E.g. by merging
content locks into buffer locks we will be able to release a buffer lock
and pin in one atomic operation.
4) There are more complicated optimizations, like long-lived "super pinned &
locked" pages, that cannot realistically be implemented with the generic
lwlock implementation.
Therefore implement content locks inside bufmgr.c. The lockstate is stored as
part of BufferDesc.state. The implementation of buffer content locks is fairly
similar to lwlocks, with a few important differences:
1) An additional lock-level share-exclusive has been added. This lock level
conflicts with exclusive locks and itself, but not share locks.
2) Error recovery for content locks is implemented as part of the already
existing private-refcount tracking mechanism in combination with resowners,
instead of a bespoke mechanism as the case for lwlocks. This means we do
not need to add dedicated error-recovery codepaths to release all content
locks (like done with LWLockReleaseAll() for content locks).
3) The lock state is embedded in BufferDesc.state instead of having its own
struct.
4) The wakeup logic is a tad more complicated due to needing to support the
additional lock level
This commit unfortunately introduces some code that is very similar to the
code in lwlock.c, however the code is not equivalent enough to easily merge
it. The future wins that this commit makes possible seem worth the cost.
As of this commit nothing uses the new share-exclusive lock mode. It will be
documented and used in a future commit. It seemed too complicated to introduce
the lock-level in a separate commit.
TODO:
- Address FIXMEs
- Perhaps move the locking code into a buffer_locking.h or such? Needs to be
inline functions for efficiency unfortunately.
- reflow some comments that I didn't reflow to make the diff more readable
Discussion: https://postgr.es/m/fvfmkr5kk4nyex56ejgxj3uzi63isfxovp2biecb4bspbjrze7@az2pljabhnff
---
src/include/storage/buf_internals.h | 55 +-
src/include/storage/bufmgr.h | 20 +-
src/include/storage/proc.h | 8 +-
src/backend/postmaster/auxprocess.c | 1 +
src/backend/storage/buffer/buf_init.c | 7 +-
src/backend/storage/buffer/bufmgr.c | 776 ++++++++++++++++--
.../utils/activity/wait_event_names.txt | 3 +
7 files changed, 804 insertions(+), 66 deletions(-)
diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h
index 28519ad2813..0a145d95024 100644
--- a/src/include/storage/buf_internals.h
+++ b/src/include/storage/buf_internals.h
@@ -23,6 +23,7 @@
#include "storage/condition_variable.h"
#include "storage/lwlock.h"
#include "storage/procnumber.h"
+#include "storage/proclist_types.h"
#include "storage/shmem.h"
#include "storage/smgr.h"
#include "storage/spin.h"
@@ -32,22 +33,29 @@
/*
* Buffer state is a single 64-bit variable where following data is combined.
*
+ * State of the buffer itself:
* - 18 bits refcount
* - 4 bits usage count
* - 10 bits of flags
*
+ * State of the content lock:
+ * - 1 bit has_waiter
+ * - 1 bit release_ok
+ * - 1 bit lock state locked
+ * - 1 bit exclusively locked
+ * - 1 bit share exclusively locked
+ * - 18 bits share lock count
+ *
* Combining these values allows to perform some operations without locking
* the buffer header, by modifying them together with a CAS loop.
*
- * NB: A future commit will use a significant portion of the remaining bits to
- * implement buffer locking as part of the state variable.
- *
* The definition of buffer state components is below.
*/
#define BUF_REFCOUNT_BITS 18
#define BUF_USAGECOUNT_BITS 4
#define BUF_FLAG_BITS 10
+/* FIXME: Also assert lock state size, just not yet sure how */
StaticAssertDecl(BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS + BUF_FLAG_BITS == 32,
"parts of buffer state space need to equal 32");
@@ -69,7 +77,7 @@ StaticAssertDecl(BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS + BUF_FLAG_BITS == 32,
((uint32)(((state) & BUF_USAGECOUNT_MASK) >> BUF_USAGECOUNT_SHIFT))
/*
- * Flags for buffer descriptors
+ * Flags for buffer descriptor state
*
* Note: BM_TAG_VALID essentially means that there is a buffer hashtable
* entry associated with the buffer's tag.
@@ -111,6 +119,20 @@ StaticAssertDecl(BM_MAX_USAGE_COUNT < (UINT64CONST(1) << BUF_USAGECOUNT_BITS),
StaticAssertDecl(MAX_BACKENDS_BITS <= BUF_REFCOUNT_BITS,
"MAX_BACKENDS_BITS needs to be <= BUF_REFCOUNT_BITS");
+
+/*
+ * Definitions related to buffer content locks
+ */
+#define BM_LOCK_HAS_WAITERS (UINT64CONST(1) << 63)
+#define BM_LOCK_RELEASE_OK (UINT64CONST(1) << 62)
+
+#define BM_LOCK_VAL_SHARED (UINT64CONST(1) << 32)
+#define BM_LOCK_VAL_SHARE_EXCLUSIVE (UINT64CONST(1) << (32 + MAX_BACKENDS_BITS))
+#define BM_LOCK_VAL_EXCLUSIVE (UINT64CONST(1) << (32 + 1 + MAX_BACKENDS_BITS))
+
+#define BM_LOCK_MASK (((uint64)MAX_BACKENDS << 32) | BM_LOCK_VAL_SHARE_EXCLUSIVE | BM_LOCK_VAL_EXCLUSIVE)
+
+
/*
* Buffer tag identifies which disk block the buffer contains.
*
@@ -253,9 +275,6 @@ BufMappingPartitionLockByIndex(uint32 index)
* it is held. However, existing buffer pins may be released while the buffer
* header spinlock is held, using an atomic subtraction.
*
- * The LWLock can take care of itself. The buffer header lock is *not* used
- * to control access to the data in the buffer!
- *
* If we have the buffer pinned, its tag can't change underneath us, so we can
* examine the tag without locking the buffer header. Also, in places we do
* one-time reads of the flags without bothering to lock the buffer header;
@@ -268,6 +287,15 @@ BufMappingPartitionLockByIndex(uint32 index)
* wait_backend_pgprocno and setting flag bit BM_PIN_COUNT_WAITER. At present,
* there can be only one such waiter per buffer.
*
+ * The content of buffers is protected via the buffer content lock,
+ * implemented as part buffer state. Note that the buffer header lock is *not*
+ * used to control access to the data in the buffer! We used to use an LWLock
+ * to implement the content lock, but having a dedicated implementation of
+ * content locks allows to implement some otherwise hard things (e.g.
+ * race-freely checking if AIO is in progress before locking a buffer
+ * exclusively) and makes otherwise impossible optimizations possible
+ * (e.g. unlocking and unpinning a buffer in one atomic operation).
+ *
* We use this same struct for local buffer headers, but the locks are not
* used and not all of the flag bits are useful either. To avoid unnecessary
* overhead, manipulations of the state field should be done without actual
@@ -309,7 +337,12 @@ typedef struct BufferDesc
int wait_backend_pgprocno;
PgAioWaitRef io_wref; /* set iff AIO is in progress */
- LWLock content_lock; /* to lock access to buffer contents */
+
+ /*
+ * List of PGPROCs waiting for the buffer content lock. Protected by the
+ * buffer header spinlock.
+ */
+ proclist_head lock_waiters;
} BufferDesc;
/*
@@ -396,12 +429,6 @@ BufferDescriptorGetIOCV(const BufferDesc *bdesc)
return &(BufferIOCVArray[bdesc->buf_id]).cv;
}
-static inline LWLock *
-BufferDescriptorGetContentLock(const BufferDesc *bdesc)
-{
- return (LWLock *) (&bdesc->content_lock);
-}
-
/*
* Functions for acquiring/releasing a shared buffer header's spinlock. Do
* not apply these to local buffers!
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index 5fc3de20abc..8e442492d4d 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -204,6 +204,7 @@ typedef enum BufferLockMode
{
BUFFER_LOCK_UNLOCK,
BUFFER_LOCK_SHARE,
+ BUFFER_LOCK_SHARE_EXCLUSIVE,
BUFFER_LOCK_EXCLUSIVE,
} BufferLockMode;
@@ -302,7 +303,24 @@ extern void BufferGetTag(Buffer buffer, RelFileLocator *rlocator,
extern void MarkBufferDirtyHint(Buffer buffer, bool buffer_std);
extern void UnlockBuffers(void);
-extern void LockBuffer(Buffer buffer, BufferLockMode mode);
+extern void UnlockBuffer(Buffer buffer);
+extern void LockBufferInternal(Buffer buffer, BufferLockMode mode);
+
+/*
+ * Handling BUFFER_LOCK_UNLOCK in bufmgr.c leads to sufficiently worse branch
+ * prediction to impact performance. Therefore handle that switch here, were
+ * most of the time `mode` will be a constant and thus can be optimized out by
+ * the compiler.
+ */
+static inline void
+LockBuffer(Buffer buffer, BufferLockMode mode)
+{
+ if (mode == BUFFER_LOCK_UNLOCK)
+ UnlockBuffer(buffer);
+ else
+ LockBufferInternal(buffer, mode);
+}
+
extern bool ConditionalLockBuffer(Buffer buffer);
extern void LockBufferForCleanup(Buffer buffer);
extern bool ConditionalLockBufferForCleanup(Buffer buffer);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index c6f5ebceefd..d1f6d314d57 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -242,7 +242,13 @@ struct PGPROC
*/
bool recoveryConflictPending;
- /* Info about LWLock the process is currently waiting for, if any. */
+ /*
+ * Info about LWLock the process is currently waiting for, if any.
+ *
+ * This is currently used both for lwlocks and buffer content locks, which
+ * is acceptable, although not pretty, because a backend can't wait for
+ * both types of locks at the same time.
+ */
uint8 lwWaiting; /* see LWLockWaitState */
uint8 lwWaitMode; /* lwlock mode being waited for */
proclist_node lwWaitLink; /* position in LW lock wait list */
diff --git a/src/backend/postmaster/auxprocess.c b/src/backend/postmaster/auxprocess.c
index a6d3630398f..2012f959fd9 100644
--- a/src/backend/postmaster/auxprocess.c
+++ b/src/backend/postmaster/auxprocess.c
@@ -18,6 +18,7 @@
#include "miscadmin.h"
#include "pgstat.h"
#include "postmaster/auxprocess.h"
+#include "storage/bufmgr.h"
#include "storage/condition_variable.h"
#include "storage/ipc.h"
#include "storage/proc.h"
diff --git a/src/backend/storage/buffer/buf_init.c b/src/backend/storage/buffer/buf_init.c
index 25f71191ec3..f3224c793c4 100644
--- a/src/backend/storage/buffer/buf_init.c
+++ b/src/backend/storage/buffer/buf_init.c
@@ -17,6 +17,7 @@
#include "storage/aio.h"
#include "storage/buf_internals.h"
#include "storage/bufmgr.h"
+#include "storage/proclist.h"
BufferDescPadded *BufferDescriptors;
char *BufferBlocks;
@@ -121,16 +122,14 @@ BufferManagerShmemInit(void)
ClearBufferTag(&buf->tag);
- pg_atomic_init_u64(&buf->state, 0);
+ pg_atomic_init_u64(&buf->state, BM_LOCK_RELEASE_OK);
buf->wait_backend_pgprocno = INVALID_PROC_NUMBER;
buf->buf_id = i;
pgaio_wref_clear(&buf->io_wref);
- LWLockInitialize(BufferDescriptorGetContentLock(buf),
- LWTRANCHE_BUFFER_CONTENT);
-
+ proclist_init(&buf->lock_waiters);
ConditionVariableInit(BufferDescriptorGetIOCV(buf));
}
}
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index b7ce4bafdea..da83b775d0b 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -58,6 +58,7 @@
#include "storage/ipc.h"
#include "storage/lmgr.h"
#include "storage/proc.h"
+#include "storage/proclist.h"
#include "storage/read_stream.h"
#include "storage/smgr.h"
#include "storage/standby.h"
@@ -96,6 +97,12 @@ typedef struct PrivateRefCountData
* How many times has the buffer been pinned by this backend.
*/
int32 refcount;
+
+ /*
+ * Is the buffer locked by this backend? BUFFER_LOCK_UNLOCK indicates that
+ * the buffer is not locked.
+ */
+ BufferLockMode lockmode;
} PrivateRefCountData;
typedef struct PrivateRefCountEntry
@@ -334,6 +341,7 @@ ReservePrivateRefCountEntry(void)
victim_entry->buffer = InvalidBuffer;
memset(&victim_entry->data, 0, sizeof(victim_entry->data));
victim_entry->data.refcount = 0;
+ victim_entry->data.lockmode = BUFFER_LOCK_UNLOCK;
PrivateRefCountOverflowed++;
}
@@ -357,6 +365,7 @@ NewPrivateRefCountEntry(Buffer buffer)
PrivateRefCountArrayKeys[ReservedRefCountSlot] = buffer;
res->buffer = buffer;
res->data.refcount = 0;
+ res->data.lockmode = BUFFER_LOCK_UNLOCK;
PrivateRefcountEntryLast = ReservedRefCountSlot;
@@ -495,6 +504,7 @@ static void
ForgetPrivateRefCountEntry(PrivateRefCountEntry *ref)
{
Assert(ref->data.refcount == 0);
+ Assert(ref->data.lockmode == BUFFER_LOCK_UNLOCK);
if (ref >= &PrivateRefCountArray[0] &&
ref < &PrivateRefCountArray[REFCOUNT_ARRAY_ENTRIES])
@@ -604,6 +614,20 @@ static inline int buffertag_comparator(const BufferTag *ba, const BufferTag *bb)
static inline int ckpt_buforder_comparator(const CkptSortItem *a, const CkptSortItem *b);
static int ts_ckpt_progress_comparator(Datum a, Datum b, void *arg);
+static void BufferLockAcquire(Buffer buffer, BufferDesc *buf_hdr, BufferLockMode mode);
+static void BufferLockUnlock(Buffer buffer, BufferDesc *buf_hdr);
+static bool BufferLockConditional(Buffer buffer, BufferDesc *buf_hdr, BufferLockMode mode);
+static bool BufferLockHeldByMeInMode(BufferDesc *buf_hdr, BufferLockMode mode);
+static bool BufferLockHeldByMe(BufferDesc *buf_hdr);
+static inline void BufferLockDisown(Buffer buffer, BufferDesc *buf_hdr);
+static inline int BufferLockDisownInternal(Buffer buffer, BufferDesc *buf_hdr);
+static inline bool BufferLockAttempt(BufferDesc *buf_hdr, BufferLockMode mode);
+static void BufferLockQueueSelf(BufferDesc *buf_hdr, BufferLockMode mode);
+static void BufferLockDequeueSelf(BufferDesc *buf_hdr);
+static void BufferLockWakeup(BufferDesc *buf_hdr, bool unlocked);
+static void BufferLockProcessRelease(BufferDesc *buf_hdr, BufferLockMode mode, uint64 lockstate);
+static inline uint64 BufferLockReleaseSub(BufferLockMode mode);
+
/*
* Implementation of PrefetchBuffer() for shared buffers.
@@ -2404,8 +2428,6 @@ again:
*/
if (buf_state & BM_DIRTY)
{
- LWLock *content_lock;
-
Assert(buf_state & BM_TAG_VALID);
Assert(buf_state & BM_VALID);
@@ -2423,8 +2445,7 @@ again:
* one just happens to be trying to split the page the first one got
* from StrategyGetBuffer.)
*/
- content_lock = BufferDescriptorGetContentLock(buf_hdr);
- if (!LWLockConditionalAcquire(content_lock, LW_SHARED))
+ if (!BufferLockConditional(buf, buf_hdr, BUFFER_LOCK_SHARE))
{
/*
* Someone else has locked the buffer, so give it up and loop back
@@ -2453,7 +2474,7 @@ again:
if (XLogNeedsFlush(lsn)
&& StrategyRejectBuffer(strategy, buf_hdr, from_ring))
{
- LWLockRelease(content_lock);
+ LockBuffer(buf, BUFFER_LOCK_UNLOCK);
UnpinBuffer(buf_hdr);
goto again;
}
@@ -2461,7 +2482,7 @@ again:
/* OK, do the I/O */
FlushBuffer(buf_hdr, NULL, IOOBJECT_RELATION, io_context);
- LWLockRelease(content_lock);
+ LockBuffer(buf, BUFFER_LOCK_UNLOCK);
ScheduleBufferTagForWriteback(&BackendWritebackContext, io_context,
&buf_hdr->tag);
@@ -2903,7 +2924,7 @@ BufferIsLockedByMe(Buffer buffer)
else
{
bufHdr = GetBufferDescriptor(buffer - 1);
- return LWLockHeldByMe(BufferDescriptorGetContentLock(bufHdr));
+ return BufferLockHeldByMe(bufHdr);
}
}
@@ -2928,23 +2949,8 @@ BufferIsLockedByMeInMode(Buffer buffer, BufferLockMode mode)
}
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_mode);
+ return BufferLockHeldByMeInMode(bufHdr, mode);
}
}
@@ -3331,7 +3337,7 @@ UnpinBufferNoOwner(BufferDesc *buf)
* 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)));
+ Assert(!BufferLockHeldByMe(buf));
/* decrement the shared reference count */
old_buf_state = pg_atomic_fetch_sub_u64(&buf->state, BUF_REFCOUNT_ONE);
@@ -4176,6 +4182,8 @@ static void
AssertNotCatalogBufferLock(LWLock *lock, LWLockMode mode,
void *unused_context)
{
+ /* FIXME */
+#ifdef NOT_YET
BufferDesc *bufHdr;
BufferTag tag;
Oid relid;
@@ -4205,6 +4213,7 @@ AssertNotCatalogBufferLock(LWLock *lock, LWLockMode mode,
return;
Assert(!IsCatalogRelationOid(relid));
+#endif
}
#endif
@@ -4470,9 +4479,11 @@ static void
FlushUnlockedBuffer(BufferDesc *buf, SMgrRelation reln,
IOObject io_object, IOContext io_context)
{
- LWLockAcquire(BufferDescriptorGetContentLock(buf), LW_SHARED);
+ Buffer buffer = BufferDescriptorGetBuffer(buf);
+
+ BufferLockAcquire(buffer, buf, BUFFER_LOCK_SHARE);
FlushBuffer(buf, reln, IOOBJECT_RELATION, IOCONTEXT_NORMAL);
- LWLockRelease(BufferDescriptorGetContentLock(buf));
+ BufferLockUnlock(buffer, buf);
}
/*
@@ -5615,9 +5626,10 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
*
* Used to clean up after errors.
*
- * Currently, we can expect that lwlock.c's LWLockReleaseAll() took care
- * of releasing buffer content locks per se; the only thing we need to deal
- * with here is clearing any PIN_COUNT request that was in progress.
+ * Currently, we can expect that resource owner cleanup, via
+ * ResOwnerReleaseBufferPin(), took care releasing buffer content locks per
+ * se; the only thing we need to deal with here is clearing any PIN_COUNT
+ * request that was in progress.
*/
void
UnlockBuffers(void)
@@ -5648,25 +5660,684 @@ UnlockBuffers(void)
}
/*
- * Acquire or release the content_lock for the buffer.
+ * Acquire the buffer content lock in the specified mode
+ *
+ * If the lock is not available, sleep until it is.
+ *
+ * Side effect: cancel/die interrupts are held off until lock release.
+ *
+ * This uses almost the same locking approach as lwlock.c's
+ * LWLockAcquire(). See documentation atop of lwlock.c for a more detailed
+ * discussion.
+ *
+ * The reason that this, and most of the other BufferLock* functions, get both
+ * the Buffer and BufferDesc* as parameters, is that looking up one from the
+ * other repeatedly shows up noticeably in profiles.
+ */
+static inline void
+BufferLockAcquire(Buffer buffer, BufferDesc *buf_hdr, BufferLockMode mode)
+{
+ PrivateRefCountEntry *entry;
+ int extraWaits = 0;
+
+ /*
+ * Get reference to the refcount entry before we hold the lock, it seems
+ * better to do before holding the lock.
+ */
+ entry = GetPrivateRefCountEntry(buffer, true);
+
+ /*
+ * Lock out cancel/die interrupts until we exit the code section protected
+ * by the content lock. This ensures that interrupts will not interfere
+ * with manipulations of data structures in shared memory.
+ */
+ HOLD_INTERRUPTS();
+
+ for (;;)
+ {
+ bool mustwait;
+ uint32 wait_event;
+
+ /*
+ * Try to grab the lock the first time, we're not in the waitqueue
+ * yet/anymore.
+ */
+ mustwait = BufferLockAttempt(buf_hdr, mode);
+
+ if (likely(!mustwait))
+ {
+ break;
+ }
+
+ /*
+ * Ok, at this point we couldn't grab the lock on the first try. We
+ * cannot simply queue ourselves to the end of the list and wait to be
+ * woken up because by now the lock could long have been released.
+ * Instead add us to the queue and try to grab the lock again. If we
+ * succeed we need to revert the queuing and be happy, otherwise we
+ * recheck the lock. If we still couldn't grab it, we know that the
+ * other locker will see our queue entries when releasing since they
+ * existed before we checked for the lock.
+ */
+
+ /* add to the queue */
+ BufferLockQueueSelf(buf_hdr, mode);
+
+ /* we're now guaranteed to be woken up if necessary */
+ mustwait = BufferLockAttempt(buf_hdr, mode);
+
+ /* ok, grabbed the lock the second time round, need to undo queueing */
+ if (!mustwait)
+ {
+ BufferLockDequeueSelf(buf_hdr);
+ break;
+ }
+
+ switch (mode)
+ {
+ case BUFFER_LOCK_EXCLUSIVE:
+ wait_event = WAIT_EVENT_BUFFER_EXCLUSIVE;
+ break;
+ case BUFFER_LOCK_SHARE_EXCLUSIVE:
+ wait_event = WAIT_EVENT_BUFFER_SHARE_EXCLUSIVE;
+ break;
+ case BUFFER_LOCK_SHARE:
+ wait_event = WAIT_EVENT_BUFFER_SHARED;
+ break;
+ case BUFFER_LOCK_UNLOCK:
+ pg_unreachable();
+
+ }
+ pgstat_report_wait_start(wait_event);
+
+ /*
+ * Wait until awakened.
+ *
+ * It is possible that we get awakened for a reason other than being
+ * signaled by LWLockRelease. If so, loop back and wait again. Once
+ * we've gotten the LWLock, re-increment the sema by the number of
+ * additional signals received.
+ */
+ for (;;)
+ {
+ PGSemaphoreLock(MyProc->sem);
+ if (MyProc->lwWaiting == LW_WS_NOT_WAITING)
+ break;
+ extraWaits++;
+ }
+
+ pgstat_report_wait_end();
+
+ /* Retrying, allow BufferLockRelease to release waiters again. */
+ pg_atomic_fetch_or_u64(&buf_hdr->state, BM_LOCK_RELEASE_OK);
+ }
+
+ /* Remember that we now hold this lock */
+ entry->data.lockmode = mode;
+
+ /*
+ * Fix the process wait semaphore's count for any absorbed wakeups.
+ */
+ while (unlikely(extraWaits-- > 0))
+ PGSemaphoreUnlock(MyProc->sem);
+}
+
+/*
+ * Release a previously acquired buffer content lock.
+ */
+static void
+BufferLockUnlock(Buffer buffer, BufferDesc *buf_hdr)
+{
+ BufferLockMode mode;
+ uint64 oldstate;
+ uint64 sub;
+
+ mode = BufferLockDisownInternal(buffer, buf_hdr);
+
+ /*
+ * Release my hold on lock, after that it can immediately be acquired by
+ * others, even if we still have to wakeup other waiters.
+ */
+ sub = BufferLockReleaseSub(mode);
+
+ oldstate = pg_atomic_sub_fetch_u64(&buf_hdr->state, sub);
+
+ BufferLockProcessRelease(buf_hdr, mode, oldstate);
+
+ /*
+ * Now okay to allow cancel/die interrupts.
+ */
+ RESUME_INTERRUPTS();
+}
+
+
+/*
+ * Acquire the content lock for the buffer, but only if we don't have to wait.
+ */
+static bool
+BufferLockConditional(Buffer buffer, BufferDesc *buf_hdr, BufferLockMode mode)
+{
+ bool mustwait;
+
+ /*
+ * Lock out cancel/die interrupts until we exit the code section protected
+ * by the content lock. This ensures that interrupts will not interfere
+ * with manipulations of data structures in shared memory.
+ */
+ HOLD_INTERRUPTS();
+
+ /* Check for the lock */
+ mustwait = BufferLockAttempt(buf_hdr, mode);
+
+ if (mustwait)
+ {
+ /* Failed to get lock, so release interrupt holdoff */
+ RESUME_INTERRUPTS();
+ }
+ else
+ {
+ PrivateRefCountEntry *entry =
+ GetPrivateRefCountEntry(buffer, true);
+
+ entry->data.lockmode = mode;
+ }
+
+ return !mustwait;
+}
+
+/*
+ * Internal function that tries to atomically acquire the content lock in the
+ * passed in mode.
+ *
+ * This function will not block waiting for a lock to become free - that's the
+ * caller's job.
+ *
+ * Similar to LWLockAttemptLock().
+ */
+static inline bool
+BufferLockAttempt(BufferDesc *buf_hdr, BufferLockMode mode)
+{
+ uint64 old_state;
+
+ /*
+ * Read once outside the loop, later iterations will get the newer value
+ * via compare & exchange.
+ */
+ old_state = pg_atomic_read_u64(&buf_hdr->state);
+
+ /* loop until we've determined whether we could acquire the lock or not */
+ while (true)
+ {
+ uint64 desired_state;
+ bool lock_free;
+
+ desired_state = old_state;
+
+ if (mode == BUFFER_LOCK_EXCLUSIVE)
+ {
+ lock_free = (old_state & BM_LOCK_MASK) == 0;
+ if (lock_free)
+ desired_state += BM_LOCK_VAL_EXCLUSIVE;
+ }
+ else if (mode == BUFFER_LOCK_SHARE_EXCLUSIVE)
+ {
+ lock_free = (old_state & (BM_LOCK_VAL_EXCLUSIVE | BM_LOCK_VAL_SHARE_EXCLUSIVE)) == 0;
+ if (lock_free)
+ desired_state += BM_LOCK_VAL_SHARE_EXCLUSIVE;
+ }
+ else
+ {
+ lock_free = (old_state & BM_LOCK_VAL_EXCLUSIVE) == 0;
+ if (lock_free)
+ desired_state += BM_LOCK_VAL_SHARED;
+ }
+
+ /*
+ * Attempt to swap in the state we are expecting. If we didn't see
+ * lock to be free, that's just the old value. If we saw it as free,
+ * we'll attempt to mark it acquired. The reason that we always swap
+ * in the value is that this doubles as a memory barrier. We could try
+ * to be smarter and only swap in values if we saw the lock as free,
+ * but benchmark haven't shown it as beneficial so far.
+ *
+ * Retry if the value changed since we last looked at it.
+ */
+ if (likely(pg_atomic_compare_exchange_u64(&buf_hdr->state,
+ &old_state, desired_state)))
+ {
+ if (lock_free)
+ {
+ /* Great! Got the lock. */
+ return false;
+ }
+ else
+ return true; /* somebody else has the lock */
+ }
+ }
+
+ pg_unreachable();
+}
+
+/*
+ * Add ourselves to the end of the content lock's wait queue.
+ */
+static void
+BufferLockQueueSelf(BufferDesc *buf_hdr, BufferLockMode mode)
+{
+ /*
+ * If we don't have a PGPROC structure, there's no way to wait. This
+ * should never occur, since MyProc should only be null during shared
+ * memory initialization.
+ */
+ if (MyProc == NULL)
+ elog(PANIC, "cannot wait without a PGPROC structure");
+
+ if (MyProc->lwWaiting != LW_WS_NOT_WAITING)
+ elog(PANIC, "queueing for lock while waiting on another one");
+
+ LockBufHdr(buf_hdr);
+
+ /* setting the flag is protected by the spinlock */
+ pg_atomic_fetch_or_u64(&buf_hdr->state, BM_LOCK_HAS_WAITERS);
+
+ /*
+ * FIXME: This is reusing the lwlock fields. That's not a correctness
+ * issue, a backend can't wait for both an lwlock and a buffer content
+ * lock at the same time. However, it seems pretty ugly, particularly
+ * given that the field names have an lw* prefix. But duplicating the
+ * fields also seems somewhat superfluous.
+ */
+ MyProc->lwWaiting = LW_WS_WAITING;
+ MyProc->lwWaitMode = mode;
+
+ proclist_push_tail(&buf_hdr->lock_waiters, MyProcNumber, lwWaitLink);
+
+ /* Can release the mutex now */
+ UnlockBufHdr(buf_hdr);
+}
+
+/*
+ * Remove ourselves from the waitlist.
+ *
+ * This is used if we queued ourselves because we thought we needed to sleep
+ * but, after further checking, we discovered that we don't actually need to
+ * do so.
+ */
+static void
+BufferLockDequeueSelf(BufferDesc *buf_hdr)
+{
+ bool on_waitlist;
+
+ LockBufHdr(buf_hdr);
+
+ on_waitlist = MyProc->lwWaiting == LW_WS_WAITING;
+ if (on_waitlist)
+ proclist_delete(&buf_hdr->lock_waiters, MyProcNumber, lwWaitLink);
+
+ if (proclist_is_empty(&buf_hdr->lock_waiters) &&
+ (pg_atomic_read_u64(&buf_hdr->state) & BM_LOCK_HAS_WAITERS) != 0)
+ {
+ pg_atomic_fetch_and_u64(&buf_hdr->state, ~BM_LOCK_HAS_WAITERS);
+ }
+
+ /* XXX: combine with fetch_and above? */
+ UnlockBufHdr(buf_hdr);
+
+ /* clear waiting state again, nice for debugging */
+ if (on_waitlist)
+ MyProc->lwWaiting = LW_WS_NOT_WAITING;
+ else
+ {
+ int extraWaits = 0;
+
+
+ /*
+ * Somebody else dequeued us and has or will wake us up. Deal with the
+ * superfluous absorption of a wakeup.
+ */
+
+ /*
+ * Reset RELEASE_OK flag if somebody woke us before we removed
+ * ourselves - they'll have set it to false.
+ */
+ pg_atomic_fetch_or_u64(&buf_hdr->state, BM_LOCK_RELEASE_OK);
+
+ /*
+ * Now wait for the scheduled wakeup, otherwise our ->lwWaiting would
+ * get reset at some inconvenient point later. Most of the time this
+ * will immediately return.
+ */
+ for (;;)
+ {
+ PGSemaphoreLock(MyProc->sem);
+ if (MyProc->lwWaiting == LW_WS_NOT_WAITING)
+ break;
+ extraWaits++;
+ }
+
+ /*
+ * Fix the process wait semaphore's count for any absorbed wakeups.
+ */
+ while (extraWaits-- > 0)
+ PGSemaphoreUnlock(MyProc->sem);
+ }
+}
+
+/*
+ * Stop treating lock as held by current backend.
+ *
+ * After calling this function it's the callers responsibility to ensure that
+ * the lock gets released, even in case of an error. This only is desirable if
+ * the lock is going to be released in a different process than the process
+ * that acquired it.
+ */
+static inline void
+BufferLockDisown(Buffer buffer, BufferDesc *buf_hdr)
+{
+ BufferLockDisownInternal(buffer, buf_hdr);
+ RESUME_INTERRUPTS();
+}
+
+/*
+ * Stop treating lock as held by current backend.
+ *
+ * This is the code that can be shared between actually releasing a lock
+ * (BufferLockUnlock()) and just not tracking ownership of the lock anymore
+ * without releasing the lock (BufferLockDisown()).
+ */
+static inline int
+BufferLockDisownInternal(Buffer buffer, BufferDesc *buf_hdr)
+{
+ BufferLockMode mode;
+ PrivateRefCountEntry *ref;
+
+ ref = GetPrivateRefCountEntry(buffer, false);
+ if (ref == NULL)
+ elog(ERROR, "lock %d is not held", buffer);
+ mode = ref->data.lockmode;
+ ref->data.lockmode = BUFFER_LOCK_UNLOCK;
+
+ return mode;
+}
+
+/*
+ * Wakeup all the lockers that currently have a chance to acquire the lock.
+ */
+static void
+BufferLockWakeup(BufferDesc *buf_hdr, bool unlocked)
+{
+ bool new_release_ok;
+ bool wake_exclusive = unlocked;
+ bool wake_share_exclusive = true;
+ proclist_head wakeup;
+ proclist_mutable_iter iter;
+
+ proclist_init(&wakeup);
+
+ new_release_ok = true;
+
+ /* lock wait list while collecting backends to wake up */
+ LockBufHdr(buf_hdr);
+
+ proclist_foreach_modify(iter, &buf_hdr->lock_waiters, lwWaitLink)
+ {
+ PGPROC *waiter = GetPGProcByNumber(iter.cur);
+
+ if (!wake_exclusive && waiter->lwWaitMode == BUFFER_LOCK_EXCLUSIVE)
+ continue;
+
+ if (!wake_share_exclusive && waiter->lwWaitMode == BUFFER_LOCK_SHARE_EXCLUSIVE)
+ continue;
+
+ proclist_delete(&buf_hdr->lock_waiters, iter.cur, lwWaitLink);
+ proclist_push_tail(&wakeup, iter.cur, lwWaitLink);
+
+ /*
+ * Prevent additional wakeups until retryer gets to run. Backends that
+ * are just waiting for the lock to become free don't retry
+ * automatically.
+ */
+ new_release_ok = false;
+
+ /*
+ * Don't wakeup further share-exclusive/exclusive lock waiters after
+ * waking a conflicting waiter.
+ */
+ if (waiter->lwWaitMode == BUFFER_LOCK_EXCLUSIVE)
+ {
+ wake_exclusive = false;
+ wake_share_exclusive = false;
+ }
+ else if (waiter->lwWaitMode == BUFFER_LOCK_SHARE_EXCLUSIVE)
+ wake_share_exclusive = false;
+
+ /*
+ * Signal that the process isn't on the wait list anymore. This allows
+ * BufferLockDequeueSelf() to remove itself of the waitlist with a
+ * proclist_delete(), rather than having to check if it has been
+ * removed from the list.
+ */
+ Assert(waiter->lwWaiting == LW_WS_WAITING);
+ waiter->lwWaiting = LW_WS_PENDING_WAKEUP;
+
+ /*
+ * Once we've woken up an exclusive lock, there's no point in waking
+ * up anybody else.
+ */
+ if (waiter->lwWaitMode == BUFFER_LOCK_EXCLUSIVE)
+ break;
+ }
+
+ Assert(proclist_is_empty(&wakeup) || pg_atomic_read_u64(&buf_hdr->state) & BM_LOCK_HAS_WAITERS);
+
+ /* unset required flags, and release lock, in one fell swoop */
+ {
+ uint64 old_state;
+ uint64 desired_state;
+
+ old_state = pg_atomic_read_u64(&buf_hdr->state);
+ while (true)
+ {
+ desired_state = old_state;
+
+ /* compute desired flags */
+
+ if (new_release_ok)
+ desired_state |= BM_LOCK_RELEASE_OK;
+ else
+ desired_state &= ~BM_LOCK_RELEASE_OK;
+
+ if (proclist_is_empty(&buf_hdr->lock_waiters))
+ desired_state &= ~BM_LOCK_HAS_WAITERS;
+
+ desired_state &= ~BM_LOCKED; /* release lock */
+
+ if (pg_atomic_compare_exchange_u64(&buf_hdr->state, &old_state,
+ desired_state))
+ break;
+ }
+ }
+
+ /* Awaken any waiters I removed from the queue. */
+ proclist_foreach_modify(iter, &wakeup, lwWaitLink)
+ {
+ PGPROC *waiter = GetPGProcByNumber(iter.cur);
+
+ proclist_delete(&wakeup, iter.cur, lwWaitLink);
+
+ /*
+ * Guarantee that lwWaiting being unset only becomes visible once the
+ * unlink from the link has completed. Otherwise the target backend
+ * could be woken up for other reason and enqueue for a new lock - if
+ * that happens before the list unlink happens, the list would end up
+ * being corrupted.
+ *
+ * The barrier pairs with the LWLockWaitListLock() when enqueuing for
+ * another lock.
+ */
+ pg_write_barrier();
+ waiter->lwWaiting = LW_WS_NOT_WAITING;
+ PGSemaphoreUnlock(waiter->sem);
+ }
+}
+
+/*
+ * Compute subtraction from buffer state for a release of a held lock in
+ * `mode`.
+ *
+ * This is separated from BufferLockUnlock() as we want to combine the lock
+ * release with other atomic operations when possible, leading to the lock
+ * release being done in multiple places.
+ */
+static inline uint64
+BufferLockReleaseSub(BufferLockMode mode)
+{
+
+ /*
+ * Turns out that a switch() leads gcc to generate sufficiently worse code
+ * for this to show up in profiles...
+ */
+ if (mode == BUFFER_LOCK_EXCLUSIVE)
+ return BM_LOCK_VAL_EXCLUSIVE;
+ else if (mode == BUFFER_LOCK_SHARE_EXCLUSIVE)
+ return BM_LOCK_VAL_SHARE_EXCLUSIVE;
+ else
+ {
+ Assert(mode == BUFFER_LOCK_SHARE);
+ return BM_LOCK_VAL_SHARED;
+ }
+
+ return 0;
+}
+
+/*
+ * Handle work that needs to be done after releasing a lock that was held in
+ * `mode`, where `lockstate` is the result of the atomic operation modifying
+ * the state variable.
+ *
+ * This is separated from BufferLockUnlock() as we want to combine the lock
+ * release with other atomic operations when possible, leading to the lock
+ * release being done in multiple places.
+ */
+static void
+BufferLockProcessRelease(BufferDesc *buf_hdr, BufferLockMode mode, uint64 lockstate)
+{
+ bool check_waiters = false;
+ bool unlocked = false;
+
+ /* nobody else can have that kind of lock */
+ Assert(!(lockstate & BM_LOCK_VAL_EXCLUSIVE));
+
+ /*
+ * We're still waiting for backends to get scheduled, don't wake them up
+ * again.
+ */
+ if ((lockstate & (BM_LOCK_HAS_WAITERS | BM_LOCK_RELEASE_OK)) ==
+ (BM_LOCK_HAS_WAITERS | BM_LOCK_RELEASE_OK))
+ {
+ if ((lockstate & BM_LOCK_MASK) == 0)
+ {
+ check_waiters = true;
+ unlocked = true;
+ }
+ else if (mode == BUFFER_LOCK_SHARE_EXCLUSIVE)
+ {
+ check_waiters = true;
+ unlocked = false;
+ }
+ }
+
+ /*
+ * As waking up waiters requires the spinlock to be acquired, only do so
+ * if necessary.
+ */
+ if (check_waiters)
+ BufferLockWakeup(buf_hdr, unlocked);
+}
+
+/*
+ * BufferLockHeldByMeInMode - test whether my process holds the content lock
+ * in the specified mode
+ *
+ * This is meant as debug support only.
+ */
+static bool
+BufferLockHeldByMeInMode(BufferDesc *buf_hdr, BufferLockMode mode)
+{
+ PrivateRefCountEntry *entry =
+ GetPrivateRefCountEntry(BufferDescriptorGetBuffer(buf_hdr), false);
+
+ if (!entry)
+ return false;
+ else
+ return entry->data.lockmode == mode;
+
+}
+
+/*
+ * BufferLockHeldByMe - test whether my process holds the content lock in any
+ * mode
+ *
+ * This is meant as debug support only.
+ */
+static bool
+BufferLockHeldByMe(BufferDesc *buf_hdr)
+{
+ PrivateRefCountEntry *entry =
+ GetPrivateRefCountEntry(BufferDescriptorGetBuffer(buf_hdr), false);
+
+ if (!entry)
+ return false;
+ else
+ return entry->data.lockmode != BUFFER_LOCK_UNLOCK;
+}
+
+/*
+ * Release the content lock for the buffer.
+ */
+void
+UnlockBuffer(Buffer buffer)
+{
+ BufferDesc *buf_hdr;
+
+ Assert(BufferIsPinned(buffer));
+ if (BufferIsLocal(buffer))
+ return; /* local buffers need no lock */
+
+ buf_hdr = GetBufferDescriptor(buffer - 1);
+ BufferLockUnlock(buffer, buf_hdr);
+}
+
+/*
+ * Acquire the content_lock for the buffer.
*/
void
-LockBuffer(Buffer buffer, BufferLockMode mode)
+LockBufferInternal(Buffer buffer, BufferLockMode mode)
{
- BufferDesc *buf;
+ BufferDesc *buf_hdr;
+
+ /*
+ * We can't wait if we haven't got a PGPROC. This should only occur
+ * during bootstrap or shared memory initialization. Put an Assert here
+ * to catch unsafe coding practices.
+ */
+ Assert(!(MyProc == NULL && IsUnderPostmaster));
+
+ /* handled in LockBuffer() wrapper */
+ Assert(mode != BUFFER_LOCK_UNLOCK);
Assert(BufferIsPinned(buffer));
if (BufferIsLocal(buffer))
return; /* local buffers need no lock */
- buf = GetBufferDescriptor(buffer - 1);
+ buf_hdr = GetBufferDescriptor(buffer - 1);
- if (mode == BUFFER_LOCK_UNLOCK)
- LWLockRelease(BufferDescriptorGetContentLock(buf));
- else if (mode == BUFFER_LOCK_SHARE)
- LWLockAcquire(BufferDescriptorGetContentLock(buf), LW_SHARED);
+ if (mode == BUFFER_LOCK_SHARE)
+ BufferLockAcquire(buffer, buf_hdr, BUFFER_LOCK_SHARE);
+ else if (mode == BUFFER_LOCK_SHARE_EXCLUSIVE)
+ BufferLockAcquire(buffer, buf_hdr, BUFFER_LOCK_SHARE_EXCLUSIVE);
else if (mode == BUFFER_LOCK_EXCLUSIVE)
- LWLockAcquire(BufferDescriptorGetContentLock(buf), LW_EXCLUSIVE);
+ BufferLockAcquire(buffer, buf_hdr, BUFFER_LOCK_EXCLUSIVE);
else
elog(ERROR, "unrecognized buffer lock mode: %d", mode);
}
@@ -5687,8 +6358,7 @@ ConditionalLockBuffer(Buffer buffer)
buf = GetBufferDescriptor(buffer - 1);
- return LWLockConditionalAcquire(BufferDescriptorGetContentLock(buf),
- LW_EXCLUSIVE);
+ return BufferLockConditional(buffer, buf, BUFFER_LOCK_EXCLUSIVE);
}
/*
@@ -6625,7 +7295,25 @@ ResOwnerReleaseBufferPin(Datum res)
if (BufferIsLocal(buffer))
UnpinLocalBufferNoOwner(buffer);
else
+ {
+ PrivateRefCountEntry *ref;
+
+ ref = GetPrivateRefCountEntry(buffer, false);
+
+ /*
+ * If the buffer was locked at the time of the resowner release,
+ * release the lock now. This should only happen after errors.
+ */
+ if (ref->data.lockmode != BUFFER_LOCK_UNLOCK)
+ {
+ BufferDesc *buf = GetBufferDescriptor(buffer - 1);
+
+ HOLD_INTERRUPTS(); /* match the upcoming RESUME_INTERRUPTS */
+ BufferLockUnlock(buffer, buf);
+ }
+
UnpinBufferNoOwner(GetBufferDescriptor(buffer - 1));
+ }
}
static char *
@@ -6927,16 +7615,12 @@ buffer_stage_common(PgAioHandle *ioh, bool is_write, bool is_temp)
*/
if (is_write && !is_temp)
{
- LWLock *content_lock;
-
- content_lock = BufferDescriptorGetContentLock(buf_hdr);
-
- Assert(LWLockHeldByMe(content_lock));
+ Assert(BufferLockHeldByMe(buf_hdr));
/*
* Lock is now owned by AIO subsystem.
*/
- LWLockDisown(content_lock);
+ BufferLockDisown(buffer, buf_hdr);
}
/*
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 1e5e368a5dc..39ae7cdf856 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -285,6 +285,9 @@ ABI_compatibility:
Section: ClassName - WaitEventBuffer
BUFFER_CLEANUP "Waiting to acquire an exclusive pin on a buffer. Buffer pin waits can be protracted if another process holds an open cursor that last read data from the buffer in question."
+BUFFER_SHARED "Waiting to acquire shared lock on a buffer."
+BUFFER_SHARE_EXCLUSIVE "Waiting to acquire share exclusive lock on a buffer."
+BUFFER_EXCLUSIVE "Waiting to acquire exclusive lock on a buffer."
ABI_compatibility:
--
2.48.1.76.g4e746b1a31.dirty
[text/x-diff] v6-0009-heapam-Move-logic-to-handle-HEAP_MOVED-into-a-hel.patch (11.5K, ../6rgb2nvhyvnszz4ul3wfzlf5rheb2kkwrglthnna7qhe24onwr@vw27225tkyar/10-v6-0009-heapam-Move-logic-to-handle-HEAP_MOVED-into-a-hel.patch)
download | inline diff:
From 5244aaf57a4dba00139058173e1be476ace0ee8e Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 23 Sep 2024 12:23:33 -0400
Subject: [PATCH v6 09/14] heapam: Move logic to handle HEAP_MOVED into a
helper function
Before we dealt with this in 6 near identical and one very similar copy.
The helper function errors out when encountering a
HEAP_MOVED_IN/HEAP_MOVED_OUT tuple with xvac considered current or
in-progress. It'd be preferrable to do that change separately, but otherwise
it'd not be possible to deduplicate the handling in
HeapTupleSatisfiesVacuum().
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
src/backend/access/heap/heapam_visibility.c | 307 ++++----------------
1 file changed, 61 insertions(+), 246 deletions(-)
diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index 05f6946fe60..4fefcbca5f5 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -144,6 +144,55 @@ HeapTupleSetHintBits(HeapTupleHeader tuple, Buffer buffer,
SetHintBits(tuple, buffer, infomask, xid);
}
+/*
+ * If HEAP_MOVED_OFF or HEAP_MOVED_IN are set on the tuple, remove them and
+ * adjust hint bits. See the comment for SetHintBits() for more background.
+ *
+ * This helper returns false if the row ought to be invisible, true otherwise.
+ */
+static inline bool
+HeapTupleCleanMoved(HeapTupleHeader tuple, Buffer buffer)
+{
+ TransactionId xvac;
+
+ /* only used by pre-9.0 binary upgrades */
+ if (likely(!(tuple->t_infomask & (HEAP_MOVED_OFF | HEAP_MOVED_IN))))
+ return true;
+
+ xvac = HeapTupleHeaderGetXvac(tuple);
+
+ if (TransactionIdIsCurrentTransactionId(xvac))
+ elog(ERROR, "encountered tuple with HEAP_MOVED considered current");
+
+ if (TransactionIdIsInProgress(xvac))
+ elog(ERROR, "encountered tuple with HEAP_MOVED considered in-progress");
+
+ if (tuple->t_infomask & HEAP_MOVED_OFF)
+ {
+ if (TransactionIdDidCommit(xvac))
+ {
+ SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
+ InvalidTransactionId);
+ return false;
+ }
+ SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
+ InvalidTransactionId);
+ }
+ else if (tuple->t_infomask & HEAP_MOVED_IN)
+ {
+ if (TransactionIdDidCommit(xvac))
+ SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
+ InvalidTransactionId);
+ else
+ {
+ SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
+ InvalidTransactionId);
+ return false;
+ }
+ }
+
+ return true;
+}
/*
* HeapTupleSatisfiesSelf
@@ -179,45 +228,8 @@ HeapTupleSatisfiesSelf(HeapTuple htup, Snapshot snapshot, Buffer buffer)
if (HeapTupleHeaderXminInvalid(tuple))
return false;
- /* Used by pre-9.0 binary upgrades */
- if (tuple->t_infomask & HEAP_MOVED_OFF)
- {
- TransactionId xvac = HeapTupleHeaderGetXvac(tuple);
-
- if (TransactionIdIsCurrentTransactionId(xvac))
- return false;
- if (!TransactionIdIsInProgress(xvac))
- {
- if (TransactionIdDidCommit(xvac))
- {
- SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
- InvalidTransactionId);
- return false;
- }
- SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
- InvalidTransactionId);
- }
- }
- /* Used by pre-9.0 binary upgrades */
- else if (tuple->t_infomask & HEAP_MOVED_IN)
- {
- TransactionId xvac = HeapTupleHeaderGetXvac(tuple);
-
- if (!TransactionIdIsCurrentTransactionId(xvac))
- {
- if (TransactionIdIsInProgress(xvac))
- return false;
- if (TransactionIdDidCommit(xvac))
- SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
- InvalidTransactionId);
- else
- {
- SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
- InvalidTransactionId);
- return false;
- }
- }
- }
+ if (!HeapTupleCleanMoved(tuple, buffer))
+ return false;
else if (TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetRawXmin(tuple)))
{
if (tuple->t_infomask & HEAP_XMAX_INVALID) /* xid invalid */
@@ -372,45 +384,8 @@ HeapTupleSatisfiesToast(HeapTuple htup, Snapshot snapshot,
if (HeapTupleHeaderXminInvalid(tuple))
return false;
- /* Used by pre-9.0 binary upgrades */
- if (tuple->t_infomask & HEAP_MOVED_OFF)
- {
- TransactionId xvac = HeapTupleHeaderGetXvac(tuple);
-
- if (TransactionIdIsCurrentTransactionId(xvac))
- return false;
- if (!TransactionIdIsInProgress(xvac))
- {
- if (TransactionIdDidCommit(xvac))
- {
- SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
- InvalidTransactionId);
- return false;
- }
- SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
- InvalidTransactionId);
- }
- }
- /* Used by pre-9.0 binary upgrades */
- else if (tuple->t_infomask & HEAP_MOVED_IN)
- {
- TransactionId xvac = HeapTupleHeaderGetXvac(tuple);
-
- if (!TransactionIdIsCurrentTransactionId(xvac))
- {
- if (TransactionIdIsInProgress(xvac))
- return false;
- if (TransactionIdDidCommit(xvac))
- SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
- InvalidTransactionId);
- else
- {
- SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
- InvalidTransactionId);
- return false;
- }
- }
- }
+ if (!HeapTupleCleanMoved(tuple, buffer))
+ return false;
/*
* An invalid Xmin can be left behind by a speculative insertion that
@@ -468,45 +443,8 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
if (HeapTupleHeaderXminInvalid(tuple))
return TM_Invisible;
- /* Used by pre-9.0 binary upgrades */
- if (tuple->t_infomask & HEAP_MOVED_OFF)
- {
- TransactionId xvac = HeapTupleHeaderGetXvac(tuple);
-
- if (TransactionIdIsCurrentTransactionId(xvac))
- return TM_Invisible;
- if (!TransactionIdIsInProgress(xvac))
- {
- if (TransactionIdDidCommit(xvac))
- {
- SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
- InvalidTransactionId);
- return TM_Invisible;
- }
- SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
- InvalidTransactionId);
- }
- }
- /* Used by pre-9.0 binary upgrades */
- else if (tuple->t_infomask & HEAP_MOVED_IN)
- {
- TransactionId xvac = HeapTupleHeaderGetXvac(tuple);
-
- if (!TransactionIdIsCurrentTransactionId(xvac))
- {
- if (TransactionIdIsInProgress(xvac))
- return TM_Invisible;
- if (TransactionIdDidCommit(xvac))
- SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
- InvalidTransactionId);
- else
- {
- SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
- InvalidTransactionId);
- return TM_Invisible;
- }
- }
- }
+ else if (!HeapTupleCleanMoved(tuple, buffer))
+ return false;
else if (TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetRawXmin(tuple)))
{
if (HeapTupleHeaderGetCmin(tuple) >= curcid)
@@ -756,45 +694,8 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
if (HeapTupleHeaderXminInvalid(tuple))
return false;
- /* Used by pre-9.0 binary upgrades */
- if (tuple->t_infomask & HEAP_MOVED_OFF)
- {
- TransactionId xvac = HeapTupleHeaderGetXvac(tuple);
-
- if (TransactionIdIsCurrentTransactionId(xvac))
- return false;
- if (!TransactionIdIsInProgress(xvac))
- {
- if (TransactionIdDidCommit(xvac))
- {
- SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
- InvalidTransactionId);
- return false;
- }
- SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
- InvalidTransactionId);
- }
- }
- /* Used by pre-9.0 binary upgrades */
- else if (tuple->t_infomask & HEAP_MOVED_IN)
- {
- TransactionId xvac = HeapTupleHeaderGetXvac(tuple);
-
- if (!TransactionIdIsCurrentTransactionId(xvac))
- {
- if (TransactionIdIsInProgress(xvac))
- return false;
- if (TransactionIdDidCommit(xvac))
- SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
- InvalidTransactionId);
- else
- {
- SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
- InvalidTransactionId);
- return false;
- }
- }
- }
+ if (!HeapTupleCleanMoved(tuple, buffer))
+ return false;
else if (TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetRawXmin(tuple)))
{
if (tuple->t_infomask & HEAP_XMAX_INVALID) /* xid invalid */
@@ -979,45 +880,8 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
if (HeapTupleHeaderXminInvalid(tuple))
return false;
- /* Used by pre-9.0 binary upgrades */
- if (tuple->t_infomask & HEAP_MOVED_OFF)
- {
- TransactionId xvac = HeapTupleHeaderGetXvac(tuple);
-
- if (TransactionIdIsCurrentTransactionId(xvac))
- return false;
- if (!XidInMVCCSnapshot(xvac, snapshot))
- {
- if (TransactionIdDidCommit(xvac))
- {
- SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
- InvalidTransactionId);
- return false;
- }
- SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
- InvalidTransactionId);
- }
- }
- /* Used by pre-9.0 binary upgrades */
- else if (tuple->t_infomask & HEAP_MOVED_IN)
- {
- TransactionId xvac = HeapTupleHeaderGetXvac(tuple);
-
- if (!TransactionIdIsCurrentTransactionId(xvac))
- {
- if (XidInMVCCSnapshot(xvac, snapshot))
- return false;
- if (TransactionIdDidCommit(xvac))
- SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
- InvalidTransactionId);
- else
- {
- SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
- InvalidTransactionId);
- return false;
- }
- }
- }
+ if (!HeapTupleCleanMoved(tuple, buffer))
+ return false;
else if (TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetRawXmin(tuple)))
{
if (HeapTupleHeaderGetCmin(tuple) >= snapshot->curcid)
@@ -1222,57 +1086,8 @@ HeapTupleSatisfiesVacuumHorizon(HeapTuple htup, Buffer buffer, TransactionId *de
{
if (HeapTupleHeaderXminInvalid(tuple))
return HEAPTUPLE_DEAD;
- /* Used by pre-9.0 binary upgrades */
- else if (tuple->t_infomask & HEAP_MOVED_OFF)
- {
- TransactionId xvac = HeapTupleHeaderGetXvac(tuple);
-
- if (TransactionIdIsCurrentTransactionId(xvac))
- return HEAPTUPLE_DELETE_IN_PROGRESS;
- if (TransactionIdIsInProgress(xvac))
- return HEAPTUPLE_DELETE_IN_PROGRESS;
- if (TransactionIdDidCommit(xvac))
- {
- SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
- InvalidTransactionId);
- return HEAPTUPLE_DEAD;
- }
- SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
- InvalidTransactionId);
- }
- /* Used by pre-9.0 binary upgrades */
- else if (tuple->t_infomask & HEAP_MOVED_IN)
- {
- TransactionId xvac = HeapTupleHeaderGetXvac(tuple);
-
- if (TransactionIdIsCurrentTransactionId(xvac))
- return HEAPTUPLE_INSERT_IN_PROGRESS;
- if (TransactionIdIsInProgress(xvac))
- return HEAPTUPLE_INSERT_IN_PROGRESS;
- if (TransactionIdDidCommit(xvac))
- SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
- InvalidTransactionId);
- else
- {
- SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
- InvalidTransactionId);
- return HEAPTUPLE_DEAD;
- }
- }
- else if (TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetRawXmin(tuple)))
- {
- if (tuple->t_infomask & HEAP_XMAX_INVALID) /* xid invalid */
- return HEAPTUPLE_INSERT_IN_PROGRESS;
- /* only locked? run infomask-only check first, for performance */
- if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask) ||
- HeapTupleHeaderIsOnlyLocked(tuple))
- return HEAPTUPLE_INSERT_IN_PROGRESS;
- /* inserted and then deleted by same xact */
- if (TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetUpdateXid(tuple)))
- return HEAPTUPLE_DELETE_IN_PROGRESS;
- /* deleting subtransaction must have aborted */
- return HEAPTUPLE_INSERT_IN_PROGRESS;
- }
+ else if (!HeapTupleCleanMoved(tuple, buffer))
+ return HEAPTUPLE_DEAD;
else if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmin(tuple)))
{
/*
--
2.48.1.76.g4e746b1a31.dirty
[text/x-diff] v6-0010-heapam-Use-exclusive-lock-on-old-page-in-CLUSTER.patch (2.7K, ../6rgb2nvhyvnszz4ul3wfzlf5rheb2kkwrglthnna7qhe24onwr@vw27225tkyar/11-v6-0010-heapam-Use-exclusive-lock-on-old-page-in-CLUSTER.patch)
download | inline diff:
From 1c1f8f7b7d5a4baffe6cb282cdd4089671361b22 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Sun, 26 Jan 2025 15:18:46 -0500
Subject: [PATCH v6 10/14] heapam: Use exclusive lock on old page in CLUSTER
To be able to guarantee that we can set the hint bit, acquire an exclusive
lock on the old buffer. We need the hint bits to be set as otherwise
reform_and_rewrite_tuple() -> rewrite_heap_tuple() -> heap_freeze_tuple() will
get confused.
It'd be better if we somehow could avoid setting hint bits on the old page. A
commonreason to use VACUUM FULL are very bloated tables - rewriting most of
the old table before during VACUUM FULL doesn't exactly help.
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
src/backend/access/heap/heapam_handler.c | 13 ++++++++++++-
src/backend/access/heap/heapam_visibility.c | 7 +++++++
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index bcbac844bb6..f84254f0737 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -837,7 +837,18 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap,
tuple = ExecFetchSlotHeapTuple(slot, false, NULL);
buf = hslot->buffer;
- LockBuffer(buf, BUFFER_LOCK_SHARE);
+ /*
+ * To be able to guarantee that we can set the hint bit, acquire an
+ * exclusive lock on the old buffer. We need the hint bits to be set
+ * as otherwise reform_and_rewrite_tuple() -> rewrite_heap_tuple() ->
+ * heap_freeze_tuple() will get confused.
+ *
+ * It'd be better if we somehow could avoid setting hint bits on the
+ * old page. One reason to use VACUUM FULL are very bloated tables -
+ * rewriting most of the old table before during VACUUM FULL doesn't
+ * exactly help...
+ */
+ LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
switch (HeapTupleSatisfiesVacuum(tuple, OldestXmin, buf))
{
diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index 4fefcbca5f5..762538a2040 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -141,6 +141,13 @@ void
HeapTupleSetHintBits(HeapTupleHeader tuple, Buffer buffer,
uint16 infomask, TransactionId xid)
{
+ /*
+ * The uses from heapam.c rely on being able to perform the hint bit
+ * updates, which can only be guaranteed if we are holding an exclusive
+ * lock on the buffer - which all callers are doing.
+ */
+ Assert(BufferIsLockedByMeInMode(buffer, BUFFER_LOCK_EXCLUSIVE));
+
SetHintBits(tuple, buffer, infomask, xid);
}
--
2.48.1.76.g4e746b1a31.dirty
[text/x-diff] v6-0011-heapam-Add-batch-mode-mvcc-check-and-use-it-in-pa.patch (7.6K, ../6rgb2nvhyvnszz4ul3wfzlf5rheb2kkwrglthnna7qhe24onwr@vw27225tkyar/12-v6-0011-heapam-Add-batch-mode-mvcc-check-and-use-it-in-pa.patch)
download | inline diff:
From c93a15d76539720a8564de1b0a1100c4734389a7 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Thu, 17 Oct 2024 13:16:36 -0400
Subject: [PATCH v6 11/14] heapam: Add batch mode mvcc check and use it in page
mode
There are two reasons for doing so:
1) It is generally faster to perform checks in a batched fashion and making
sequential scans faster is nice.
2) We would like to stop setting hint bits while pages are being written
out. The necessary locking becomes visible for page mode scans if done for
every tuple. With batching the overhead can be amortized to only happen
once per page.
There are substantial further optimization opportunities along these
lines:
- Right now HeapTupleSatisfiesMVCCBatch() simply uses the single-tuple
HeapTupleSatisfiesMVCC(), relying on the compiler to inline it. We could
instead write an explicitly optimized version that avoids repeated xid
tests.
- Introduce batched version of the serializability test
- Introduce batched version of HeapTupleSatisfiesVacuum
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
src/include/access/heapam.h | 28 +++++++
src/backend/access/heap/heapam.c | 91 ++++++++++++++++-----
src/backend/access/heap/heapam_visibility.c | 47 +++++++++++
src/tools/pgindent/typedefs.list | 1 +
4 files changed, 147 insertions(+), 20 deletions(-)
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 909db73b7bb..13e4a4096c3 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -410,6 +410,34 @@ extern bool HeapTupleHeaderIsOnlyLocked(HeapTupleHeader tuple);
extern bool HeapTupleIsSurelyDead(HeapTuple htup,
GlobalVisState *vistest);
+/*
+ * FIXME: define to be removed
+ *
+ * Without this I see worse performance. But it's a bit ugly, so I thought
+ * it'd be useful to leave a way in for others to experiment with this.
+ */
+#define BATCHMVCC_FEWER_ARGS
+
+#ifdef BATCHMVCC_FEWER_ARGS
+typedef struct BatchMVCCState
+{
+ HeapTupleData tuples[MaxHeapTuplesPerPage];
+ bool visible[MaxHeapTuplesPerPage];
+} BatchMVCCState;
+#endif
+
+extern int HeapTupleSatisfiesMVCCBatch(Snapshot snapshot, Buffer buffer,
+ int ntups,
+#ifdef BATCHMVCC_FEWER_ARGS
+ BatchMVCCState *batchmvcc,
+#else
+ HeapTupleData *tuples,
+ bool *visible,
+#endif
+ OffsetNumber *vistuples_dense);
+
+
+
/*
* To avoid leaking too much knowledge about reorderbuffer implementation
* details this is implemented in reorderbuffer.c not heapam_visibility.c
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 4b0c49f4bb0..ddabd1a3ec3 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -504,42 +504,93 @@ page_collect_tuples(HeapScanDesc scan, Snapshot snapshot,
BlockNumber block, int lines,
bool all_visible, bool check_serializable)
{
+ Oid relid = RelationGetRelid(scan->rs_base.rs_rd);
+#ifdef BATCHMVCC_FEWER_ARGS
+ BatchMVCCState batchmvcc;
+ HeapTupleData *tuples = batchmvcc.tuples;
+ bool *visible = batchmvcc.visible;
+#else
+ HeapTupleData tuples[MaxHeapTuplesPerPage];
+ bool visible[MaxHeapTuplesPerPage];
+#endif
int ntup = 0;
- OffsetNumber lineoff;
+ int nvis = 0;
- for (lineoff = FirstOffsetNumber; lineoff <= lines; lineoff++)
+ /* page at a time should have been disabled otherwise */
+ Assert(IsMVCCSnapshot(snapshot));
+
+ /* first find all tuples on the page */
+ for (OffsetNumber lineoff = FirstOffsetNumber; lineoff <= lines; lineoff++)
{
ItemId lpp = PageGetItemId(page, lineoff);
- HeapTupleData loctup;
- bool valid;
+ HeapTuple tup;
- if (!ItemIdIsNormal(lpp))
+ if (unlikely(!ItemIdIsNormal(lpp)))
continue;
- loctup.t_data = (HeapTupleHeader) PageGetItem(page, lpp);
- loctup.t_len = ItemIdGetLength(lpp);
- loctup.t_tableOid = RelationGetRelid(scan->rs_base.rs_rd);
- ItemPointerSet(&(loctup.t_self), block, lineoff);
+ /*
+ * If the page is not all-visible or we need to check serializability,
+ * maintain enough state to be able to refind the tuple efficiently,
+ * without again needing to extract it from the page.
+ */
+ if (!all_visible || check_serializable)
+ {
+ tup = &tuples[ntup];
+ tup->t_data = (HeapTupleHeader) PageGetItem(page, lpp);
+ tup->t_len = ItemIdGetLength(lpp);
+ tup->t_tableOid = relid;
+ ItemPointerSet(&(tup->t_self), block, lineoff);
+ }
+
+ /*
+ * If the page is all visible, these fields won'otherwise wont be
+ * populated in loop below.
+ */
if (all_visible)
- valid = true;
- else
- valid = HeapTupleSatisfiesVisibility(&loctup, snapshot, buffer);
-
- if (check_serializable)
- HeapCheckForSerializableConflictOut(valid, scan->rs_base.rs_rd,
- &loctup, buffer, snapshot);
-
- if (valid)
{
+ if (check_serializable)
+ {
+ visible[ntup] = true;
+ }
scan->rs_vistuples[ntup] = lineoff;
- ntup++;
}
+
+ ntup++;
}
Assert(ntup <= MaxHeapTuplesPerPage);
- return ntup;
+ /* unless the page is all visible, test visibility for all tuples one go */
+ if (all_visible)
+ nvis = ntup;
+ else
+ nvis = HeapTupleSatisfiesMVCCBatch(snapshot, buffer,
+ ntup,
+#ifdef BATCHMVCC_FEWER_ARGS
+ &batchmvcc,
+#else
+ tuples, visible,
+#endif
+ scan->rs_vistuples
+ );
+
+ /*
+ * So far we don't have batch API for testing serializabilty, so do so
+ * one-by-one.
+ */
+ if (check_serializable)
+ {
+ for (int i = 0; i < ntup; i++)
+ {
+ HeapCheckForSerializableConflictOut(visible[i],
+ scan->rs_base.rs_rd,
+ &tuples[i],
+ buffer, snapshot);
+ }
+ }
+
+ return nvis;
}
/*
diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index 762538a2040..5645cfd8a49 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -1584,6 +1584,53 @@ HeapTupleSatisfiesHistoricMVCC(HeapTuple htup, Snapshot snapshot,
return true;
}
+/*
+ * Perform HeaptupleSatisfiesMVCC() on each passed in tuple. This is more
+ * efficient than doing HeapTupleSatisfiesMVCC() one-by-one.
+ *
+ * To be checked tuples are passed via BatchMVCCState->tuples. Each tuple's
+ * visibility is set in batchmvcc->visible[]. In addition, ->vistuples_dense
+ * is set to contain the offsets of visible tuples.
+ *
+ * Returns the number of visible tuples.
+ */
+int
+HeapTupleSatisfiesMVCCBatch(Snapshot snapshot, Buffer buffer,
+ int ntups,
+#ifdef BATCHMVCC_FEWER_ARGS
+ BatchMVCCState *batchmvcc,
+#else
+ HeapTupleData *tuples,
+ bool *visible,
+#endif
+ OffsetNumber *vistuples_dense)
+{
+ int nvis = 0;
+#ifdef BATCHMVCC_FEWER_ARGS
+ HeapTupleData *tuples = batchmvcc->tuples;
+ bool *visible = batchmvcc->visible;
+#endif
+
+ Assert(IsMVCCSnapshot(snapshot));
+
+ for (int i = 0; i < ntups; i++)
+ {
+ bool valid;
+ HeapTuple tup = &tuples[i];
+
+ valid = HeapTupleSatisfiesMVCC(tup, snapshot, buffer);
+ visible[i] = valid;
+
+ if (likely(valid))
+ {
+ vistuples_dense[nvis] = tup->t_self.ip_posid;
+ nvis++;
+ }
+ }
+
+ return nvis;
+}
+
/*
* HeapTupleSatisfiesVisibility
* True iff heap tuple satisfies a time qual.
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 9a89e68c59c..9d14239b4c4 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -249,6 +249,7 @@ Barrier
BaseBackupCmd
BaseBackupTargetHandle
BaseBackupTargetType
+BatchMVCCState
BeginDirectModify_function
BeginForeignInsert_function
BeginForeignModify_function
--
2.48.1.76.g4e746b1a31.dirty
[text/x-diff] v6-0012-Require-share-exclusive-lock-to-set-hint-bits.patch (32.7K, ../6rgb2nvhyvnszz4ul3wfzlf5rheb2kkwrglthnna7qhe24onwr@vw27225tkyar/13-v6-0012-Require-share-exclusive-lock-to-set-hint-bits.patch)
download | inline diff:
From 6e61b1b2d2202c23674f27ba80cd50ff596fb95b Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 18 Nov 2025 09:22:28 -0500
Subject: [PATCH v6 12/14] Require share-exclusive lock to set hint bits
At the moment hint bits can be set with just a share lock on a page (and in
one place even without any lock). Because of this we need to copy pages while
writing them out, as otherwise the checksum could be corrupted.
The need to copy the page is problematic to implement AIO writes:
1) Instead of just needing a single buffer for a copied page we need one for
each page that's potentially undergoing IO
2) To be able to use the "worker" AIO implementation the copied page needs to
reside in shared memory.
It also causes problems for using unbuffered/direct-IO, independent of AIO:
Some filesystems, raid implementations, ... do not tolerate the data being
written out to change during the write. E.g. they may compute internal
checksums that can be invalidated by concurrent modifications, leading e.g. to
filesystem errors (as the case with btrfs).
It also just is plain odd to allow modifications of buffers that are just
share locked.
To address these issue, this commit changes the rules so that modifications to
pages are not allowed anymore while holding a share lock. Instead the new
share-exclusive lock (introduced in FIXME XXXX TODO) allows at most one
backend to modify a buffer while other backends have the same page share
locked. An existing share-lock can be upgraded to a share-exclusive lock, if
there are no conflicting locks. For that
BufferBeginSetHintBits()/BufferBeginSetHintBits() and BufferSetHintBits16()
have been introduced.
The biggest change to adapt to this is in heapam. To avoid performance
regressions for sequential scans that need to set a lot of hint bits, we need
to amortize the cost of BufferBeginSetHintBits() for cases where hint bits are
set at a high frequency, HeapTupleSatisfiesMVCCBatch() uses the new
SetHintBitsExt() which defers BufferFinishSetHintBits() until all hint bits on
a page have been set. Conversely, to avoid regressions in cases where we
can't set hint bits in bulk (because we're looking only at individual tuples),
use BufferSetHintBits16() when setting hint bits without batching.
Several other places also need to be adapted, but those changes are
comparatively simpler.
After this we do not need to copy buffers to write them out anymore. That
change is done separately however.
TODO:
- Address FIXMEs
- reflow parts of storage/buffer/README that I didn't reindent to make the
diff more readable
Discussion: https://postgr.es/m/fvfmkr5kk4nyex56ejgxj3uzi63isfxovp2biecb4bspbjrze7@az2pljabhnff
Discussion: https://postgr.es/m/stj36ea6yyhoxtqkhpieia2z4krnam7qyetc57rfezgk4zgapf%40gcnactj4z56m
---
src/include/storage/bufmgr.h | 4 +
src/backend/access/gist/gistget.c | 19 +-
src/backend/access/hash/hashutil.c | 10 +-
src/backend/access/heap/heapam_visibility.c | 124 ++++++++--
src/backend/access/nbtree/nbtinsert.c | 28 ++-
src/backend/access/nbtree/nbtutils.c | 16 +-
src/backend/storage/buffer/README | 32 ++-
src/backend/storage/buffer/bufmgr.c | 241 +++++++++++++++-----
src/backend/storage/freespace/freespace.c | 20 +-
src/backend/storage/freespace/fsmpage.c | 11 +-
src/tools/pgindent/typedefs.list | 1 +
11 files changed, 392 insertions(+), 114 deletions(-)
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index 8e442492d4d..afa16afffc9 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -302,6 +302,10 @@ extern void BufferGetTag(Buffer buffer, RelFileLocator *rlocator,
extern void MarkBufferDirtyHint(Buffer buffer, bool buffer_std);
+extern bool BufferSetHintBits16(uint16 *ptr, uint16 val, Buffer buffer);
+extern bool BufferBeginSetHintBits(Buffer buffer);
+extern void BufferFinishSetHintBits(Buffer buffer, bool mark_dirty, bool buffer_std);
+
extern void UnlockBuffers(void);
extern void UnlockBuffer(Buffer buffer);
extern void LockBufferInternal(Buffer buffer, BufferLockMode mode);
diff --git a/src/backend/access/gist/gistget.c b/src/backend/access/gist/gistget.c
index 9ba45acfff3..956ece6bed5 100644
--- a/src/backend/access/gist/gistget.c
+++ b/src/backend/access/gist/gistget.c
@@ -63,11 +63,7 @@ gistkillitems(IndexScanDesc scan)
* safe.
*/
if (BufferGetLSNAtomic(buffer) != so->curPageLSN)
- {
- UnlockReleaseBuffer(buffer);
- so->numKilled = 0; /* reset counter */
- return;
- }
+ goto unlock;
Assert(GistPageIsLeaf(page));
@@ -77,6 +73,16 @@ gistkillitems(IndexScanDesc scan)
*/
for (i = 0; i < so->numKilled; i++)
{
+ if (!killedsomething)
+ {
+ /*
+ * Use hint bit infrastructure to be allowed to modify the page
+ * without holding an exclusive lock.
+ */
+ if (!BufferBeginSetHintBits(buffer))
+ goto unlock;
+ }
+
offnum = so->killedItems[i];
iid = PageGetItemId(page, offnum);
ItemIdMarkDead(iid);
@@ -86,9 +92,10 @@ gistkillitems(IndexScanDesc scan)
if (killedsomething)
{
GistMarkPageHasGarbage(page);
- MarkBufferDirtyHint(buffer, true);
+ BufferFinishSetHintBits(buffer, true, true);
}
+unlock:
UnlockReleaseBuffer(buffer);
/*
diff --git a/src/backend/access/hash/hashutil.c b/src/backend/access/hash/hashutil.c
index f41233fcd07..d1d603770b2 100644
--- a/src/backend/access/hash/hashutil.c
+++ b/src/backend/access/hash/hashutil.c
@@ -593,6 +593,13 @@ _hash_kill_items(IndexScanDesc scan)
if (ItemPointerEquals(&ituple->t_tid, &currItem->heapTid))
{
+ /*
+ * Use hint bit infrastructure to be allowed to modify the
+ * page without holding an exclusive lock.
+ */
+ if (!BufferBeginSetHintBits(so->currPos.buf))
+ goto unlock_page;
+
/* found the item */
ItemIdMarkDead(iid);
killedsomething = true;
@@ -610,9 +617,10 @@ _hash_kill_items(IndexScanDesc scan)
if (killedsomething)
{
opaque->hasho_flag |= LH_PAGE_HAS_DEAD_TUPLES;
- MarkBufferDirtyHint(buf, true);
+ BufferFinishSetHintBits(so->currPos.buf, true, true);
}
+unlock_page:
if (so->hashso_bucket_buf == so->currPos.buf ||
havePin)
LockBuffer(so->currPos.buf, BUFFER_LOCK_UNLOCK);
diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index 5645cfd8a49..630ba7df167 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -80,10 +80,38 @@
/*
- * SetHintBits()
+ * To be allowed to set hint bits, SetHintBits() needs to call
+ * BufferBeginSetHintBits(). However, that's not free, and some callsites call
+ * SetHintBits() on many tuples in a row. For those it makes sense to amortize
+ * the cost of BufferBeginSetHintBits(). Additionally it's desirable to defer
+ * the cost of BufferBeginSetHintBits() until a hint bit needs to actually be
+ * set. This enum serves as the necessary state space passed to
+ * SetHintbitsExt().
+ */
+typedef enum SetHintBitsState
+{
+ /* not yet checked if hint bits may be set */
+ SHB_INITIAL,
+ /* failed to get permission to set hint bits, don't check again */
+ SHB_DISABLED,
+ /* allowed to set hint bits */
+ SHB_ENABLED,
+} SetHintBitsState;
+
+/*
+ * SetHintBitsExt()
*
* Set commit/abort hint bits on a tuple, if appropriate at this time.
*
+ * To be allowed to set a hint bit on a tuple, the page must not be undergoing
+ * IO at this time (otherwise we e.g. could corrupt PG's page checksum or even
+ * the filesystem's, as is known to happen with btrfs).
+ *
+ * The right to set a hint bit can be acquired on a page level with
+ * BufferBeginSetHintBits(). Only a single backend gets the right to set hint
+ * bits at a time. Alternatively, if called with a NULL SetHintBitsState*,
+ * hint bits are set with BufferSetHintBits16().
+ *
* It is only safe to set a transaction-committed hint bit if we know the
* transaction's commit record is guaranteed to be flushed to disk before the
* buffer, or if the table is temporary or unlogged and will be obliterated by
@@ -111,24 +139,68 @@
* InvalidTransactionId if no check is needed.
*/
static inline void
-SetHintBits(HeapTupleHeader tuple, Buffer buffer,
- uint16 infomask, TransactionId xid)
+SetHintBitsExt(HeapTupleHeader tuple, Buffer buffer,
+ uint16 infomask, TransactionId xid, SetHintBitsState *state)
{
if (TransactionIdIsValid(xid))
{
- /* NB: xid must be known committed here! */
- XLogRecPtr commitLSN = TransactionIdGetCommitLSN(xid);
+ if (BufferIsPermanent(buffer))
+ {
+ /* NB: xid must be known committed here! */
+ XLogRecPtr commitLSN = TransactionIdGetCommitLSN(xid);
+
+ if (XLogNeedsFlush(commitLSN) &&
+ BufferGetLSNAtomic(buffer) < commitLSN)
+ {
+ /* not flushed and no LSN interlock, so don't set hint */
+ return;
+ }
+ }
+ }
+
+ /*
+ * If we're not operating in batch mode, use BufferSetHintBits16 to mark
+ * the page dirty, that's cheaper than
+ * BufferBeginSetHintBits()/BufferFinishSetHintBits(). That's important
+ * for cases where we set a lot of hint bits on a page individually.
+ */
+ if (!state)
+ {
+ BufferSetHintBits16(&tuple->t_infomask, tuple->t_infomask | infomask, buffer);
+ return;
+ }
+
+ /*
+ * In batched mode and we previously did not get permission to set hint
+ * bits. Don't try again, in all likelihood IO is still going on.
+ */
+ if (*state == SHB_DISABLED)
+ return;
- if (BufferIsPermanent(buffer) && XLogNeedsFlush(commitLSN) &&
- BufferGetLSNAtomic(buffer) < commitLSN)
+ if (*state == SHB_INITIAL)
+ {
+ if (!BufferBeginSetHintBits(buffer))
{
- /* not flushed and no LSN interlock, so don't set hint */
+ *state = SHB_DISABLED;
return;
}
+
+ if (state)
+ *state = SHB_ENABLED;
+
}
-
tuple->t_infomask |= infomask;
- MarkBufferDirtyHint(buffer, true);
+}
+
+/*
+ * Simple wrapper around SetHintBitExt(), use when operating on a single
+ * tuple.
+ */
+static inline void
+SetHintBits(HeapTupleHeader tuple, Buffer buffer,
+ uint16 infomask, TransactionId xid)
+{
+ SetHintBitsExt(tuple, buffer, infomask, xid, NULL);
}
/*
@@ -864,9 +936,9 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
* inserting/deleting transaction was still running --- which was more cycles
* and more contention on ProcArrayLock.
*/
-static bool
+static inline bool
HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
- Buffer buffer)
+ Buffer buffer, SetHintBitsState *state)
{
HeapTupleHeader tuple = htup->t_data;
@@ -921,8 +993,8 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
if (!TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetRawXmax(tuple)))
{
/* deleting subtransaction must have aborted */
- SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
- InvalidTransactionId);
+ SetHintBitsExt(tuple, buffer, HEAP_XMAX_INVALID,
+ InvalidTransactionId, state);
return true;
}
@@ -934,13 +1006,13 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
else if (XidInMVCCSnapshot(HeapTupleHeaderGetRawXmin(tuple), snapshot))
return false;
else if (TransactionIdDidCommit(HeapTupleHeaderGetRawXmin(tuple)))
- SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
- HeapTupleHeaderGetRawXmin(tuple));
+ SetHintBitsExt(tuple, buffer, HEAP_XMIN_COMMITTED,
+ HeapTupleHeaderGetRawXmin(tuple), state);
else
{
/* it must have aborted or crashed */
- SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
- InvalidTransactionId);
+ SetHintBitsExt(tuple, buffer, HEAP_XMIN_INVALID,
+ InvalidTransactionId, state);
return false;
}
}
@@ -1003,14 +1075,14 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
if (!TransactionIdDidCommit(HeapTupleHeaderGetRawXmax(tuple)))
{
/* it must have aborted or crashed */
- SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
- InvalidTransactionId);
+ SetHintBitsExt(tuple, buffer, HEAP_XMAX_INVALID,
+ InvalidTransactionId, state);
return true;
}
/* xmax transaction committed */
- SetHintBits(tuple, buffer, HEAP_XMAX_COMMITTED,
- HeapTupleHeaderGetRawXmax(tuple));
+ SetHintBitsExt(tuple, buffer, HEAP_XMAX_COMMITTED,
+ HeapTupleHeaderGetRawXmax(tuple), state);
}
else
{
@@ -1606,6 +1678,7 @@ HeapTupleSatisfiesMVCCBatch(Snapshot snapshot, Buffer buffer,
OffsetNumber *vistuples_dense)
{
int nvis = 0;
+ SetHintBitsState state = SHB_INITIAL;
#ifdef BATCHMVCC_FEWER_ARGS
HeapTupleData *tuples = batchmvcc->tuples;
bool *visible = batchmvcc->visible;
@@ -1618,7 +1691,7 @@ HeapTupleSatisfiesMVCCBatch(Snapshot snapshot, Buffer buffer,
bool valid;
HeapTuple tup = &tuples[i];
- valid = HeapTupleSatisfiesMVCC(tup, snapshot, buffer);
+ valid = HeapTupleSatisfiesMVCC(tup, snapshot, buffer, &state);
visible[i] = valid;
if (likely(valid))
@@ -1628,6 +1701,9 @@ HeapTupleSatisfiesMVCCBatch(Snapshot snapshot, Buffer buffer,
}
}
+ if (state == SHB_ENABLED)
+ BufferFinishSetHintBits(buffer, true, true);
+
return nvis;
}
@@ -1647,7 +1723,7 @@ HeapTupleSatisfiesVisibility(HeapTuple htup, Snapshot snapshot, Buffer buffer)
switch (snapshot->snapshot_type)
{
case SNAPSHOT_MVCC:
- return HeapTupleSatisfiesMVCC(htup, snapshot, buffer);
+ return HeapTupleSatisfiesMVCC(htup, snapshot, buffer, NULL);
case SNAPSHOT_SELF:
return HeapTupleSatisfiesSelf(htup, snapshot, buffer);
case SNAPSHOT_ANY:
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
index 7c113c007e5..545e1d7d9e0 100644
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -680,20 +680,28 @@ _bt_check_unique(Relation rel, BTInsertState insertstate, Relation heapRel,
{
/*
* The conflicting tuple (or all HOT chains pointed to by
- * all posting list TIDs) is dead to everyone, so mark the
- * index entry killed.
+ * all posting list TIDs) is dead to everyone, so try to
+ * mark the index entry killed. It's ok if we're not
+ * allowed to, this isn't required for correctness.
*/
- ItemIdMarkDead(curitemid);
- opaque->btpo_flags |= BTP_HAS_GARBAGE;
+ Buffer buf;
- /*
- * Mark buffer with a dirty hint, since state is not
- * crucial. Be sure to mark the proper buffer dirty.
- */
+ /* Be sure to operate on the proper buffer */
if (nbuf != InvalidBuffer)
- MarkBufferDirtyHint(nbuf, true);
+ buf = nbuf;
else
- MarkBufferDirtyHint(insertstate->buf, true);
+ buf = insertstate->buf;
+
+ /*
+ * Can't use BufferSetHintBits16() here as we update two
+ * different locations.
+ */
+ if (BufferBeginSetHintBits(buf))
+ {
+ ItemIdMarkDead(curitemid);
+ opaque->btpo_flags |= BTP_HAS_GARBAGE;
+ BufferFinishSetHintBits(buf, true, true);
+ }
}
/*
diff --git a/src/backend/access/nbtree/nbtutils.c b/src/backend/access/nbtree/nbtutils.c
index ab0f98b0287..34e548b9930 100644
--- a/src/backend/access/nbtree/nbtutils.c
+++ b/src/backend/access/nbtree/nbtutils.c
@@ -3542,10 +3542,19 @@ _bt_killitems(IndexScanDesc scan)
* it's possible that multiple processes attempt to do this
* simultaneously, leading to multiple full-page images being sent
* to WAL (if wal_log_hints or data checksums are enabled), which
- * is undesirable.
+ * is undesirable. We need to use the hint bit infrastructure to
+ * update the page while just holding a share lock.
*/
if (killtuple && !ItemIdIsDead(iid))
{
+ /*
+ * If we're not able to set hint bits, there's no point
+ * continuing.
+ */
+ if (!killedsomething &&
+ !BufferBeginSetHintBits(buf))
+ goto unlock_page;
+
/* found the item/all posting list items */
ItemIdMarkDead(iid);
killedsomething = true;
@@ -3556,8 +3565,6 @@ _bt_killitems(IndexScanDesc scan)
}
/*
- * Since this can be redone later if needed, mark as dirty hint.
- *
* Whenever we mark anything LP_DEAD, we also set the page's
* BTP_HAS_GARBAGE flag, which is likewise just a hint. (Note that we
* only rely on the page-level flag in !heapkeyspace indexes.)
@@ -3565,9 +3572,10 @@ _bt_killitems(IndexScanDesc scan)
if (killedsomething)
{
opaque->btpo_flags |= BTP_HAS_GARBAGE;
- MarkBufferDirtyHint(buf, true);
+ BufferFinishSetHintBits(buf, true, true);
}
+unlock_page:
if (!so->dropPin)
_bt_unlockbuf(rel, buf);
else
diff --git a/src/backend/storage/buffer/README b/src/backend/storage/buffer/README
index 119f31b5d65..9a4dc101c26 100644
--- a/src/backend/storage/buffer/README
+++ b/src/backend/storage/buffer/README
@@ -25,14 +25,20 @@ that might need to do such a wait is instead handled by waiting to obtain
the relation-level lock, which is why you'd better hold one first.) Pins
may not be held across transaction boundaries, however.
-Buffer content locks: there are two kinds of buffer lock, shared and exclusive,
-which act just as you'd expect: multiple backends can hold shared locks on
-the same buffer, but an exclusive lock prevents anyone else from holding
-either shared or exclusive lock. (These can alternatively be called READ
-and WRITE locks.) These locks are intended to be short-term: they should not
-be held for long. Buffer locks are acquired and released by LockBuffer().
-It will *not* work for a single backend to try to acquire multiple locks on
-the same buffer. One must pin a buffer before trying to lock it.
+Buffer content locks: there three kinds of buffer lock, shared,
+share-exclusive and exclusive:
+a) multiple backends can hold shared locks on the same buffer
+ (alternatively called a READ lock)
+b) one backend can hold an share-exclusive lock on a buffer while multiple
+ backends can hold a share lock
+c) an exclusive lock prevents anyone else from holding either shared or
+ exclusive lock.
+ (alternatively called a WRITE lock)
+
+These locks are intended to be short-term: they should not be held for long.
+Buffer locks are acquired and released by LockBuffer(). It will *not* work
+for a single backend to try to acquire multiple locks on the same buffer. One
+must pin a buffer before trying to lock it.
Buffer access rules:
@@ -55,8 +61,14 @@ one must hold a pin and an exclusive content lock on the containing buffer.
This ensures that no one else might see a partially-updated state of the
tuple while they are doing visibility checks.
-4. It is considered OK to update tuple commit status bits (ie, OR the
-values HEAP_XMIN_COMMITTED, HEAP_XMIN_INVALID, HEAP_XMAX_COMMITTED, or
+4. Non-critical information on a page ("hint bits") may be modified while
+holding only a share-exclusive lock and pin on the page. To do so in cases
+where only a share lock is already held, use BufferBeginSetHintBits() &
+BufferFinishSetHintBits() (if multiple hint bits are to be set) or
+BufferSetHintBits16() (if a single hit bit is set).
+
+E.g. for heapam, a share-exclusive lock allows to update tuple commit status
+bits (ie, OR the values HEAP_XMIN_COMMITTED, HEAP_XMIN_INVALID, HEAP_XMAX_COMMITTED, or
HEAP_XMAX_INVALID into t_infomask) while holding only a shared lock and
pin on a buffer. This is OK because another backend looking at the tuple
at about the same time would OR the same bits into the field, so there
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index da83b775d0b..9ed7a368d74 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -2422,9 +2422,8 @@ again:
/*
* If the buffer was dirty, try to write it out. There is a race
* condition here, in that someone might dirty it after we released the
- * buffer header lock above, or even while we are writing it out (since
- * our share-lock won't prevent hint-bit updates). We will recheck the
- * dirty bit after re-locking the buffer header.
+ * buffer header lock above. We will recheck the dirty bit after
+ * re-locking the buffer header.
*/
if (buf_state & BM_DIRTY)
{
@@ -2432,12 +2431,12 @@ again:
Assert(buf_state & BM_VALID);
/*
- * We need a share-lock on the buffer contents to write it out (else
+ * We need a share-exclusive lock on the buffer contents to write it out (else
* we might write invalid data, eg because someone else is compacting
* the page contents while we write). We must use a conditional lock
* acquisition here to avoid deadlock. Even though the buffer was not
* pinned (and therefore surely not locked) when StrategyGetBuffer
- * returned it, someone else could have pinned and exclusive-locked it
+ * returned it, someone else could have pinned and (share-)exclusive-locked it
* by the time we get here. If we try to get the lock unconditionally,
* we'd block waiting for them; if they later block waiting for us,
* deadlock ensues. (This has been observed to happen when two
@@ -2445,7 +2444,7 @@ again:
* one just happens to be trying to split the page the first one got
* from StrategyGetBuffer.)
*/
- if (!BufferLockConditional(buf, buf_hdr, BUFFER_LOCK_SHARE))
+ if (!BufferLockConditional(buf, buf_hdr, BUFFER_LOCK_SHARE_EXCLUSIVE))
{
/*
* Someone else has locked the buffer, so give it up and loop back
@@ -4014,8 +4013,8 @@ SyncOneBuffer(int buf_id, bool skip_recently_used, WritebackContext *wb_context)
}
/*
- * Pin it, share-lock it, write it. (FlushBuffer will do nothing if the
- * buffer is clean by the time we've locked it.)
+ * Pin it, share-exclusive-lock it, write it. (FlushBuffer will do
+ * nothing if the buffer is clean by the time we've locked it.)
*/
PinBuffer_Locked(bufHdr);
@@ -4329,11 +4328,8 @@ BufferGetTag(Buffer buffer, RelFileLocator *rlocator, ForkNumber *forknum,
* However, we will need to force the changes to disk via fsync before
* we can checkpoint WAL.
*
- * The caller must hold a pin on the buffer and have share-locked the
- * buffer contents. (Note: a share-lock does not prevent updates of
- * hint bits in the buffer, so the page could change while the write
- * is in progress, but we assume that that will not invalidate the data
- * written.)
+ * The caller must hold a pin on the buffer and have
+ * (share-)exclusively-locked the buffer contents.
*
* If the caller has an smgr reference for the buffer's relation, pass it
* as the second parameter. If not, pass NULL.
@@ -4349,6 +4345,9 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object,
char *bufToWrite;
uint64 buf_state;
+ Assert(BufferLockHeldByMeInMode(buf, BUFFER_LOCK_EXCLUSIVE) ||
+ BufferLockHeldByMeInMode(buf, BUFFER_LOCK_SHARE_EXCLUSIVE));
+
/*
* Try to start an I/O operation. If StartBufferIO returns false, then
* someone else flushed the buffer before we could, so we need not do
@@ -4481,7 +4480,7 @@ FlushUnlockedBuffer(BufferDesc *buf, SMgrRelation reln,
{
Buffer buffer = BufferDescriptorGetBuffer(buf);
- BufferLockAcquire(buffer, buf, BUFFER_LOCK_SHARE);
+ BufferLockAcquire(buffer, buf, BUFFER_LOCK_SHARE_EXCLUSIVE);
FlushBuffer(buf, reln, IOOBJECT_RELATION, IOCONTEXT_NORMAL);
BufferLockUnlock(buffer, buf);
}
@@ -5400,8 +5399,8 @@ FlushDatabaseBuffers(Oid dbid)
}
/*
- * Flush a previously, shared or exclusively, locked and pinned buffer to the
- * OS.
+ * Flush a previously, share-exclusively or exclusively, locked and pinned
+ * buffer to the OS.
*/
void
FlushOneBuffer(Buffer buffer)
@@ -5474,39 +5473,23 @@ IncrBufferRefCount(Buffer buffer)
}
/*
- * MarkBufferDirtyHint
+ * Shared-buffer only helper for MarkBufferDirtyHint() and
+ * BufferSetHintBits16().
*
- * Mark a buffer dirty for non-critical changes.
- *
- * This is essentially the same as MarkBufferDirty, except:
- *
- * 1. The caller does not write WAL; so if checksums are enabled, we may need
- * to write an XLOG_FPI_FOR_HINT WAL record to protect against torn pages.
- * 2. The caller might have only share-lock instead of exclusive-lock on the
- * buffer's content lock.
- * 3. This function does not guarantee that the buffer is always marked dirty
- * (due to a race condition), so it cannot be used for important changes.
+ * This is separated out because it turns out that the repeated checks for
+ * local buffers, repeated GetBufferDescriptor() and repeated reading of the
+ * buffer's state sufficiently hurts the performance of BufferSetHintBits16().
*/
-void
-MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
+static inline void
+MarkSharedBufferDirtyHint(Buffer buffer, BufferDesc *bufHdr, uint64 lockstate, bool buffer_std)
{
- BufferDesc *bufHdr;
Page page = BufferGetPage(buffer);
- if (!BufferIsValid(buffer))
- elog(ERROR, "bad buffer ID: %d", buffer);
-
- if (BufferIsLocal(buffer))
- {
- MarkLocalBufferDirty(buffer);
- return;
- }
-
- bufHdr = GetBufferDescriptor(buffer - 1);
-
Assert(GetPrivateRefCount(buffer) > 0);
- /* here, either share or exclusive lock is OK */
- Assert(BufferIsLockedByMe(buffer));
+
+ /* here, either share-exclusive or exclusive lock is OK */
+ Assert(BufferLockHeldByMeInMode(bufHdr, BUFFER_LOCK_EXCLUSIVE) ||
+ BufferLockHeldByMeInMode(bufHdr, BUFFER_LOCK_SHARE_EXCLUSIVE));
/*
* This routine might get called many times on the same page, if we are
@@ -5519,8 +5502,8 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
* is only intended to be used in cases where failing to write out the
* data would be harmless anyway, it doesn't really matter.
*/
- if ((pg_atomic_read_u64(&bufHdr->state) & (BM_DIRTY | BM_JUST_DIRTIED)) !=
- (BM_DIRTY | BM_JUST_DIRTIED))
+ if (unlikely((lockstate & (BM_DIRTY | BM_JUST_DIRTIED)) !=
+ (BM_DIRTY | BM_JUST_DIRTIED)))
{
XLogRecPtr lsn = InvalidXLogRecPtr;
bool dirtied = false;
@@ -5589,13 +5572,13 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
dirtied = true; /* Means "will be dirtied by this action" */
/*
- * Set the page LSN if we wrote a backup block. We aren't supposed
- * to set this when only holding a share lock but as long as we
- * serialise it somehow we're OK. We choose to set LSN while
- * holding the buffer header lock, which causes any reader of an
- * LSN who holds only a share lock to also obtain a buffer header
- * lock before using PageGetLSN(), which is enforced in
- * BufferGetLSNAtomic().
+ * Set the page LSN if we wrote a backup block. To allow backends
+ * that only hold a share lock on the buffer to read the LSN in a
+ * tear-free manner, we set the page LSN while holding the buffer
+ * header lock. This allows any reader of an LSN who holds only a
+ * share lock to also obtain a buffer header lock before using
+ * PageGetLSN() to read the LSN in a tear free way. This is done
+ * in BufferGetLSNAtomic().
*
* If checksums are enabled, you might think we should reset the
* checksum here. That will happen when the page is written
@@ -5621,6 +5604,40 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
}
}
+/*
+ * MarkBufferDirtyHint
+ *
+ * Mark a buffer dirty for non-critical changes.
+ *
+ * This is essentially the same as MarkBufferDirty, except:
+ *
+ * 1. The caller does not write WAL; so if checksums are enabled, we may need
+ * to write an XLOG_FPI_FOR_HINT WAL record to protect against torn pages.
+ * 2. The caller might have only share-exclusive-lock instead of
+ * exclusive-lock on the buffer's content lock.
+ * 3. This function does not guarantee that the buffer is always marked dirty
+ * (due to a race condition), so it cannot be used for important changes.
+ */
+inline void
+MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
+{
+ BufferDesc *bufHdr;
+
+ bufHdr = GetBufferDescriptor(buffer - 1);
+
+ if (!BufferIsValid(buffer))
+ elog(ERROR, "bad buffer ID: %d", buffer);
+
+ if (BufferIsLocal(buffer))
+ {
+ MarkLocalBufferDirty(buffer);
+ return;
+ }
+
+ MarkSharedBufferDirtyHint(buffer, bufHdr, pg_atomic_read_u64(&bufHdr->state),
+ buffer_std);
+}
+
/*
* Release buffer content locks for shared buffers.
*
@@ -6673,6 +6690,126 @@ IsBufferCleanupOK(Buffer buffer)
return false;
}
+static inline bool
+SharedBufferBeginSetHintBits(Buffer buffer, BufferDesc *buf_hdr, uint64 *lockstate)
+{
+ uint64 old_state;
+ PrivateRefCountEntry *ref;
+ BufferLockMode mode;
+
+ ref = GetPrivateRefCountEntry(buffer, true);
+
+ if (ref == NULL)
+ elog(ERROR, "lock is not held");
+
+ mode = ref->data.lockmode;
+ if (mode == BUFFER_LOCK_UNLOCK)
+ elog(ERROR, "buffer is not locked");
+
+ /*
+ * Already am holding the required lock level.
+ */
+ if (mode == BUFFER_LOCK_EXCLUSIVE || mode == BUFFER_LOCK_SHARE_EXCLUSIVE)
+ {
+ *lockstate = pg_atomic_read_u64(&buf_hdr->state);
+ return true;
+ }
+
+ /*
+ * Only holding a share lock right now, try to upgrade to SHARE_EXCLUSIVE.
+ */
+ Assert(mode == BUFFER_LOCK_SHARE);
+
+ old_state = pg_atomic_read_u64(&buf_hdr->state);
+ while (true)
+ {
+ uint64 desired_state;
+
+ desired_state = old_state;
+
+ /*
+ * Can't upgrade if somebody else holds the lock in exlusive or
+ * share-exclusive mode.
+ */
+ if (unlikely((old_state & (BM_LOCK_VAL_EXCLUSIVE | BM_LOCK_VAL_SHARE_EXCLUSIVE)) != 0))
+ {
+ return false;
+ }
+
+ /* currently held lock state */
+ desired_state -= BM_LOCK_VAL_SHARED;
+
+ /* new lock level */
+ desired_state += BM_LOCK_VAL_SHARE_EXCLUSIVE;
+
+ if (likely(pg_atomic_compare_exchange_u64(&buf_hdr->state,
+ &old_state, desired_state)))
+ {
+ ref->data.lockmode = BUFFER_LOCK_SHARE_EXCLUSIVE;
+ *lockstate = desired_state;
+
+ return true;
+ }
+ }
+
+}
+
+bool
+BufferSetHintBits16(uint16 *ptr, uint16 val, Buffer buffer)
+{
+ BufferDesc *buf_hdr;
+ uint64 lockstate;
+
+ if (BufferIsLocal(buffer))
+ {
+ *ptr = val;
+
+ MarkLocalBufferDirty(buffer);
+
+ return true;
+ }
+
+ buf_hdr = GetBufferDescriptor(buffer - 1);
+
+ if (SharedBufferBeginSetHintBits(buffer, buf_hdr, &lockstate))
+ {
+ *ptr = val;
+
+ MarkSharedBufferDirtyHint(buffer, buf_hdr, lockstate, true);
+
+ return true;
+ }
+
+ return false;
+}
+
+bool
+BufferBeginSetHintBits(Buffer buffer)
+{
+ BufferDesc *buf_hdr;
+ uint64 lockstate;
+
+ if (BufferIsLocal(buffer))
+ {
+ /*
+ * TODO: will need to check for write IO once that's done
+ * asynchronously.
+ */
+
+ return true;
+ }
+
+ buf_hdr = GetBufferDescriptor(buffer - 1);
+
+ return SharedBufferBeginSetHintBits(buffer, buf_hdr, &lockstate);
+}
+
+void
+BufferFinishSetHintBits(Buffer buffer, bool mark_dirty, bool buffer_std)
+{
+ if (mark_dirty)
+ MarkBufferDirtyHint(buffer, buffer_std);
+}
/*
* Functions for buffer I/O handling
diff --git a/src/backend/storage/freespace/freespace.c b/src/backend/storage/freespace/freespace.c
index 4773a9cc65e..6cdfbbeb260 100644
--- a/src/backend/storage/freespace/freespace.c
+++ b/src/backend/storage/freespace/freespace.c
@@ -904,14 +904,22 @@ fsm_vacuum_page(Relation rel, FSMAddress addr,
max_avail = fsm_get_max_avail(page);
/*
- * Reset the next slot pointer. This encourages the use of low-numbered
- * pages, increasing the chances that a later vacuum can truncate the
- * relation. We don't bother with a lock here, nor with marking the page
- * dirty if it wasn't already, since this is just a hint.
+ * Try to reset the next slot pointer. This encourages the use of
+ * low-numbered pages, increasing the chances that a later vacuum can
+ * truncate the relation. We don't bother with a lock here, nor with
+ * marking the page dirty if it wasn't already, since this is just a hint.
+ *
+ * To be allowed to update the page without an exclusive lock, we have to
+ * use the hint bit infrastructure.
*/
- ((FSMPage) PageGetContents(page))->fp_next_slot = 0;
+ LockBuffer(buf, BUFFER_LOCK_SHARE);
+ if (BufferBeginSetHintBits(buf))
+ {
+ ((FSMPage) PageGetContents(page))->fp_next_slot = 0;
+ BufferFinishSetHintBits(buf, false, false);
+ }
- ReleaseBuffer(buf);
+ UnlockReleaseBuffer(buf);
return max_avail;
}
diff --git a/src/backend/storage/freespace/fsmpage.c b/src/backend/storage/freespace/fsmpage.c
index 66a5c80b5a6..a59696b6484 100644
--- a/src/backend/storage/freespace/fsmpage.c
+++ b/src/backend/storage/freespace/fsmpage.c
@@ -298,9 +298,18 @@ restart:
* lock and get a garbled next pointer every now and then, than take the
* concurrency hit of an exclusive lock.
*
+ * Without an exclusive lock, we need to use the hint bit infrastructure
+ * to be allowed to modify the page.
+ *
* Wrap-around is handled at the beginning of this function.
*/
- fsmpage->fp_next_slot = slot + (advancenext ? 1 : 0);
+ if (exclusive_lock_held || BufferBeginSetHintBits(buf))
+ {
+ fsmpage->fp_next_slot = slot + (advancenext ? 1 : 0);
+
+ if (!exclusive_lock_held)
+ BufferFinishSetHintBits(buf, false, true);
+ }
return slot;
}
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 9d14239b4c4..66e8f2e9fa6 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2731,6 +2731,7 @@ SetConstraintStateData
SetConstraintTriggerData
SetExprState
SetFunctionReturnMode
+SetHintBitsState
SetOp
SetOpCmd
SetOpPath
--
2.48.1.76.g4e746b1a31.dirty
[text/x-diff] v6-0013-WIP-Make-UnlockReleaseBuffer-more-efficient.patch (3.5K, ../6rgb2nvhyvnszz4ul3wfzlf5rheb2kkwrglthnna7qhe24onwr@vw27225tkyar/14-v6-0013-WIP-Make-UnlockReleaseBuffer-more-efficient.patch)
download | inline diff:
From 1c24af4fedb372b9b89d0635895860889da86b47 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 19 Nov 2025 15:32:20 -0500
Subject: [PATCH v6 13/14] WIP: Make UnlockReleaseBuffer() more efficient
Now that the buffer content lock is implemented as part of BufferDesc.state,
releasing the lock and unpinning the buffer can be implemented as a single
atomic operation.
Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
src/backend/access/nbtree/nbtpage.c | 22 +++++++++++-
src/backend/storage/buffer/bufmgr.c | 52 ++++++++++++++++++++++++++++-
2 files changed, 72 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index 30b43a4dd18..2fd8141854c 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -1006,11 +1006,18 @@ _bt_relandgetbuf(Relation rel, Buffer obuf, BlockNumber blkno, int access)
Assert(BlockNumberIsValid(blkno));
if (BufferIsValid(obuf))
+ {
+ _bt_relbuf(rel, obuf);
+#if 0
+ Assert(BufferGetBlockNumber(obuf) != blkno);
_bt_unlockbuf(rel, obuf);
- buf = ReleaseAndReadBuffer(obuf, rel, blkno);
+#endif
+ }
+ buf = ReadBuffer(rel, blkno);
_bt_lockbuf(rel, buf, access);
_bt_checkpage(rel, buf);
+
return buf;
}
@@ -1022,8 +1029,21 @@ _bt_relandgetbuf(Relation rel, Buffer obuf, BlockNumber blkno, int access)
void
_bt_relbuf(Relation rel, Buffer buf)
{
+#if 0
_bt_unlockbuf(rel, buf);
ReleaseBuffer(buf);
+#else
+ /*
+ * Buffer is pinned and locked, which means that it is expected to be
+ * defined and addressable. Check that proactively.
+ */
+ VALGRIND_CHECK_MEM_IS_DEFINED(BufferGetPage(buf), BLCKSZ);
+
+ UnlockReleaseBuffer(buf);
+
+ if (!RelationUsesLocalBuffers(rel))
+ VALGRIND_MAKE_MEM_NOACCESS(BufferGetPage(buf), BLCKSZ);
+#endif
}
/*
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 9ed7a368d74..584c3b2ee75 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -5437,13 +5437,63 @@ ReleaseBuffer(Buffer buffer)
/*
* UnlockReleaseBuffer -- release the content lock and pin on a buffer
*
- * This is just a shorthand for a common combination.
+ * This is just a, more efficient, shorthand for a common combination.
*/
void
UnlockReleaseBuffer(Buffer buffer)
{
+#if 1
+ int mode;
+ BufferDesc *buf;
+ PrivateRefCountEntry *ref;
+ uint64 sub;
+ uint64 lockstate;
+
+ if (!BufferIsValid(buffer))
+ elog(ERROR, "bad buffer ID: %d", buffer);
+
+ if (BufferIsLocal(buffer))
+ {
+ UnpinLocalBuffer(buffer);
+ return;
+ }
+
+ ResourceOwnerForgetBuffer(CurrentResourceOwner, buffer);
+
+ buf = GetBufferDescriptor(buffer - 1);
+
+ mode = BufferLockDisownInternal(buffer, buf);
+
+ /* compute state modification for lock release */
+ sub = BufferLockReleaseSub(mode);
+
+ /* compute state modification for pin release */
+ ref = GetPrivateRefCountEntry(buffer, false);
+ Assert(ref != NULL);
+ Assert(ref->data.refcount > 0);
+ ref->data.refcount--;
+
+ if (ref->data.refcount == 0)
+ {
+ sub |= BUF_REFCOUNT_ONE;
+ ForgetPrivateRefCountEntry(ref);
+ }
+
+ /* perform the lock and pin release in one atomic op */
+ lockstate = pg_atomic_sub_fetch_u64(&buf->state, sub);
+
+ /* wake up waiters etc */
+ BufferLockProcessRelease(buf, mode, lockstate);
+
+ if (lockstate & BM_PIN_COUNT_WAITER)
+ WakePinCountWaiter(buf);
+
+ RESUME_INTERRUPTS();
+
+#else
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
ReleaseBuffer(buffer);
+#endif
}
/*
--
2.48.1.76.g4e746b1a31.dirty
[text/x-diff] v6-0014-WIP-bufmgr-Don-t-copy-pages-while-writing-out.patch (11.6K, ../6rgb2nvhyvnszz4ul3wfzlf5rheb2kkwrglthnna7qhe24onwr@vw27225tkyar/15-v6-0014-WIP-bufmgr-Don-t-copy-pages-while-writing-out.patch)
download | inline diff:
From 8b73c9143118487ee3e7663b4adb050db99b86d8 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Thu, 17 Oct 2024 14:14:35 -0400
Subject: [PATCH v6 14/14] WIP: bufmgr: Don't copy pages while writing out
After the series of preceding commits introducing and using
BufferBeginSetHintBits()/BufferSetHintBits16() hint bits are not set
anymore while IO is going on. Therefore we do not need to copy pages while
they are being written out anymore.
TODO: Update comments
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
src/include/storage/bufpage.h | 3 +-
src/backend/access/hash/hashpage.c | 2 +-
src/backend/access/transam/xloginsert.c | 43 ++++++----------------
src/backend/storage/buffer/bufmgr.c | 21 +++++------
src/backend/storage/buffer/localbuf.c | 2 +-
src/backend/storage/page/bufpage.c | 48 ++++---------------------
src/backend/storage/smgr/bulk_write.c | 2 +-
src/test/modules/test_aio/test_aio.c | 2 +-
8 files changed, 33 insertions(+), 90 deletions(-)
diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h
index abc2cf2a020..f8f621446c4 100644
--- a/src/include/storage/bufpage.h
+++ b/src/include/storage/bufpage.h
@@ -504,7 +504,6 @@ extern void PageIndexMultiDelete(Page page, OffsetNumber *itemnos, int nitems);
extern void PageIndexTupleDeleteNoCompact(Page page, OffsetNumber offnum);
extern bool PageIndexTupleOverwrite(Page page, OffsetNumber offnum,
const void *newtup, Size newsize);
-extern char *PageSetChecksumCopy(Page page, BlockNumber blkno);
-extern void PageSetChecksumInplace(Page page, BlockNumber blkno);
+extern void PageSetChecksum(Page page, BlockNumber blkno);
#endif /* BUFPAGE_H */
diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c
index b8e5bd005e5..dd17eff59d1 100644
--- a/src/backend/access/hash/hashpage.c
+++ b/src/backend/access/hash/hashpage.c
@@ -1029,7 +1029,7 @@ _hash_alloc_buckets(Relation rel, BlockNumber firstblock, uint32 nblocks)
zerobuf.data,
true);
- PageSetChecksumInplace(page, lastblock);
+ PageSetChecksum(page, lastblock);
smgrextend(RelationGetSmgr(rel), MAIN_FORKNUM, lastblock, zerobuf.data,
false);
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index a56d5a55282..0af148e9496 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -261,8 +261,11 @@ XLogRegisterBuffer(uint8 block_id, Buffer buffer, uint8 flags)
*/
#ifdef USE_ASSERT_CHECKING
if (!(flags & REGBUF_NO_CHANGE))
- Assert(BufferIsLockedByMeInMode(buffer, BUFFER_LOCK_EXCLUSIVE) &&
- BufferIsDirty(buffer));
+ {
+ Assert(BufferIsDirty(buffer));
+ Assert(BufferIsLockedByMeInMode(buffer, BUFFER_LOCK_EXCLUSIVE) ||
+ BufferIsLockedByMeInMode(buffer, BUFFER_LOCK_SHARE_EXCLUSIVE));
+ }
#endif
if (block_id >= max_registered_block_id)
@@ -1066,7 +1069,7 @@ XLogCheckBufferNeedsBackup(Buffer buffer)
* Write a backup block if needed when we are setting a hint. Note that
* this may be called for a variety of page types, not just heaps.
*
- * Callable while holding just share lock on the buffer content.
+ * Callable while holding just share-exclusive lock on the buffer content.
*
* We can't use the plain backup block mechanism since that relies on the
* Buffer being exclusively locked. Since some modifications (setting LSN, hint
@@ -1074,6 +1077,8 @@ XLogCheckBufferNeedsBackup(Buffer buffer)
* failures. So instead we copy the page and insert the copied data as normal
* record data.
*
+ * FIXME: outdated
+ *
* We only need to do something if page has not yet been full page written in
* this checkpoint round. The LSN of the inserted wal record is returned if we
* had to write, InvalidXLogRecPtr otherwise.
@@ -1102,46 +1107,20 @@ XLogSaveBufferForHint(Buffer buffer, bool buffer_std)
/*
* We assume page LSN is first data on *every* page that can be passed to
- * XLogInsert, whether it has the standard page layout or not. Since we're
- * only holding a share-lock on the page, we must take the buffer header
- * lock when we look at the LSN.
+ * XLogInsert, whether it has the standard page layout or not.
*/
lsn = BufferGetLSNAtomic(buffer);
if (lsn <= RedoRecPtr)
{
- int flags = 0;
- PGAlignedBlock copied_buffer;
- char *origdata = (char *) BufferGetBlock(buffer);
- RelFileLocator rlocator;
- ForkNumber forkno;
- BlockNumber blkno;
-
- /*
- * Copy buffer so we don't have to worry about concurrent hint bit or
- * lsn updates. We assume pd_lower/upper cannot be changed without an
- * exclusive lock, so the contents bkp are not racy.
- */
- if (buffer_std)
- {
- /* Assume we can omit data between pd_lower and pd_upper */
- Page page = BufferGetPage(buffer);
- uint16 lower = ((PageHeader) page)->pd_lower;
- uint16 upper = ((PageHeader) page)->pd_upper;
-
- memcpy(copied_buffer.data, origdata, lower);
- memcpy(copied_buffer.data + upper, origdata + upper, BLCKSZ - upper);
- }
- else
- memcpy(copied_buffer.data, origdata, BLCKSZ);
+ int flags = REGBUF_NO_CHANGE;
XLogBeginInsert();
if (buffer_std)
flags |= REGBUF_STANDARD;
- BufferGetTag(buffer, &rlocator, &forkno, &blkno);
- XLogRegisterBlock(0, &rlocator, forkno, blkno, copied_buffer.data, flags);
+ XLogRegisterBuffer(0, buffer, flags);
recptr = XLogInsert(RM_XLOG_ID, XLOG_FPI_FOR_HINT);
}
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 584c3b2ee75..d6a638613ae 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -4342,7 +4342,6 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object,
ErrorContextCallback errcallback;
instr_time io_start;
Block bufBlock;
- char *bufToWrite;
uint64 buf_state;
Assert(BufferLockHeldByMeInMode(buf, BUFFER_LOCK_EXCLUSIVE) ||
@@ -4413,12 +4412,8 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object,
*/
bufBlock = BufHdrGetBlock(buf);
- /*
- * Update page checksum if desired. Since we have only shared lock on the
- * buffer, other processes might be updating hint bits in it, so we must
- * copy the page to private storage if we do checksumming.
- */
- bufToWrite = PageSetChecksumCopy((Page) bufBlock, buf->tag.blockNum);
+ /* Update page checksum if desired. */
+ PageSetChecksum((Page) bufBlock, buf->tag.blockNum);
io_start = pgstat_prepare_io_time(track_io_timing);
@@ -4428,7 +4423,7 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object,
smgrwrite(reln,
BufTagGetForkNum(&buf->tag),
buf->tag.blockNum,
- bufToWrite,
+ bufBlock,
false);
/*
@@ -4552,8 +4547,8 @@ BufferIsPermanent(Buffer buffer)
/*
* BufferGetLSNAtomic
* Retrieves the LSN of the buffer atomically using a buffer header lock.
- * This is necessary for some callers who may not have an exclusive lock
- * on the buffer.
+ * This is necessary for some callers who may not have a (share-)exclusive
+ * lock on the buffer.
*/
XLogRecPtr
BufferGetLSNAtomic(Buffer buffer)
@@ -5606,6 +5601,12 @@ MarkSharedBufferDirtyHint(Buffer buffer, BufferDesc *bufHdr, uint64 lockstate, b
* It's possible we may enter here without an xid, so it is
* essential that CreateCheckPoint waits for virtual transactions
* rather than full transactionids.
+ *
+ * FIXME: I think we now should simply mark the page dirty before
+ * WAL logging the hint bit - afaikt it then should work just like
+ * any other buffer write (due to SyncBuffers()/SyncOneBuffer()
+ * seeing the dirty bit and trying to lock the page
+ * share-exclusive, and thus having to wait).
*/
Assert((MyProc->delayChkptFlags & DELAY_CHKPT_START) == 0);
MyProc->delayChkptFlags |= DELAY_CHKPT_START;
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index a41a5facd3a..5826d4b54c6 100644
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -199,7 +199,7 @@ FlushLocalBuffer(BufferDesc *bufHdr, SMgrRelation reln)
reln = smgropen(BufTagGetRelFileLocator(&bufHdr->tag),
MyProcNumber);
- PageSetChecksumInplace(localpage, bufHdr->tag.blockNum);
+ PageSetChecksum(localpage, bufHdr->tag.blockNum);
io_start = pgstat_prepare_io_time(track_io_timing);
diff --git a/src/backend/storage/page/bufpage.c b/src/backend/storage/page/bufpage.c
index aac6e695954..c8cbdd1f7a6 100644
--- a/src/backend/storage/page/bufpage.c
+++ b/src/backend/storage/page/bufpage.c
@@ -1494,51 +1494,15 @@ PageIndexTupleOverwrite(Page page, OffsetNumber offnum,
/*
* Set checksum for a page in shared buffers.
*
- * If checksums are disabled, or if the page is not initialized, just return
- * the input. Otherwise, we must make a copy of the page before calculating
- * the checksum, to prevent concurrent modifications (e.g. setting hint bits)
- * from making the final checksum invalid. It doesn't matter if we include or
- * exclude hints during the copy, as long as we write a valid page and
- * associated checksum.
+ * If checksums are disabled, or if the page is not initialized, just
+ * return. Otherwise compute and set the checksum.
*
- * Returns a pointer to the block-sized data that needs to be written. Uses
- * statically-allocated memory, so the caller must immediately write the
- * returned page and not refer to it again.
- */
-char *
-PageSetChecksumCopy(Page page, BlockNumber blkno)
-{
- static char *pageCopy = NULL;
-
- /* If we don't need a checksum, just return the passed-in data */
- if (PageIsNew(page) || !DataChecksumsEnabled())
- return page;
-
- /*
- * We allocate the copy space once and use it over on each subsequent
- * call. The point of palloc'ing here, rather than having a static char
- * array, is first to ensure adequate alignment for the checksumming code
- * and second to avoid wasting space in processes that never call this.
- */
- if (pageCopy == NULL)
- pageCopy = MemoryContextAllocAligned(TopMemoryContext,
- BLCKSZ,
- PG_IO_ALIGN_SIZE,
- 0);
-
- memcpy(pageCopy, page, BLCKSZ);
- ((PageHeader) pageCopy)->pd_checksum = pg_checksum_page(pageCopy, blkno);
- return pageCopy;
-}
-
-/*
- * Set checksum for a page in private memory.
- *
- * This must only be used when we know that no other process can be modifying
- * the page buffer.
+ * In the past this needed to be done on a copy of the page, due to the
+ * possibility of e.g. hint bits being set concurrently. However, this is not
+ * necessary anymore as hint bits won't be set while IO is going on.
*/
void
-PageSetChecksumInplace(Page page, BlockNumber blkno)
+PageSetChecksum(Page page, BlockNumber blkno)
{
/* If we don't need a checksum, just return */
if (PageIsNew(page) || !DataChecksumsEnabled())
diff --git a/src/backend/storage/smgr/bulk_write.c b/src/backend/storage/smgr/bulk_write.c
index b958be15716..f4d07543365 100644
--- a/src/backend/storage/smgr/bulk_write.c
+++ b/src/backend/storage/smgr/bulk_write.c
@@ -279,7 +279,7 @@ smgr_bulk_flush(BulkWriteState *bulkstate)
BlockNumber blkno = pending_writes[i].blkno;
Page page = pending_writes[i].buf->data;
- PageSetChecksumInplace(page, blkno);
+ PageSetChecksum(page, blkno);
if (blkno >= bulkstate->relsize)
{
diff --git a/src/test/modules/test_aio/test_aio.c b/src/test/modules/test_aio/test_aio.c
index 488d98e7e66..e5fc7642dc2 100644
--- a/src/test/modules/test_aio/test_aio.c
+++ b/src/test/modules/test_aio/test_aio.c
@@ -288,7 +288,7 @@ modify_rel_block(PG_FUNCTION_ARGS)
}
else
{
- PageSetChecksumInplace(page, blkno);
+ PageSetChecksum(page, blkno);
}
smgrwrite(RelationGetSmgr(rel),
--
2.48.1.76.g4e746b1a31.dirty
view thread (120+ messages) latest in thread
Message-ID: <6rgb2nvhyvnszz4ul3wfzlf5rheb2kkwrglthnna7qhe24onwr@vw27225tkyar>
Permalink: ../6rgb2nvhyvnszz4ul3wfzlf5rheb2kkwrglthnna7qhe24onwr@vw27225tkyar/
Also on: postgresql.org/message-id/6rgb2nvhyvnszz4ul3wfzlf5rheb2kkwrglthnna7qhe24onwr@vw27225tkyar
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: pgsql-hackers@postgresql.org
Cc: andres@anarazel.de, boekewurm+postgres@gmail.com, melanieplageman@gmail.com, thomas.munro@gmail.com, hlinnaka@iki.fi, noah@leadboat.com, robertmhaas@gmail.com, michael.paquier@gmail.com
Subject: Re: Buffer locking is special (hints, checksums, AIO writes)
In-Reply-To: <6rgb2nvhyvnszz4ul3wfzlf5rheb2kkwrglthnna7qhe24onwr@vw27225tkyar>
* 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