Re: BUG #15321: XMLTABLE leaks memory like crazy

2018-08-11 5:00 GMT+02:00 Andrew Gierth <andrew@tao11.riddles.org.uk>:

>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.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)