From 472b75150ffaca1d30147f764208341181c31571 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Fri, 4 Apr 2025 14:55:30 -0400 Subject: [PATCH v1 1/3] 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: Discussion: https://postgr.es/m/3pd4322mogfmdd5nln3zphdwhtmq3rzdldqjwb2sfqzcgs22lf@ok2gletdaoe6 --- src/backend/storage/buffer/bufmgr.c | 13 +++++++++++++ src/backend/storage/buffer/localbuf.c | 9 +++++++++ 2 files changed, 22 insertions(+) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 1c37d7dfe2f..13cebad9b12 100644 --- 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 ed56202af14..fb44d756dad 100644 --- 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,9 @@ 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() */ + VALGRIND_MAKE_MEM_DEFINED(LocalBufHdrGetBlock(buf_hdr), BLCKSZ); } LocalRefCount[bufid]++; ResourceOwnerRememberBuffer(CurrentResourceOwner, @@ -843,6 +849,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); } } -- 2.48.1.76.g4e746b1a31.dirty