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

Tom Lane writes:

If we do keep a separate context, I agree with the idea of one
MemoryContextReset in the exit of ExecHashSubPlan, but the proposed
patch seems like a mess. I think what we ought to do is refactor
ExecHashSubPlan so that there's exactly one "ExecClearTuple(slot)”
down at the bottom, and then we can add a MemoryContextReset after it.

The proposed patch was inspired by the approach used in ExecRecursiveUnion.

Refactoring ExecHashSubPlan would be a better long‑term solution.

It's good if we have an example that one can watch to confirm
it-leaks-or-not by monitoring the process's memory consumption,
but I don't foresee committing it.

Should we omit the test case, or add one in the same form as in the patch?

—

Thanks,

Haying Li

李海洋
阿里巴巴及蚂蚁集团
邮箱：mohen(dot)lhy(at)alibaba-inc(dot)com
地址：浙江-杭州-云谷园区 1-3C-577

阿里巴巴及蚂蚁集团 企业主页
信息安全声明：本邮件包含信息归发件人所在组织所有，发件人所在组织对该邮件拥有所有权利。
请接收者注意保密，未经发件人书面许可，不得向任何第三方组织和个人透露本邮件所含信息的全部或部分。以上声明仅适用于工作邮件。
Information Security Notice: The information contained in this mail is solely property of the sender's organization.
This mail communication is confidential. Recipients named above are obligated to maintain secrecy and are not permitted to disclose the contents of this communication to others. ------------------------------------------------------------------
发件人：Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us>
日　期：2025年09月03日 23:35:26
收件人：feichanghong<feichanghong(at)qq(dot)com>
抄　送：ocean_li_996<ocean_li_996(at)163(dot)com>; 李海洋(陌痕)<mohen(dot)lhy(at)alibaba-inc(dot)com>; pgsql-bugs(at)lists(dot)postgresql(dot)org<pgsql-bugs(at)lists(dot)postgresql(dot)org>; <jdavis(at)postgresql(dot)org>
主　题：Re: BUG #19040: Memory leak in hashed subplan node due to missing hashtempcxt reset

feichanghong <feichanghong(at)qq(dot)com> writes:
> It seems this issue has been around for many years. I took a quick
> look at the patch for fixing it. Why don't we reset the temp context
> in the LookupTupleHashEntry, TupleHashTableHash, LookupTupleHashEntryHash,
> and FindTupleHashEntry functions? This seems more robust.

No, I disagree with that. MemoryContextReset is not free. The
existing code seems to expect that the hash tempcxt will be a
per-tuple context or similar, which will be reset once per executor
cycle by existing mechanisms. I wonder why ExecInitSubPlan is
making a separate context for this at all, rather than using some
surrounding short-lived context.

If we do keep a separate context, I agree with the idea of one
MemoryContextReset in the exit of ExecHashSubPlan, but the proposed
patch seems like a mess. I think what we ought to do is refactor
ExecHashSubPlan so that there's exactly one "ExecClearTuple(slot)"
down at the bottom, and then we can add a MemoryContextReset after it.

> Furthermore,
> the added test case doesn't seem to detect whether there's a memory leak.

Yeah, test cases for memory leaks are problematic. The only way they
can really "detect" one is if they run so long as to be pretty much
guaranteed to hit OOM, which is (a) impossible to quantify across
a range of platforms and (b) not something we'd care to commit anyway.

It's good if we have an example that one can watch to confirm
it-leaks-or-not by monitoring the process's memory consumption,
but I don't foresee committing it.

regards, tom lane