From: Tom Lane <tgl@sss.pgh.pa.us>
To: "=?UTF-8?B?5p2O5rW35rSLKOmZjOeXlSk=?=" <mohen.lhy@alibaba-inc.com>
cc: "feichanghong" <feichanghong@qq.com>,
        "ocean_li_996" <ocean_li_996@163.com>,
        "pgsql-bugs@lists.postgresql.org" <pgsql-bugs@lists.postgresql.org>
Subject: =?UTF-8?Q?Re:_=E5=9B=9E=E5=A4=8D=EF=BC=9ABUG_#19040:_Memory_leak_?=
         =?UTF-8?Q?in_hashed_subplan_node_due_to_missing_hashtempcxt_reset?=
In-reply-to: <68ba6c1d-40a8-4aa5-aec1-f0421455bcf5.mohen.lhy@alibaba-inc.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>
 <507583.1757352192@sss.pgh.pa.us> <721997.1757364370@sss.pgh.pa.us>
 <tencent_8ED17B52864FA7C5479B890C071A8281D206@qq.com>,<1073615.1757387109@sss.pgh.pa.us>
 <68ba6c1d-40a8-4aa5-aec1-f0421455bcf5.mohen.lhy@alibaba-inc.com>
Comments: In-reply-to "=?UTF-8?B?5p2O5rW35rSLKOmZjOeXlSk=?="
 <mohen.lhy@alibaba-inc.com>
	message dated "Tue, 09 Sep 2025 15:30:48 +0800"
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-ID: <1239240.1757456662.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Tue, 09 Sep 2025 18:24:22 -0400
Message-ID: <1239241.1757456662@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1239241.1757456662%40sss.pgh.pa.us>
Precedence: bulk

"=3D?UTF-8?B?5p2O5rW35rSLKOmZjOeXlSk=3D?=3D" <mohen.lhy@alibaba-inc.com> w=
rites:
> For v04-0002, I checked the commit history, and before commit 133924e1,
> buildSubPlanHash was using ecxt_per_tuple_memory. It seems that the
> "Subplan HashTable Temp Context" was introduced in order to fix a certai=
n bug.

You're quite right that 0002 looks suspiciously like it's reverting
133924e1.  However, it doesn't cause the test case added by that
commit to fail (even under Valgrind).  I think the reason is what
you say next:

> However, the changed behavior of TupleHashTableMatch introduced by commi=
t
> bf6c614a (noted in [1]) may make the condition=EF=BC=9A
> ```
> However, the hashtable routines feel free to reset their temp context at=
 any time, =

> which'd lead to destroying input data that was still needed.
> ```
> no longer holds true. Then,  the lifespan of tempcxt in buildHashtable i=
s similar
> to that of innercontext->ecxt_per_tuple_memory, so it makes sense to mer=
ge the two,
> I think.

Looking around in 133924e1^, the only place in execGrouping.c that
would reset the hashtable's tempcxt was execTuplesMatch (which was
called by TupleHashTableMatch).  But bf6c614a removed execTuplesMatch.
The modern execGrouping.c code resets hashtable->tempcxt nowhere.
Instead, TupleHashTableMatch applies ExecQualAndReset to
hashtable->exprcontext, so that what is reset is the
ecxt_per_tuple_memory of that econtext --- but that's manufactured by
CreateStandaloneExprContext in BuildTupleHashTable, and has no
connection at all to any context that nodeSubplan.c will touch.
It is certainly not the same ecxt_per_tuple_memory that pre-133924e1
was resetting.

So basically I'm saying that bf6c614a made it okay to revert
133924e1.

> BTW, I ran the test case supported in commit 133924e1 on version not con=
tained commit
> 133924e1 (tag REL8_0_26). I did not find any problems. But i can not fin=
d more information
> about this issue. =


Digging in the archives, I found the discussion leading to 133924e1 at
https://www.postgresql.org/message-id/flat/i2jnbo%241lcj%241%40news.hub.or=
g

As for trying it on 8.0.26, the commit message for 133924e1 says
specifically that the problem isn't there pre-8.1.  I did try to
duplicate your test using 133924e1^, and it didn't fail for me either.
But I'm not too excited about that, because building PG versions this
old on modern platforms is really hard.  I had to compile with -O0
to get a build that worked at all, and that's already a significant
difference from the code we would have been testing back in 2010.
It may be that the reason for non-reproduction is buried somewhere in
that, or in the different compiler toolchain.  I'm not sure it's worth
a lot of effort to dig deeply into the details.

			regards, tom lane