On Wed, Nov 13, 2019 at 09:05:28AM -0800, Andres Freund wrote:
>Hi,
>
>On 2019-11-13 15:50:04 +0100, Tomas Vondra wrote:
>> Yeah, I can reproduce this pretty easily. It seems like a memory leak in
>> ExecMakeTableFunctionResult. a9c35cf85ca reworked FunctionCallInfo to be
>> variable-length, but it gets allocated in ExecutorState context directly
>> and so until the end of the query.
>
>Damn. Too bad this got discovered just after the point release was
>wrapped :(
>
>
>> The attached trivial patch fixes that by adding a pfree() at the end of
>> the function.
>
>Hm. That's clearly an improvement. But I'm not quite sure it's really
>the right direction. It seems like a bad idea to rely on
>ExecMakeTableFunctionResult() otherwise never leaking any memory.
>
>It seems to we should be using memory contexts to provide a backstop
>against leaks, like we normally do, instead of operating in the
>per-query context. It's noteworthy that nodeProjectSet already does so.
>In contrast to nodeProjectSet, I think we'd need an additional memory
>context however, as we eagerly evaluate ValuePerCall expressions - and
>we'd like to reset the context after each iteration.
>
Possibly, but I think most of the allocations already use the per-tuple
context. It's just the fcinfo that seems to be allocated outside of it,
so maybe we should just move it to that memory context.
>
>I think we also ought to use SetExprState->fcinfo, instead of allocating
>a separate allocation in ExecMakeTableFunctionResult(). But that's
>perhaps less important.
>
Maybe.
>
>> I wonder if we have the same issue elsewhere ...
>
>Quite possible - but I think in most cases we are using memory contexts
>to protect against leaks like this (which is more efficient than retail
>pfree'ing).
>
Yeah, probably. I had a quick look at some of those places (found by
looking for SizeForFunctionCallInfo and palloc on the same line) and
those that I looked at seemed fine.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
