Message-ID: <0643dbc2-53a6-37ac-b567-243a489cef0f@enterprisedb.com>
Date: Thu, 15 Jun 2023 22:30:58 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: Memory leak in incremental sort re-scan
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Laurenz Albe <laurenz.albe@cybertec.at>,
 pgsql-hackers@lists.postgresql.org
References: <b2bd02dff61af15e3526293e2771f874cf2a3be7.camel@cybertec.at>
 <99e584a1-2f55-a56d-efb1-d38c57eb2beb@enterprisedb.com>
 <2267955.1686859883@sss.pgh.pa.us>
Content-Language: en-US
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: <2267955.1686859883@sss.pgh.pa.us>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/0643dbc2-53a6-37ac-b567-243a489cef0f%40enterprisedb.com>
Precedence: bulk


On 6/15/23 22:11, Tom Lane wrote:
> 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.

Funny how these reports often come in pairs ...

> 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.

Yeah, I was wondering about that too when I skimmed over that code
earlier today.

> 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.)
> 

Thanks for looking. Are you planning to work on this and push the fix,
or do you want me to finish this up?


regards

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