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