Re: Memory leak in incremental sort re-scan

Hi,

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. Since the next call to
> ExecIncrementalSort() will create them again, we end up leaking these contexts for every
> re-scan.
>
> Here is a reproducer with the regression test database:
>
> SET enable_sort = off;
> SET enable_hashjoin = off;
> SET enable_mergejoin = off;
> SET enable_material = off;
>
> SELECT t.unique2, t2.r
> FROM tenk1 AS t
> JOIN (SELECT unique1,
> row_number() OVER (ORDER BY hundred, thousand) AS r
> FROM tenk1
> OFFSET 0) AS t2
> ON t.unique1 + 0 = t2.unique1
> WHERE t.unique1 < 1000;
>
> The execution plan:
>
> Nested Loop
> Join Filter: ((t.unique1 + 0) = 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); 188008 used
> TupleSort main: 32832 total in 2 blocks; 7320 free (0 chunks); 25512 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); 25576 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); 25512 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 *node)
> node->execution_status = INCSORT_LOADFULLSORT;
>
> /*
> - * 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 to
> - * repay the setup cost, and because ExecIncrementalSort guards presorted
> - * 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 != 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;
> }
>
>
> The original comment hints that this might mot be the correct thing to do...
>

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?

AFAICS the comment says that we can't just do tuplesort_reset and keep
the pointers, because some other code depends on them being NULL.

In hindsight, that's a bit awkward - it'd probably be better to have a
separate flag, which would allow us to just reset the tuplesort.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company