public inbox for [email protected]  

help / color / mirror / Atom feed

From: Tom Lane <[email protected]>
To: Tomas Vondra <[email protected]>
Cc: Laurenz Albe <[email protected]>
Cc: [email protected]
Subject: Re: Memory leak in incremental sort re-scan
Date: Thu, 15 Jun 2023 16:11:23 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>

Tomas Vondra <[email protected]> writes:
> On 6/15/23 13:48, Laurenz Albe wrote:
>> ExecIncrementalSort() calls tuplesort_begin_common(), which creates the "TupleSort main"
>> and "TupleSort sort" memory contexts, and ExecEndIncrementalSort() calls tuplesort_end(),
>> which destroys them.
>> But ExecReScanIncrementalSort() only resets the memory contexts.

> I think it's correct, but I need to look at the code more closely - it's
> been a while. The code is a bit silly, as it resets the tuplesort and
> then throws away all the pointers - so what could the _end() break?

The report at [1] seems to be the same issue of ExecReScanIncrementalSort
leaking memory.  I applied Laurenz's fix, and that greatly reduces the
speed of leak but doesn't remove the problem entirely.  It looks like
the remaining issue is that the data computed by preparePresortedCols() is
recomputed each time we rescan the node.  This seems entirely gratuitous,
because there's nothing in that that could change across rescans.
I see zero leakage in that example after applying the attached quick
hack.  (It might be better to make the check in the caller, or to just
move the call to ExecInitIncrementalSort.)

			regards, tom lane

[1] https://www.postgresql.org/message-id/db03c582-086d-e7cd-d4a1-3bc722f81765%40inf.ethz.ch



Attachments:

  [text/x-diff] stop-incremental-sort-rescan-leaks.patch (1.2K, ../[email protected]/2-stop-incremental-sort-rescan-leaks.patch)
  download | inline diff:
diff --git a/src/backend/executor/nodeIncrementalSort.c b/src/backend/executor/nodeIncrementalSort.c
index 34257ce34b..655b1c30e1 100644
--- a/src/backend/executor/nodeIncrementalSort.c
+++ b/src/backend/executor/nodeIncrementalSort.c
@@ -166,6 +166,9 @@ preparePresortedCols(IncrementalSortState *node)
 {
 	IncrementalSort *plannode = castNode(IncrementalSort, node->ss.ps.plan);
 
+	if (node->presorted_keys)
+		return;
+
 	node->presorted_keys =
 		(PresortedKeyData *) palloc(plannode->nPresortedCols *
 									sizeof(PresortedKeyData));
@@ -1140,7 +1143,6 @@ ExecReScanIncrementalSort(IncrementalSortState *node)
 	node->outerNodeDone = false;
 	node->n_fullsort_remaining = 0;
 	node->bound_Done = 0;
-	node->presorted_keys = NULL;
 
 	node->execution_status = INCSORT_LOADFULLSORT;
 
@@ -1154,12 +1156,12 @@ ExecReScanIncrementalSort(IncrementalSortState *node)
 	 */
 	if (node->fullsort_state != NULL)
 	{
-		tuplesort_reset(node->fullsort_state);
+		tuplesort_end(node->fullsort_state);
 		node->fullsort_state = NULL;
 	}
 	if (node->prefixsort_state != NULL)
 	{
-		tuplesort_reset(node->prefixsort_state);
+		tuplesort_end(node->prefixsort_state);
 		node->prefixsort_state = NULL;
 	}
 

view thread (4+ messages)  latest in thread

reply

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: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Memory leak in incremental sort re-scan
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox