On 2014-05-05 15:41:22 -0400, Tom Lane wrote: > > After far, far too much confused head scratching, code reading, random > > elog()s et al I found out that this is just because of a deficiency in > > valgrind's undefinedness tracking. [...] > > Unfortunately this cannot precisely be caught by valgrind's > > suppressions. Thus I'd like to add memset(SharedInvalidationMessage msg, > > 0) in AddCatcacheInvalidationMessage() et al. to suppress these > > warnings. Imo we can just add them unconditionally, but if somebody else > > prefers we can add #ifdef USE_VALGRIND around them. > > I'd be okay with USE_VALGRIND. I'm not particularly hot on adding a > memset for everybody just to make valgrind less confused. Especially > since that's really going to hide any problems, not fix them.
The attached patch does VALGRIND_MAKE_MEM_DEFINED() on the relevant structs. That's already defined to empty if USE_VALGRIND isn't defined. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
>From 4da7b0caa2058c8b07ee4a7ab529b9ca0c292bcf Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Wed, 7 May 2014 22:30:05 +0200 Subject: [PATCH] Silence a spurious valgrind warning in inval.c. For valgrind's sake explicitly define all bytes of SharedInvalidationMessage as defined before sending them. Otherwise valgrind sometimes treats bytes as undefined that aren't because valgrind doesn't understand that the ringbuffer in sinvaladt.c is accesses by multiple processes. Valgrind remembers that it stored a undefined byte into parts of a slot in the ringbuffer and warns when it uses that byte again - not realizing it has been stored by another process. --- src/backend/utils/cache/inval.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/backend/utils/cache/inval.c b/src/backend/utils/cache/inval.c index 5971469..dd46e18 100644 --- a/src/backend/utils/cache/inval.c +++ b/src/backend/utils/cache/inval.c @@ -103,6 +103,7 @@ #include "storage/smgr.h" #include "utils/catcache.h" #include "utils/inval.h" +#include "utils/memdebug.h" #include "utils/memutils.h" #include "utils/rel.h" #include "utils/relmapper.h" @@ -332,6 +333,14 @@ AddCatcacheInvalidationMessage(InvalidationListHeader *hdr, msg.cc.id = (int8) id; msg.cc.dbId = dbId; msg.cc.hashValue = hashValue; + /* + * Define padding bytes to be defined, otherwise the sinvaladt.c + * ringbuffer, which is accessed by multiple processes, will cause false + * positives because valgrind remembers the undefined bytes from this + * processes store, not realizing that another process has written since. + */ + VALGRIND_MAKE_MEM_DEFINED(&msg, sizeof(msg)); + AddInvalidationMessage(&hdr->cclist, &msg); } @@ -347,6 +356,9 @@ AddCatalogInvalidationMessage(InvalidationListHeader *hdr, msg.cat.id = SHAREDINVALCATALOG_ID; msg.cat.dbId = dbId; msg.cat.catId = catId; + /* check AddCatcacheInvalidationMessage() for an explanation */ + VALGRIND_MAKE_MEM_DEFINED(&msg, sizeof(msg)); + AddInvalidationMessage(&hdr->cclist, &msg); } @@ -370,6 +382,9 @@ AddRelcacheInvalidationMessage(InvalidationListHeader *hdr, msg.rc.id = SHAREDINVALRELCACHE_ID; msg.rc.dbId = dbId; msg.rc.relId = relId; + /* check AddCatcacheInvalidationMessage() for an explanation */ + VALGRIND_MAKE_MEM_DEFINED(&msg, sizeof(msg)); + AddInvalidationMessage(&hdr->rclist, &msg); } @@ -393,6 +408,9 @@ AddSnapshotInvalidationMessage(InvalidationListHeader *hdr, msg.sn.id = SHAREDINVALSNAPSHOT_ID; msg.sn.dbId = dbId; msg.sn.relId = relId; + /* check AddCatcacheInvalidationMessage() for an explanation */ + VALGRIND_MAKE_MEM_DEFINED(&msg, sizeof(msg)); + AddInvalidationMessage(&hdr->rclist, &msg); } @@ -1242,6 +1260,9 @@ CacheInvalidateSmgr(RelFileNodeBackend rnode) msg.sm.backend_hi = rnode.backend >> 16; msg.sm.backend_lo = rnode.backend & 0xffff; msg.sm.rnode = rnode.node; + /* check AddCatcacheInvalidationMessage() for an explanation */ + VALGRIND_MAKE_MEM_DEFINED(&msg, sizeof(msg)); + SendSharedInvalidMessages(&msg, 1); } @@ -1267,6 +1288,9 @@ CacheInvalidateRelmap(Oid databaseId) msg.rm.id = SHAREDINVALRELMAP_ID; msg.rm.dbId = databaseId; + /* check AddCatcacheInvalidationMessage() for an explanation */ + VALGRIND_MAKE_MEM_DEFINED(&msg, sizeof(msg)); + SendSharedInvalidMessages(&msg, 1); } -- 1.8.5.rc2.dirty
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
