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@anarazel.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