Re: BUG #15321: XMLTABLE leaks memory like crazy

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

-- 
Andrew (irc:RhodiumToad)

diff --git a/src/backend/executor/nodeTableFuncscan.c b/src/backend/executor/nodeTableFuncscan.c
index b03d2ef762..222711bb5a 100644
--- a/src/backend/executor/nodeTableFuncscan.c
+++ b/src/backend/executor/nodeTableFuncscan.c
@@ -288,6 +288,9 @@ tfuncFetchRows(TableFuncScanState *tstate, ExprContext *econtext)
     oldcxt = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
     tstate->tupstore = tuplestore_begin_heap(false, false, work_mem);
 
+    MemoryContextReset(tstate->perValueCxt);
+    MemoryContextSwitchTo(tstate->perValueCxt);
+
     PG_TRY();
     {
         routine->InitOpaque(tstate,
@@ -319,8 +322,7 @@ tfuncFetchRows(TableFuncScanState *tstate, ExprContext *econtext)
     }
     PG_END_TRY();
 
-    /* return to original memory context, and clean up */
-    MemoryContextSwitchTo(oldcxt);
+    /* clean up and return to original memory context */
 
     if (tstate->opaque != NULL)
     {
@@ -328,6 +330,9 @@ tfuncFetchRows(TableFuncScanState *tstate, ExprContext *econtext)
         tstate->opaque = NULL;
     }
 
+    MemoryContextSwitchTo(oldcxt);
+    MemoryContextReset(tstate->perValueCxt);
+
     return;
 }
 
@@ -433,7 +438,7 @@ tfuncLoadRows(TableFuncScanState *tstate, ExprContext *econtext)
 
     ordinalitycol =
         ((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.
@@ -496,7 +501,7 @@ tfuncLoadRows(TableFuncScanState *tstate, ExprContext *econtext)
 
         tuplestore_putvalues(tstate->tupstore, tupdesc, values, nulls);
 
-        MemoryContextReset(tstate->perValueCxt);
+        MemoryContextReset(econtext->ecxt_per_tuple_memory);
     }
 
     MemoryContextSwitchTo(oldcxt);