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

diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index ca62b9cdcf039a5106da8ea4bb5bc830e8a9847f..edf17ce3ea14fe46fad38a3cb468d82355c75016 100644 (file)
--- 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