Horton

Re: Getting better results from valgrind leak tracking

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@lists.postgresql.org
Date: 2021-03-17T22:07:36Z
Lists: pgsql-hackers 
Andres Freund <andres@anarazel.de> writes:
>> I found a way around that late last night. Need to mark the context
>> itself as an allocation. But I made a mess on the way to that and need
>> to clean the patch up before sending it (and need to drop my
>> girlfriend off first).

> Unfortunately I didn't immediately find a way to do this while keeping
> the MEMPOOL_CREATE/DESTROY in mcxt.c. The attached patch moves the pool
> creation into the memory context implementations, "allocates" the
> context itself as part of that pool, and changes the reset
> implementation from MEMPOOL_DESTROY + MEMPOOL_CREATE to instead do
> MEMPOOL_TRIM. That leaves the memory context itself valid (and thus
> tracking ->ident etc), but marks all the other memory as freed.

Huh, interesting.  I wonder why that makes the ident problem go away?
I'd supposed that valgrind would see the context headers as ordinary
memory belonging to the global "malloc" pool, so that any pointers
inside them ought to be considered valid.

Anyway, I don't have a problem with rearranging the responsibility
like this.  It gives the individual allocators more freedom to do
odd stuff, at the cost of very minor duplication of valgrind calls.

I agree we need more comments -- would you like me to have a go at
writing them?

One thing I was stewing over last night is that a MemoryContextReset
will mess up any context identifier assigned with
MemoryContextCopyAndSetIdentifier.  I'd left that as a problem to
fix later, because we don't currently have a need to reset contexts
that use copied identifiers.  But that assumption obviously will bite
us someday, so maybe now is a good time to think about it.

The very simplest fix would be to allocate non-constant idents with
malloc; which'd require adding a flag to track whether context->ident
needs to be free()d.  We have room for another bool near the top of
struct MemoryContextData (and at some point we could turn those
bool fields into a flags word).  The only real cost here is one
more free() while destroying a labeled context, which is probably
negligible.

Other ideas are possible but they seem to require getting the individual
mcxt methods involved, and I doubt it's worth the complexity.

> There are a few leak warnings around guc.c that look like they might be
> real, not false positives, and thus a bit concerning. Looks like several
> guc check hooks don't bother to free the old *extra before allocating a
> new one.

I'll take a look, but I'm pretty certain that guc.c, not the hooks, is
responsible for freeing those.  Might be another case of valgrind not
understanding what's happening.

> I suspect we might get better results from valgrind, not just for leaks
> but also undefined value tracking, if we changed the way we represent
> pools to utilize VALGRIND_MEMPOOL_METAPOOL |
> VALGRIND_MEMPOOL_AUTO_FREE.

Don't really see why that'd help?  I mean, it could conceivably help
catch bugs in the allocators themselves, but I don't follow the argument
that it'd improve anything else.  Defined is defined, as far as I can
tell from the valgrind manual.

			regards, tom lane

Commits

  1. Improve our support for Valgrind's leak tracking.

  2. Increment xactCompletionCount during subtransaction abort.

  3. Adjust design of per-worker parallel seqscan data struct

  4. Don't leak compiled regex(es) when an ispell cache entry is dropped.

  5. Don't leak malloc'd error string in libpqrcv_check_conninfo().

  6. Don't leak malloc'd strings when a GUC setting is rejected.

  7. Don't leak rd_statlist when a relcache entry is dropped.

  8. Don't run RelationInitTableAccessMethod in a long-lived context.