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
Date: Sun, 12 Aug 2018 12:18:07 +0100
Message-ID: <877ekv7s9j.fsf@news-spur.riddles.org.uk> (raw)
In-Reply-To: <CAFj8pRDr74=wxRD68EGMS6Z90rTn4JOp6g9s_3BE37gtkJAX6A@mail.gmail.com>
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>
>>>>> "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)
Attachments:
[text/x-patch] xmlmem.patch (2.4K, ../877ekv7s9j.fsf@news-spur.riddles.org.uk/2-xmlmem.patch)
download | inline diff:diff --git a/src/backend/executor/nodeTableFuncscan.c b/src/backend/executor/nodeTableFuncscan.cindex 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);
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: andrew@tao11.riddles.org.uk, pavel.stehule@gmail.com, alvherre@2ndquadrant.com
Subject: Re: BUG #15321: XMLTABLE leaks memory like crazy
In-Reply-To: <877ekv7s9j.fsf@news-spur.riddles.org.uk>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox