From: Tom Lane <tgl@sss.pgh.pa.us>
To: feichanghong <feichanghong@qq.com>
cc: =?utf-8?B?Iuadjua1t+a0iyjpmYznl5UpIg==?= <mohen.lhy@alibaba-inc.com>,
        ocean_li_996 <ocean_li_996@163.com>,
        "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
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>
Comments: In-reply-to feichanghong <feichanghong@qq.com>
	message dated "Sun, 07 Sep 2025 17:02:23 +0800"
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-ID: <507582.1757352192.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Mon, 08 Sep 2025 13:23:12 -0400
Message-ID: <507583.1757352192@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/507583.1757352192%40sss.pgh.pa.us>
Precedence: bulk

feichanghong <feichanghong@qq.com> writes:
>> On Sep 7, 2025, at 16:24, =E6=9D=8E=E6=B5=B7=E6=B4=8B(=E9=99=8C=E7=97=95=
) <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=E2=80=
=99s
>>> 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 "tempcx=
t"
>> is not always an independent memory context. In some cases, it referenc=
es
>> another context =E2=80=94 for example, in nodeSetOp.c=E2=80=99s "build_=
hash_table", =E2=80=9Ctempcxt"
>> points to "setopstate->ps.ps_ExprContext->ecxt_per_tuple_memory". There=
 is
>> similar usage in nodeAgg.c as well. I=E2=80=99m 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 =E2=80=9C=
tempcxt=E2=80=9D.

> 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 BuildTupleHashTa=
ble,
> 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