From 1e0dfd166b3fa7fc79e4fad73b6fae056bab598a Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Fri, 17 Jul 2020 17:49:45 -0700 Subject: [PATCH] Add Valgrind buffer access instrumentation. MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Teach Valgrind memcheck to maintain the "defined-ness" of each shared buffer based on whether the backend holds at least one pin at the point it is accessed by access method code. Bugs like the one fixed by commit b0229f26 can be detected using this new instrumentation. Note that backends running with Valgrind naturally have their own independent ideas about whether any given byte in shared memory is safe or unsafe to access. There is no risk that concurrent access by multiple backends to the same shared memory will confuse Valgrind's instrumentation, because everything already works at the process level (or at the memory mapping level, if you prefer). Author: Álvaro Herrera, Peter Geoghegan Reviewed-By: Anastasia Lubennikova Discussion: https://postgr.es/m/20150723195349.GW5596@postgresql.org Discussion: https://postgr.es/m/CAH2-WzkLgyN3zBvRZ1pkNJThC=xi_0gpWRUb_45eexLH1+k2_Q@mail.gmail.com --- src/backend/storage/buffer/bufmgr.c | 18 ++++++++++++++++++ src/include/pg_config_manual.h | 13 +++++++------ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 29c920800a6..8ef073b3821 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -49,6 +49,7 @@ #include "storage/proc.h" #include "storage/smgr.h" #include "storage/standby.h" +#include "utils/memdebug.h" #include "utils/ps_status.h" #include "utils/rel.h" #include "utils/resowner_private.h" @@ -1633,6 +1634,13 @@ PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy) buf_state)) { result = (buf_state & BM_VALID) != 0; + + /* + * If we successfully acquired our first pin on this buffer + * within this backend, mark buffer contents defined + */ + if (result) + VALGRIND_MAKE_MEM_DEFINED(BufHdrGetBlock(buf), BLCKSZ); break; } } @@ -1683,6 +1691,13 @@ 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. @@ -1728,6 +1743,9 @@ UnpinBuffer(BufferDesc *buf, bool fixOwner) uint32 buf_state; uint32 old_buf_state; + /* Mark undefined, now that no pins remain in backend */ + VALGRIND_MAKE_MEM_NOACCESS(BufHdrGetBlock(buf), BLCKSZ); + /* I'd better not still hold any locks on the buffer */ Assert(!LWLockHeldByMe(BufferDescriptorGetContentLock(buf))); Assert(!LWLockHeldByMe(BufferDescriptorGetIOLock(buf))); diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h index 8f3ec6bde18..45b6a457896 100644 --- a/src/include/pg_config_manual.h +++ b/src/include/pg_config_manual.h @@ -269,12 +269,13 @@ /* * Include Valgrind "client requests", mostly in the memory allocator, so * Valgrind understands PostgreSQL memory contexts. This permits detecting - * memory errors that Valgrind would not detect on a vanilla build. See also - * src/tools/valgrind.supp. "make installcheck" runs 20-30x longer under - * Valgrind. Note that USE_VALGRIND slowed older versions of Valgrind by an - * additional order of magnitude; Valgrind 3.8.1 does not have this problem. - * The client requests fall in hot code paths, so USE_VALGRIND also slows - * native execution by a few percentage points. + * memory errors that Valgrind would not detect on a vanilla build. It also + * enables detection of buffer accesses that take place without holding a + * buffer pin. See also src/tools/valgrind.supp. + * + * "make installcheck" is significantly slower under Valgrind. The client + * requests fall in hot code paths, so USE_VALGRIND slows native execution by + * a few percentage points even when not run under Valgrind. * * You should normally use MEMORY_CONTEXT_CHECKING with USE_VALGRIND; * instrumentation of repalloc() is inferior without it. -- 2.39.5