Re: BUG #16112: large, unexpected memory consumption

On Thu, Apr 09, 2020 at 08:21:54AM -0400, Ben Cornett wrote:
>Hi,
>
>Did this patch ever get applied? It is not clear to me that it did.
>

Hmmm, I don't think it got applied, which means we've missed yet another
minor release :-(

Andres, any plans to push this?

regards

>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);
>
>
>

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services