Re: BUG #15321: XMLTABLE leaks memory like crazy

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

Pavel

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