On Thu, 30 Oct 2025 at 13:22, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > Is it worth mentioning there that we leave it up to the destruction of
> > the hash table itself to when the ExecutorState context is reset?
> > (Apparently execGrouping.c does not expose any way to do
> > tuplehash_destroy() anyway)
>
> Yeah. I think this is better documented in execGrouping.c. I propose
> adding this:
>
> @@ -169,6 +169,13 @@ execTuplesHashPrepare(int numCols,
> * reasonably often, typically once per tuple. (We do it that way, rather
> * than managing an extra context within the hashtable, because in many cases
> * the caller can specify a tempcxt that it needs to reset per-tuple anyway.)
> + *
> + * We don't currently provide DestroyTupleHashTable functionality; the hash
> + * table will be cleaned up at destruction of the metacxt. (Some callers
> + * bother to delete the tuplescxt explicitly, though it'd be sufficient to
> + * ensure it's a child of the metacxt.) There's not much point in working
> + * harder than this so long as the expression-evaluation infrastructure
> + * behaves similarly.
> */
> TupleHashTable
> BuildTupleHashTable(PlanState *parent,
Looks good.
> > 2) I'm not sure if it makes it neater or not, but did you consider
> > moving the creation of the setopstate->tuplesContext context into
> > build_hash_table()? The motivation there is that it keeps the
> > hash-related initialisations together.
>
> Hmm, could do that, but all of these callers currently follow the
> pattern of creating these contexts in the plan nodes' Init functions
> and destroying them in the End functions. I don't see a big advantage
> in changing that.
All good. I'm fine either way.
> > 3) Not for this patch, but wanted to note the observation: I see we
> > always create the hash table during plan startup in nodeSetOp.c. I
> > suspect this could cause the same issue for Setops as I fixed in
> > Memoize in 57f59396b, which fixed a slow EXPLAIN in [1].
>
> Fair point. I'd be inclined to just skip the hashtable creation
> if (eflags & EXEC_FLAG_EXPLAIN_ONLY), like nodeAgg.c did.
>
> I wonder though if skipping the initialization of the hashtable
> execution expressions has any visible impact on what EXPLAIN can
> print. I have a vague recollection that there were some places
> where we backed off similar optimizations because of concerns
> like that.
I recall some discussion around JIT and EXPLAIN (ANALYZE OFF). I seem
to have a branch here that disables it completely, but I recall Andres
or you rejecting the idea. I can't seem to find the thread. I agree
would be quite strange if the JIT functions count was to differ
between EXPLAIN and EXPLAIN ANALYZE, but if we have to contort our
code too much to make that work, then I do question the usefulness of
showing the JIT details in EXPLAIN (ANALYZE OFF). Anyway, maybe off
topic for this thread.
I'm fine with the patch, assuming it now includes the new
BuildTupleHashTable() comment and adjusted
ExecEndRecursiveUnion/ExecEndSetOp comments.
David
