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
