From: Tom Lane <tgl@sss.pgh.pa.us>
To: feichanghong <feichanghong@qq.com>
Cc: "李海洋(陌痕)" <mohen.lhy@alibaba-inc.com>
Cc: ocean_li_996 <ocean_li_996@163.com>
Cc: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
Subject: Re: BUG #19040: Memory leak in hashed subplan node due to missing hashtempcxt reset
Date: Mon, 08 Sep 2025 13:23:12 -0400
Message-ID: <507583.1757352192@sss.pgh.pa.us> (raw)
In-Reply-To: <tencent_0C8392F1FB41065427D6C7AC8919073F8206@qq.com>
References: <19040-c9b6073ef814f48c@postgresql.org>
<58062871.112.1990d3e06ea.Coremail.ocean_li_996@163.com>
<tencent_8E54DE49323C3A293EE44DFE385011189509@qq.com>
<808767.1756913726@sss.pgh.pa.us>
<c765ad39-39eb-4a39-8e67-91bcb9ee65e0.mohen.lhy@alibaba-inc.com>
<160523.1757190713@sss.pgh.pa.us>
<bfb8be51-18e5-4839-a555-5c3d0ccd32d0.mohen.lhy@alibaba-inc.com>
<tencent_0C8392F1FB41065427D6C7AC8919073F8206@qq.com>
feichanghong <feichanghong@qq.com> writes:
>> On Sep 7, 2025, at 16:24, 李海洋(陌痕) <mohen.lhy@alibaba-inc.com> wrote:
>> On 2025-09-06 20:31:53 Tom Lane <tgl@sss.pgh.pa.us> writes:
>>> After contemplating things for awhile, I think that feichanghong’s
>>> idea is the right one after all: in each of the functions that switch
>>> into hashtable->tempcxt, let's do a reset on the way out, as attached.
>> I have considered this approach as well, but my concern is that "tempcxt"
>> is not always an independent memory context. In some cases, it references
>> another context — for example, in nodeSetOp.c’s "build_hash_table", “tempcxt"
>> points to "setopstate->ps.ps_ExprContext->ecxt_per_tuple_memory". There is
>> similar usage in nodeAgg.c as well. I’m not entirely sure that this approach would
>> not discard data we still need, because the lifespan of
>> "ps_ExprContext->ecxt_per_tuple_memory" seems to be longer than “tempcxt”.
> Yes, I agree with that.
Yeah, that is a fair point. The existing API is that the caller is
responsible for resetting tempcxt sufficiently often, and it looks
like nodeSubplan.c is the only place that gets this wrong. Let's
just fix nodeSubplan.c, add a comment documenting this requirement,
and call it good.
>> Should we make tempcxt a completely independent memory context?
> It looks fine. Perhaps we don't need to pass tempcxt to BuildTupleHashTable,
> but rather create a new one within it. After each switch to tempcxt, we should
> clean it up using MemoryContextReset.
I thought about that too, but that would result in two short-lived
contexts and two reset operations per tuple cycle where only one
is needed. I'm rather tempted to fix nodeSubplan.c by making it
use innerecontext->ecxt_per_tuple_memory instead of a separate
hash tmpcontext. That context is getting reset already, at least in
buildSubPlanHash(). That's probably too risky for the back branches
but we could do it in HEAD.
regards, tom lane
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: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, feichanghong@qq.com, mohen.lhy@alibaba-inc.com, ocean_li_996@163.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #19040: Memory leak in hashed subplan node due to missing hashtempcxt reset
In-Reply-To: <507583.1757352192@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox