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
