Message-ID: <b2bd02dff61af15e3526293e2771f874cf2a3be7.camel@cybertec.at>
Subject: Memory leak in incremental sort re-scan
From: Laurenz Albe <laurenz.albe@cybertec.at>
To: pgsql-hackers@lists.postgresql.org
Date: Thu, 15 Jun 2023 13:48:44 +0200
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Archived-At: <https://www.postgresql.org/message-id/b2bd02dff61af15e3526293e2771f874cf2a3be7.camel%40cybertec.at>
Precedence: bulk

ExecIncrementalSort() calls tuplesort_begin_common(), which creates the "Tu=
pleSort main"
and "TupleSort sort" memory contexts, and ExecEndIncrementalSort() calls tu=
plesort_end(),
which destroys them.
But ExecReScanIncrementalSort() only resets the memory contexts.  Since the=
 next call to
ExecIncrementalSort() will create them again, we end up leaking these conte=
xts for every
re-scan.

Here is a reproducer with the regression test database:

  SET enable_sort =3D off;
  SET enable_hashjoin =3D off;
  SET enable_mergejoin =3D off;
  SET enable_material =3D off;

  SELECT t.unique2, t2.r
  FROM tenk1 AS t=20
     JOIN (SELECT unique1,=20
                  row_number() OVER (ORDER BY hundred, thousand) AS r=20
           FROM tenk1=20
           OFFSET 0) AS t2=20
        ON t.unique1 + 0 =3D t2.unique1
  WHERE t.unique1 < 1000;

The execution plan:

 Nested Loop
   Join Filter: ((t.unique1 + 0) =3D tenk1.unique1)
   ->  Bitmap Heap Scan on tenk1 t
         Recheck Cond: (unique1 < 1000)
         ->  Bitmap Index Scan on tenk1_unique1
               Index Cond: (unique1 < 1000)
   ->  WindowAgg
         ->  Incremental Sort
               Sort Key: tenk1.hundred, tenk1.thousand
               Presorted Key: tenk1.hundred
               ->  Index Scan using tenk1_hundred on tenk1


A memory context dump at the end of the execution looks like this:

      ExecutorState: 262144 total in 6 blocks; 74136 free (29 chunks); 1880=
08 used
        TupleSort main: 32832 total in 2 blocks; 7320 free (0 chunks); 2551=
2 used
          TupleSort sort: 8192 total in 1 blocks; 7928 free (0 chunks); 264=
 used
            Caller tuples: 8192 total in 1 blocks (0 chunks); 7984 free (0 =
chunks); 208 used
        TupleSort main: 32832 total in 2 blocks; 7256 free (0 chunks); 2557=
6 used
          TupleSort sort: 8192 total in 1 blocks; 7928 free (0 chunks); 264=
 used
            Caller tuples: 8192 total in 1 blocks (0 chunks); 7984 free (0 =
chunks); 208 used
        TupleSort main: 32832 total in 2 blocks; 7320 free (0 chunks); 2551=
2 used
          TupleSort sort: 8192 total in 1 blocks; 7928 free (0 chunks); 264=
 used
            Caller tuples: 8192 total in 1 blocks (0 chunks); 7984 free (0 =
chunks); 208 used
      [many more]
        1903 more child contexts containing 93452928 total in 7597 blocks; =
44073240 free (0 chunks); 49379688 used


The following patch fixes the problem for me:

--- a/src/backend/executor/nodeIncrementalSort.c
+++ b/src/backend/executor/nodeIncrementalSort.c
@@ -1145,21 +1145,16 @@ ExecReScanIncrementalSort(IncrementalSortState *nod=
e)
    node->execution_status =3D INCSORT_LOADFULLSORT;
=20
    /*
-    * If we've set up either of the sort states yet, we need to reset them=
.
-    * We could end them and null out the pointers, but there's no reason t=
o
-    * repay the setup cost, and because ExecIncrementalSort guards presort=
ed
-    * column functions by checking to see if the full sort state has been
-    * initialized yet, setting the sort states to null here might actually
-    * cause a leak.
+    * Release tuplesort resources.
     */
    if (node->fullsort_state !=3D NULL)
    {
-       tuplesort_reset(node->fullsort_state);
+       tuplesort_end(node->fullsort_state);
        node->fullsort_state =3D NULL;
    }
    if (node->prefixsort_state !=3D NULL)
    {
-       tuplesort_reset(node->prefixsort_state);
+       tuplesort_end(node->prefixsort_state);
        node->prefixsort_state =3D NULL;
    }
=20

The original comment hints that this might mot be the correct thing to do..=
.

Yours,
Laurenz Albe