Re: Why our Valgrind reports suck

Hi,

0001:

This is rather wild, I really have only the dimmest memory of that other
thread even though I apparently resorted to reading valgrind's source code...

I think the vchunk/vpool naming, while not necessarily elegant, is helpful.

0002: Makes sense.

0003:
0004:
0005:

Ugh, that's rather gross. Not that I have a better idea. So it's probably
worth doing ...

0006: LGTM

0007:

+ /* Run the rest in xact context to avoid Valgrind leak complaints */
+ MemoryContextSwitchTo(TopTransactionContext);

It seems like it also protects at least somewhat against actual leaks?

0008: LGTM

0009:

Seems like we really ought to do more here. But it's a substantial
improvement, so let's not let perfect be the enemy of good.

0010:
0011:

Not great, but better than not doing anything.

0012:

Hm. Kinda feels like we should just always do it the USE_VALGRIND
approach. ISTM that if domain typecache entry building is a bottleneck, there
are way bigger problems than a copyObject()...

0014:

I guess I personally would try to put the freeing code into a helper, but
it's a close call.

0015:

I'd probably put the list_free() after the
LWLockRelease(LogicalRepWorkerLock)?

0016:

Agreed with the concern stated in the commit message, but addressing that
would obviously be a bigger project.

0017:

A tad weird to leave the comments above the removed = NILs in place, even
though it's obviously still correct.

0018: LGTM.

> But this is the last step to get to zero reported leaks in a run of the core
> regression tests, so let's do it.

I assume that's just about the core tests, not more? I.e. I can't make skink
enable leak checking?

Greetings,

Andres Freund