Re: BUG #15321: XMLTABLE leaks memory like crazy

>>>>> "Pavel" == Pavel Stehule <pavel.stehule@gmail.com> writes:

 >> But that's not "immediately" because tfuncLoadRows is looping over
 >> the FetchRow call, and calling GetValue for each column in that row,
 >> and in your version it is _not cleaning up memory in that loop_.

 Pavel> ok, now I am maybe understand to your motivation.

 Pavel> Usually, loading row should be memory cheap operation, but sure
 Pavel> some bytes it can take.

Yes, it'll usually be small, but you shouldn't assume that (and some
type input functions may use more memory than you think, since doing a
lot of retail pfree() calls can really slow things down).

 Pavel> Then I don't like too much using ecxt_per_tuple_memory for this.

It's there, it has the right lifetime, allocating another one just for
this is a waste. Furthermore, this is the same pattern that FunctionScan
uses, so it's more consistent.

 Pavel> Or do better comments about using this memory context for very
 Pavel> short life task, please.

What specifically do you think needs explaining?

Attached patch is the same logic as before but with more comments.

-- 
Andrew (irc:RhodiumToad)

diff --git a/src/backend/executor/nodeTableFuncscan.c b/src/backend/executor/nodeTableFuncscan.c
index b03d2ef762..d2380995aa 100644
--- a/src/backend/executor/nodeTableFuncscan.c
+++ b/src/backend/executor/nodeTableFuncscan.c
@@ -288,6 +288,17 @@ tfuncFetchRows(TableFuncScanState *tstate, ExprContext *econtext)
     oldcxt = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
     tstate->tupstore = tuplestore_begin_heap(false, false, work_mem);
 
+    /*
+     * Each call to fetch a new set of rows - of which there may be very many
+     * if XMLTABLE is being used in a lateral join - will allocate a possibly
+     * substantial amount of memory, so we cannot use the per-query context
+     * here. perValueCxt now serves the same function as "argcontext" does in
+     * FunctionScan - a place to store per-call lifetime data (as opposed to
+     * per-query or per-result-tuple).
+     */
+    MemoryContextReset(tstate->perValueCxt);
+    MemoryContextSwitchTo(tstate->perValueCxt);
+
     PG_TRY();
     {
         routine->InitOpaque(tstate,
@@ -319,8 +330,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 +338,9 @@ tfuncFetchRows(TableFuncScanState *tstate, ExprContext *econtext)
         tstate->opaque = NULL;
     }
 
+    MemoryContextSwitchTo(oldcxt);
+    MemoryContextReset(tstate->perValueCxt);
+
     return;
 }
 
@@ -433,7 +446,14 @@ tfuncLoadRows(TableFuncScanState *tstate, ExprContext *econtext)
 
     ordinalitycol =
         ((TableFuncScan *) (tstate->ss.ps.plan))->tablefunc->ordinalitycol;
-    oldcxt = MemoryContextSwitchTo(tstate->perValueCxt);
+
+    /*
+     * We need a short-lived memory context that we can clean up each time
+     * around the loop, to avoid wasting space. Our default per-tuple context
+     * is fine for the job, since we won't have used it for anything yet in
+     * this tuple cycle.
+     */
+    oldcxt = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
 
     /*
      * Keep requesting rows from the table builder until there aren't any.
@@ -496,7 +516,7 @@ tfuncLoadRows(TableFuncScanState *tstate, ExprContext *econtext)
 
         tuplestore_putvalues(tstate->tupstore, tupdesc, values, nulls);
 
-        MemoryContextReset(tstate->perValueCxt);
+        MemoryContextReset(econtext->ecxt_per_tuple_memory);
     }
 
     MemoryContextSwitchTo(oldcxt);