Horton

Re: Getting better results from valgrind leak tracking

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
From: "Andres Freund" <andres@anarazel.de>
To: pgsql-hackers@lists.postgresql.org
Date: 2021-03-17T03:50:17Z
Lists: pgsql-hackers 
Hi,

On Tue, Mar 16, 2021, at 20:01, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2021-03-16 19:36:10 -0400, Tom Lane wrote:
> >> 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.
> 
> > Well, I think for any sort of automated testing - which I think would be
> > useful - we'd really need *no* leaks.
> 
> That seems both unnecessary and impractical.  We have to consider that
> everything-still-reachable is an OK final state.

I don't consider "still reachable" a leak. Just definitely unreachable. And with a few tweaks that seems like we could achieve that?


> > I think the run might have shown a genuine leak:
> 
> > ==2048803== 16 bytes in 1 blocks are definitely lost in loss record 139 of 906
> > ==2048803==    at 0x89D2EA: palloc (mcxt.c:975)
> > ==2048803==    by 0x2392D3: heap_beginscan (heapam.c:1198)
> > ==2048803==    by 0x264E8F: table_beginscan_strat (tableam.h:918)
> > ==2048803==    by 0x265994: systable_beginscan (genam.c:453)
> > ==2048803==    by 0x83C2D1: SearchCatCacheMiss (catcache.c:1359)
> > ==2048803==    by 0x83C197: SearchCatCacheInternal (catcache.c:1299)
> 
> I didn't see anything like that after applying the fixes I showed before.
> There are a LOT of false positives from the fact that with our HEAD
> code, valgrind believes that everything in the catalog caches and
> most things in dynahash tables (including the relcache) are unreachable.

I think it's actually unreachable memory (unless you count resetting the cache context), based on manually tracing the code... I'll try to repro.


> > I do see a bunch of leaks bleats below fun:plpgsql_compile that I don't
> > yet understand. E.g.
> 
> Those are probably a variant of what you were suggesting above, ie
> plpgsql isn't terribly careful not to leak random stuff while building
> a long-lived function parse tree.  It's supposed to use a temp context
> for anything that might leak, but I suspect it's not thorough about it.

What I meant was that I didn't understand how there's not a leak danger when compilation fails halfway through, given that the context in question is below TopMemoryContext and that I didn't see a relevant TRY block. But that probably there is something cleaning it up that I didn't see.

Andres

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.