Re: [bug fix] Memory leak in dblink

Joe Conway <mail(at)joeconway(dot)com> writes:
> On 06/18/2014 12:09 PM, Tom Lane wrote:
>> I find myself a bit suspicious of this whole thing though. If
>> it's necessary to explicitly clean up the tmpcontext, why not also
>> the sinfo->cstrs allocation? And what about the tupdesc,
>> attinmeta, etc created further up in that "if (first)" block? I'd
>> have expected that all this stuff gets allocated in a context
>> that's short-lived enough that we don't really need to clean it up
>> explicitly.

> Yeah, I thought about that too. My testing showed a small amount of
> remaining leakage -- like 20 MB on 100,000 iterations -- I wasn't sure
> that it was worthwhile to worry about. The memory context leak was
> much larger.

[ Thinks for awhile... ] Ah. The memory context is a child of
the econtext's ecxt_per_tuple_memory, which is supposed to be
short-lived, but we only do MemoryContextReset() on that after
each tuple, not MemoryContextResetAndDeleteChildren(). I recall
there's been debate about changing that, but it's not something
we can risk changing in back branches, for sure. So I concur
we need an explicit context delete here.

I do see growth in the per-query context as well. I'm not sure
where that's coming from, but we probably should try to find out.
A couple hundred bytes per iteration is going to add up, even if it's
not as fast as 8K per iteration. I'm not sure it's dblink's fault,
because I don't see anything in dblink.c that is allocating anything in
the per-query context, except for the returned tuplestores, which
somebody else should clean up.

regards, tom lane