localbuf: Add Valgrind buffer access instrumentation

This mirrors 1e0dfd166b3 (+ 46ef520b9566), for temporary table buffers. This
is mainly interesting right now because the AIO work currently triggers
spurious valgrind errors, and the fix for that is cleaner if temp buffers
behave the same as shared buffers.

This requires one change beyond the annotations themselves, namely to pin
local buffers while writing them out in FlushRelationBuffers().

Reviewed-by: Noah Misch <[email protected]>
Co-authored-by: Noah Misch <[email protected]>
Discussion: https://postgr.es/m/3pd4322mogfmdd5nln3zphdwhtmq3rzdldqjwb2sfqzcgs22lf@ok2gletdaoe6

diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index ffaca5ee54dee011eef31b96861361cdfdb18e21..5da121872f436fd45d0d0280b5dd2fbe27528c65 100644 (file)
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -4895,8 +4895,21 @@ FlushRelationBuffers(Relation rel)
                errcallback.previous = error_context_stack;
                error_context_stack = &errcallback;
 
+               /* Make sure we can handle the pin */
+               ReservePrivateRefCountEntry();
+               ResourceOwnerEnlarge(CurrentResourceOwner);
+
+               /*
+                * Pin/upin mostly to make valgrind work, but it also seems
+                * like the right thing to do.
+                */
+               PinLocalBuffer(bufHdr, false);
+
+
                FlushLocalBuffer(bufHdr, srel);
 
+               UnpinLocalBuffer(BufferDescriptorGetBuffer(bufHdr));
+
                /* Pop the error context stack */
                error_context_stack = errcallback.previous;
            }

diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index ed56202af14f613d3fb2dd556e0aca0ea480e53b..63101d56a074b3637d4eca1bcc33ad668fe46970 100644 (file)
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -23,6 +23,7 @@
 #include "storage/bufmgr.h"
 #include "storage/fd.h"
 #include "utils/guc_hooks.h"
+#include "utils/memdebug.h"
 #include "utils/memutils.h"
 #include "utils/resowner.h"
 
@@ -183,6 +184,8 @@ FlushLocalBuffer(BufferDesc *bufHdr, SMgrRelation reln)
    instr_time  io_start;
    Page        localpage = (char *) LocalBufHdrGetBlock(bufHdr);
 
+   Assert(LocalRefCount[-BufferDescriptorGetBuffer(bufHdr) - 1] > 0);
+
    /*
     * Try to start an I/O operation.  There currently are no reasons for
     * StartLocalBufferIO to return false, so we raise an error in that case.
@@ -808,6 +811,15 @@ PinLocalBuffer(BufferDesc *buf_hdr, bool adjust_usagecount)
            buf_state += BUF_USAGECOUNT_ONE;
        }
        pg_atomic_unlocked_write_u32(&buf_hdr->state, buf_state);
+
+       /*
+        * See comment in PinBuffer().
+        *
+        * If the buffer isn't allocated yet, it'll be marked as defined in
+        * GetLocalBufferStorage().
+        */
+       if (LocalBufHdrGetBlock(buf_hdr) != NULL)
+           VALGRIND_MAKE_MEM_DEFINED(LocalBufHdrGetBlock(buf_hdr), BLCKSZ);
    }
    LocalRefCount[bufid]++;
    ResourceOwnerRememberBuffer(CurrentResourceOwner,
@@ -843,6 +855,9 @@ UnpinLocalBufferNoOwner(Buffer buffer)
        Assert(BUF_STATE_GET_REFCOUNT(buf_state) > 0);
        buf_state -= BUF_REFCOUNT_ONE;
        pg_atomic_unlocked_write_u32(&buf_hdr->state, buf_state);
+
+       /* see comment in UnpinBufferNoOwner */
+       VALGRIND_MAKE_MEM_NOACCESS(LocalBufHdrGetBlock(buf_hdr), BLCKSZ);
    }
 }
 
@@ -923,6 +938,16 @@ GetLocalBufferStorage(void)
    next_buf_in_block++;
    total_bufs_allocated++;
 
+   /*
+    * Caller's PinLocalBuffer() was too early for Valgrind updates, so do it
+    * here.  The block is actually undefined, but we want consistency with
+    * the regular case of not needing to allocate memory.  This is
+    * specifically needed when method_io_uring.c fills the block, because
+    * Valgrind doesn't recognize io_uring reads causing undefined memory to
+    * become defined.
+    */
+   VALGRIND_MAKE_MEM_DEFINED(this_buf, BLCKSZ);
+
    return (Block) this_buf;
 }