Re: BUG #16112: large, unexpected memory consumption

Hi,

Did this patch ever get applied? It is not clear to me that it did.

Thanks!

On Thu, Nov 14, 2019 at 06:47:07PM -0800, Andres Freund wrote:
> Hi,
>
> On 2019-11-13 13:03:31 -0500, Tom Lane wrote:
> > Andres Freund <andres(at)anarazel(dot)de> writes:
> > > Thinking about it for a second longer, I don't think we'd need a new
> > > context - afaict argcontext has exactly the lifetime requirements
> > > needed.
> >
> > Hm, maybe so. That'd definitely be a better solution if it works.
>
> Here's a patch doing so. I think it'd be a good idea to rename
> argcontext into something like setcontext or such. But as I'm not that
> happy with the name, I've not yet made that change.
>
> Greetings,
>
> Andres Freund

> diff --git i/src/backend/executor/execSRF.c w/src/backend/executor/execSRF.c
> index c8a3efc3654..85f33054265 100644
> --- i/src/backend/executor/execSRF.c
> +++ w/src/backend/executor/execSRF.c
> @@ -114,10 +114,23 @@ ExecMakeTableFunctionResult(SetExprState *setexpr,
> ReturnSetInfo rsinfo;
> HeapTupleData tmptup;
> MemoryContext callerContext;
> - MemoryContext oldcontext;
> bool first_time = true;
>
> - callerContext = CurrentMemoryContext;
> + /*
> + * Execute per-tablefunc actions in appropriate context.
> + *
> + * The FunctionCallInfo needs to live across all the calls to a
> + * ValuePerCall function, so it can't be allocated in the per-tuple
> + * context. Similarly, the function arguments need to be evaluated in a
> + * context that is longer lived than the per-tuple context: The argument
> + * values would otherwise disappear when we reset that context in the
> + * inner loop. As the caller's CurrentMemoryContext is typically a
> + * query-lifespan context, we don't want to leak memory there. We require
> + * the caller to pass a separate memory context that can be used for this,
> + * and can be reset each time through to avoid bloat.
> + */
> + MemoryContextReset(argContext);
> + callerContext = MemoryContextSwitchTo(argContext);
>
> funcrettype = exprType((Node *) setexpr->expr);
>
> @@ -164,20 +177,9 @@ ExecMakeTableFunctionResult(SetExprState *setexpr,
> setexpr->fcinfo->fncollation,
> NULL, (Node *) &rsinfo);
>
> - /*
> - * Evaluate the function's argument list.
> - *
> - * We can't do this in the per-tuple context: the argument values
> - * would disappear when we reset that context in the inner loop. And
> - * the caller's CurrentMemoryContext is typically a query-lifespan
> - * context, so we don't want to leak memory there. We require the
> - * caller to pass a separate memory context that can be used for this,
> - * and can be reset each time through to avoid bloat.
> - */
> - MemoryContextReset(argContext);
> - oldcontext = MemoryContextSwitchTo(argContext);
> + /* evaluate the function's argument list */
> + Assert(CurrentMemoryContext == argContext);
> ExecEvalFuncArgs(fcinfo, setexpr->args, econtext);
> - MemoryContextSwitchTo(oldcontext);
>
> /*
> * If function is strict, and there are any NULL arguments, skip
> @@ -217,7 +219,7 @@ ExecMakeTableFunctionResult(SetExprState *setexpr,
> CHECK_FOR_INTERRUPTS();
>
> /*
> - * reset per-tuple memory context before each call of the function or
> + * Reset per-tuple memory context before each call of the function or
> * expression. This cleans up any local memory the function may leak
> * when called.
> */
> @@ -257,6 +259,8 @@ ExecMakeTableFunctionResult(SetExprState *setexpr,
> */
> if (first_time)
> {
> + MemoryContext oldcontext;
> +
> oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
> tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);
> rsinfo.setResult = tupstore;
> @@ -285,6 +289,8 @@ ExecMakeTableFunctionResult(SetExprState *setexpr,
>
> if (tupdesc == NULL)
> {
> + MemoryContext oldcontext;
> +
> /*
> * This is the first non-NULL result from the
> * function. Use the type info embedded in the
> @@ -378,15 +384,18 @@ no_function_result:
> */
> if (rsinfo.setResult == NULL)
> {
> - MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
> + MemoryContext oldcontext;
> +
> + oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
> tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);
> rsinfo.setResult = tupstore;
> + MemoryContextSwitchTo(oldcontext);
> +
> if (!returnsSet)
> {
> int natts = expectedDesc->natts;
> bool *nullflags;
>
> - MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
> nullflags = (bool *) palloc(natts * sizeof(bool));
> memset(nullflags, true, natts * sizeof(bool));
> tuplestore_putvalues(tupstore, expectedDesc, NULL, nullflags);