Horton

Re: BUG #19040: Memory leak in hashed subplan node due to missing hashtempcxt reset

Tom Lane <tgl@sss.pgh.pa.us>

View thread
From: Tom Lane <tgl@sss.pgh.pa.us>
To: feichanghong <feichanghong@qq.com>
Cc: "李海洋(陌痕)" <mohen.lhy@alibaba-inc.com>, ocean_li_996 <ocean_li_996@163.com>, "pgsql-bugs@lists.postgresql.org" <pgsql-bugs@lists.postgresql.org>
Date: 2025-09-08T17:23:12Z
Lists: pgsql-bugs

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Eliminate duplicative hashtempcxt in nodeSubplan.c.

  2. Fix memory leakage in nodeSubplan.c.

  3. Do execGrouping.c via expression eval machinery, take two.

  4. Fix potential failure when hashing the output of a subplan that produces

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