public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tomas Vondra <[email protected]>
To: Laurenz Albe <[email protected]>
To: [email protected]
Subject: Re: Memory leak in incremental sort re-scan
Date: Thu, 15 Jun 2023 15:19:47 +0200
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
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
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]
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