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-18T01:30:48Z
Lists: pgsql-hackers 
Andres Freund <andres@anarazel.de> writes:
> On 2021-03-17 18:07:36 -0400, Tom Lane wrote:
>> Huh, interesting.  I wonder why that makes the ident problem go away?

> I spent a bit of time looking at valgrind, and it looks to be explicit
> behaviour:
>
>    // Our goal is to construct a set of chunks that includes every
>    // mempool chunk, and every malloc region that *doesn't* contain a
>    // mempool chunk.

Ugh.

> I guess that makes sense, but would definitely be nice to have had
> documented...

Indeed.  So this started happening to us when we merged the aset
header with its first allocation block.

>>> 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.

I believe I've just tracked down the cause of that.  Those errors
are (as far as I've seen) only happening in parallel workers, and
the reason is this gem in RestoreGUCState:

	/* See comment at can_skip_gucvar(). */
	for (i = 0; i < num_guc_variables; i++)
		if (!can_skip_gucvar(guc_variables[i]))
			InitializeOneGUCOption(guc_variables[i]);

where InitializeOneGUCOption zeroes out that GUC variable, causing
it to lose track of any allocations it was responsible for.

At minimum, somebody didn't understand the difference between "initialize"
and "reset".  But I suspect we could just nuke this loop altogether.
I've not yet tried to grok the comment that purports to justify it,
but I fail to see why it'd ever be useful to drop values inherited
from the postmaster.

			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.