Re: BUG #15321: XMLTABLE leaks memory like crazy

Hi

2018-08-11 9:02 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

>
>
> 2018-08-11 5:00 GMT+02:00 Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk>:
>
>> >>>>> "Andrew" == Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk> writes:
>>
>> >>> * I removed the "buildercxt" memory context. It seemed mostly
>> >>> pointless, and I was disturbed by the MemoryContextResetOnly().
>> >>> Per-value memory still uses the per-value memory context, but the
>> >>> rest of the stuff is in the per-query context, which should be
>> >>> pretty much the same.
>>
>> Andrew> A quick reading suggests that the per-value context should have
>> Andrew> been changed to not be a child of "buildercxt" (which would
>> Andrew> avoid the MemoryContextResetOnly issue - that's a good sign
>> Andrew> that you've put a child context under the wrong parent). But
>> Andrew> the use of the per-query context instead is exactly what causes
>> Andrew> this blowup; compare with what nodeFunctionscan does with its
>> Andrew> "argcontext" (and the corresponding comments in
>> Andrew> ExecMakeTableFunctionResult).
>>
>> And here's a patch (against pg10, but I don't think the code's changed
>> since) that I think does it the right way.
>>
>> This works by changing the meaning of perValueCxt - it's now the
>> per-INPUT-value context (equivalent to FunctionScan's argcontext, but I
>> didn't change the name for simplicity), not per-output-value; for the
>> latter, we use the result exprcontext's per-tuple memory like everything
>> else does (cf. FunctionScan).
>>
>> I've verified that this fixes the leak; it also passes all other tests I
>> have thrown at it. Anyone see any issues with this? (CC'ing in Pavel as
>> original author)
>>
>> I'll apply it in due course unless anyone says otherwise.
>>
>
> I'll look there this evening.
>
> I am not sure if combination
>
> + MemoryContextReset(tstate->perValueCxt);
> + MemoryContextSwitchTo(tstate->perValueCxt);
>
> is valid. Usually MemoryContext is reset after some action, not before.
> But now, I have not time to look there
>
> Regards
>

+ MemoryContextReset(tstate->perValueCxt);
+ MemoryContextSwitchTo(tstate->perValueCxt);
+
PG_TRY();

The reset of memory context is useless, because the reset of perValueCxt is
there already on the end of tfuncFetchRows function

I don't understand to using tuple memory context

((TableFuncScan *) (tstate->ss.ps.plan))->tablefunc->ordinalitycol;
- oldcxt = MemoryContextSwitchTo(tstate->perValueCxt);
+ oldcxt = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);

/*
* Keep requesting rows from the table builder until there aren't any.
@@ -493,7 +498,7 @@ tfuncLoadRows(TableFuncScanState *tstate, ExprContext
*econtext)

tuplestore_putvalues(tstate->tupstore, tupdesc, values, nulls);

- MemoryContextReset(tstate->perValueCxt);
+ MemoryContextReset(econtext->ecxt_per_tuple_memory);
}

When we are running under perValueCxt, then there changing memory context
is useless

I modified your patch. Please, check it.

Regards

Pavel

> Pavel
>
>
>
>> --
>> Andrew (irc:RhodiumToad)
>>
>>
>