From: Andrew Gierth <andrew@tao11.riddles.org.uk>
To: Pavel Stehule <pavel.stehule@gmail.com>
Cc: pgsql-bugs@lists.postgresql.org,
  Alvaro Herrera <alvherre@2ndquadrant.com>
Subject: Re: BUG #15321: XMLTABLE leaks memory like crazy
In-Reply-To: 
 <CAFj8pRDr74=wxRD68EGMS6Z90rTn4JOp6g9s_3BE37gtkJAX6A@mail.gmail.com>
	(Pavel Stehule's message of "Sun, 12 Aug 2018 12:39:44 +0200")
Message-ID: <877ekv7s9j.fsf@news-spur.riddles.org.uk>
References: <153394403528.10284.7530399040974170549@wrigleys.postgresql.org>
	<87va8h7nyl.fsf@news-spur.riddles.org.uk>
	<87r2j57hdo.fsf@news-spur.riddles.org.uk>
	<CAFj8pRAm9cRuTf5STnX=0fGLpxhu7CR6GAj8LGq+iuw+NKXfPg@mail.gmail.com>
	<CAFj8pRBMpOn7X-N2PZKFtMK2UNjTahoArGmtMo1C7Az0x9Rzeg@mail.gmail.com>
	<87k1ow6o7i.fsf@news-spur.riddles.org.uk>
	<CAFj8pRD7bkGhn47wTw-CPhAQgH8OikezB=JrzVRsP_RAWH-scA@mail.gmail.com>
	<87ftzk6jw2.fsf@news-spur.riddles.org.uk>
	<CAFj8pRDWORKuuLEvFN-tMp06nuD5g_+yPAWWVW7umanoriEwwg@mail.gmail.com>
	<87bma77v67.fsf@news-spur.riddles.org.uk>
	<CAFj8pRDr74=wxRD68EGMS6Z90rTn4JOp6g9s_3BE37gtkJAX6A@mail.gmail.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (berkeley-unix)
Date: Sun, 12 Aug 2018 12:18:07 +0100
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Precedence: bulk

--=-=-=
Content-Type: text/plain

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


--=-=-=
Content-Type: text/x-patch
Content-Disposition: inline; filename=xmlmem.patch
Content-Description: xmltable memory leak patch v2

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);

--=-=-=--