Getting better results from valgrind leak tracking

[ starting a new thread for this ]

Andres Freund <andres(at)anarazel(dot)de> writes:
> I wonder if it'd be worth starting to explicitly annotate all the places
> that do allocations and are fine with leaking them. E.g. by introducing
> malloc_permanently() or such. Right now it's hard to use valgrind et al
> to detect leaks because of all the false positives due to such "ok to
> leak" allocations.

Out of curiosity I poked at this for a little while. It doesn't appear
to me that we leak much at all, at least not if you are willing to take
"still reachable" blocks as not-leaked. Most of the problem is
more subtle than that.

I found just a couple of things that really seem like leaks of permanent
data structures to valgrind:

* Where ps_status.c copies the original "environ" array (on
PS_USE_CLOBBER_ARGV platforms), valgrind thinks that's all leaked,
implying that it doesn't count the "environ" global as a valid
reference to leakable data. I was able to shut that up by also saving
the pointer into an otherwise-unused static variable. (This is sort of
a poor man's implementation of your "malloc_permanently" idea; but I
doubt it's worth working harder, given the small number of use-cases.)

* The postmaster's sock_paths and lock_files lists appear to be leaked,
but we're doing that to ourselves by throwing away the pointers to them
without physically freeing the lists. We can just not do that.

What I found out is that we have a lot of issues that seem to devolve
to valgrind not being sure that a block is referenced. I identified
two main causes of that:

(1) We have a pointer, but it doesn't actually point right at the start
of the block. A primary culprit here is lists of thingies that use the
slist and dlist infrastructure. As an experiment, I moved the dlist_node
fields of some popular structs to the beginning, and verified that that
silences associated complaints. I'm not sure that we want to insist on
put-the-link-first as policy (although if we did, it could provide some
notational savings perhaps). However, unless someone knows of a way to
teach valgrind about this situation, there may be no other way to silence
those leakage complaints. A secondary culprit is people randomly applying
CACHELINEALIGN or the like to a palloc'd address, so that the address we
have isn't pointing right at the block start.

(2) The only pointer to the start of a block is actually somewhere within
the block. This is common in dynahash tables, where we allocate a slab
of entries in a single palloc and then thread them together. Each entry
should have exactly one referencing pointer, but that pointer is more
likely to be elsewhere within the same palloc block than in the external
hash bucket array. AFAICT, all cases except where the slab's first entry
is pointed to by a hash bucket pointer confuse valgrind to some extent.
I was able to hack around this by preventing dynahash from allocating
more than one hash entry per palloc, but I wonder if there's a better way.

Attached is a very crude hack, not meant for commit, that hacks things
up enough to greatly reduce the number of complaints with
"--leak-check=full".

One thing I've failed to silence so far is a bunch of entries like

==00:00:03:56.088 3467702== 1,861 bytes in 67 blocks are definitely lost in loss record 1,290 of 1,418
==00:00:03:56.088 3467702== at 0x950650: MemoryContextAlloc (mcxt.c:827)
==00:00:03:56.088 3467702== by 0x951710: MemoryContextStrdup (mcxt.c:1179)
==00:00:03:56.088 3467702== by 0x91C86E: RelationInitIndexAccessInfo (relcache.c:1444)
==00:00:03:56.088 3467702== by 0x91DA9C: RelationBuildDesc (relcache.c:1200)

which is complaining about the memory context identifiers for system
indexes' rd_indexcxt contexts. Those are surely not being leaked in
any real sense. I suspect that this has something to do with valgrind
not counting the context->ident fields as live pointers, but I don't
have enough valgrind-fu to fix that.

Anyway, the bottom line is that I do not think that we have all that
many uses of the pattern you postulated originally. It's more that
we've designed some valgrind-unfriendly data structures. We need to
improve that situation to make much progress here.

regards, tom lane