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.
I think we also ought to use SetExprState->fcinfo, instead of allocating
a separate allocation in ExecMakeTableFunctionResult(). But that's
perhaps less important.
> 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).
Greetings,
Andres Freund