Hi,
On 2019-01-14 10:04:23 -0800, Andres Freund wrote:
> On 2019-01-14 11:57:55 -0500, Tom Lane wrote:
> > The short answer here is that this addition to BuildTupleHashTable:
> >
> > hashtable->exprcontext = CreateExprContext(parent->state);
> >
> > allocates memory that is not freed by freeing the hashtable's tablecxt,
> > breaking the API for grouping hashtables.
> >
> > Why does a hashtable need its own exprcontext now when it didn't before?
>
> Because the comparison is now done via ExprState machinery than manual
> fmgr invocations. I'd discussed a patch solving this a few weeks ago
> with Andrew Gierth, but got stuck with the fact that essentially all
> fixes entail either an API or an ABI break - but I think we gotta do
> that anyway.
>
> For performance reasons the API really should be changed so we don't
> allocate/deallocate the entire hashtable on each round - I've prototyped
> a fix that only resets it and there's noticable performance gains. But
> that's not something we can easily do in the back branches.
>
> Let me rebase and cleanup my patches, it's probably easier to discuss
> with them in front of us.
Attached is a series of patches that fix this, and the issue in [1],
namely that we do not reuse JITed expressions, leading to significant
overhead when JIT compiling.
The first patch just moves the ExprContext into the tablecxt of the
tuple hash table, and uses a standalone expression instead of linked
into estate. I think that's OK for the the table's purpose, because the
expression used is tightly restricted because it's built with
ExecBuildGroupingEqual(). Previously we'd call fmgr functions directly,
so there can't be any dependency on expression parents here (and it's
not clear how that'd ever make any sense). Given that, I think it's ok
to not explicitly shutdown the expr context. If somebody disagrees, we
could change this, but freeing individual ExprContexts is
O(#exprcontexts), so I'd prefer not to unnecessarily go there.
This is sufficient to fix the memory leak problem (but is slower than
10, due to the overhead of creating the comparator expression
repeatedly).
The remaining patches add 0002) support for resetting a simplehash
hashtable, 0003) Add BuildTupleHashTableExt(), which allows to place the
tuple hashtable into a separate memory context besides tablecxt, 0004)
changes in-core tuple hash users to reset instead recreate hashtables.
This has the advantage of being doable without breaking external users
of the API, while still avoiding recreation of the comparator expression
(and the slot, and the TupleHashTable itself), which in turn avoids the
JIT overhead.
These patches, especially surrounding comments, need a bit of polish,
but I thought it'd be good to discuss them before investing more time.
Comments?
Greetings,
Andres Freund
[1] https://www.postgresql.org/message-id/15486-05850f065da42931%40postgresql.org
