From 353b4d30f9b8cf014d2a2b851ae989301fd894ab Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Thu, 9 Oct 2025 16:27:08 -0400 Subject: [PATCH v5 1/3] bufmgr: Fix valgrind checking for buffers pinned in StrategyGetBuffer() In 5e899859287 I made StrategyGetBuffer() pin buffers with a single CAS, instead of using PinBuffer_Locked(). Unfortunately I missed that PinBuffer_Locked() marked the page as defined for valgrind. Fix this oversight by centralizing the valgrind initialization into TrackNewBufferPin(), which also allows us to reduce the number of places doing VALGRIND_MAKE_MEM_DEFINED. Per buildfarm animal skink and Amit Langote. Discussion: https://postgr.es/m/fvfmkr5kk4nyex56ejgxj3uzi63isfxovp2biecb4bspbjrze7@az2pljabhnff Discussion: https://postgr.es/m/CA+HiwqGKJ6nEXEPQW7EpykVsEtzxp5-up_xhtcUAkWFtATVQvQ@mail.gmail.com --- src/backend/storage/buffer/bufmgr.c | 32 ++++++++++++++--------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index ca62b9cdcf0..edf17ce3ea1 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -3113,15 +3113,6 @@ PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy, result = (buf_state & BM_VALID) != 0; TrackNewBufferPin(b); - - /* - * Assume that we acquired a buffer pin for the purposes of - * Valgrind buffer client checks (even in !result case) to - * keep things simple. Buffers that are unsafe to access are - * not generally guaranteed to be marked undefined or - * non-accessible in any case. - */ - VALGRIND_MAKE_MEM_DEFINED(BufHdrGetBlock(buf), BLCKSZ); break; } } @@ -3186,13 +3177,6 @@ PinBuffer_Locked(BufferDesc *buf) */ Assert(GetPrivateRefCountEntry(BufferDescriptorGetBuffer(buf), false) == NULL); - /* - * Buffer can't have a preexisting pin, so mark its page as defined to - * Valgrind (this is similar to the PinBuffer() case where the backend - * doesn't already have a buffer pin) - */ - VALGRIND_MAKE_MEM_DEFINED(BufHdrGetBlock(buf), BLCKSZ); - /* * Since we hold the buffer spinlock, we can update the buffer state and * release the lock in one operation. @@ -3320,6 +3304,10 @@ UnpinBufferNoOwner(BufferDesc *buf) } } +/* + * Set up backend-local tracking of a buffer pinned the first time by this + * backend. + */ inline void TrackNewBufferPin(Buffer buf) { @@ -3329,6 +3317,18 @@ TrackNewBufferPin(Buffer buf) ref->refcount++; ResourceOwnerRememberBuffer(CurrentResourceOwner, buf); + + /* + * This is the first pin for this page by this backend, mark its page as + * defined to valgrind. While the page contents might not actually be + * valid yet, we don't currently guarantee that such pages are marked + * undefined or non-accessible. + * + * It's not necessarily the prettiest to do this here, but otherwise we'd + * need this block of code in multiple places. + */ + VALGRIND_MAKE_MEM_DEFINED(BufHdrGetBlock(GetBufferDescriptor(buf - 1)), + BLCKSZ); } #define ST_SORT sort_checkpoint_bufferids -- 2.48.1.76.g4e746b1a31.dirty