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