Re: BUG #16112: large, unexpected memory consumption

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