feichanghong <feichanghong@qq.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
