Re: Memory leak in incremental sort re-scan

Tomas Vondra <tomas.vondra@enterprisedb.com> 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

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