Horton

Re: Should HashSetOp go away

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

View thread
From: Tom Lane <tgl@sss.pgh.pa.us>
To: David Rowley <dgrowleyml@gmail.com>
Cc: Jeff Janes <jeff.janes@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2025-11-02T16:26:06Z
Lists: pgsql-hackers

Commits

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

  1. Change "long" numGroups fields to be Cardinality (i.e., double).

  2. Improve planner's estimates of tuple hash table sizes.

  3. Use BumpContext contexts in TupleHashTables, and do some code cleanup.

  4. Convert SetOp to read its inputs as outerPlan and innerPlan.

  5. Use more efficient hashtable for execGrouping.c to speed up hash aggregation.

Attachments

David Rowley <dgrowleyml@gmail.com> writes:
> I had a look at the v2 patches. Mostly quibbles, but #4 seems like an oversight.

Thanks for reviewing!

> 1) For the following:
> +    tuples_space = nentries * (MAXALIGN(SizeofMinimalTupleHeader) +
> +                               MAXALIGN(tupleWidth) +
> +                               MAXALIGN(additionalsize));
> If I'm not mistaken, technically that should be
> MAXALIGN(SizeofMinimalTupleHeader + tupleWidth) +
> MAXALIGN(additionalsize),

No, I think it's correct as written: the data payload of a tuple
must always start on a MAXALIGN boundary.  As you say, it doesn't
matter as long as SizeofMinimalTupleHeader is 16, but I think this
way is formally correct.  (It would matter more if we were trying
to account for tuples' null bitmaps ...)

> 2) Would it be better to reference the function name
> "buildSubPlanHash" instead of "above" in:
> + * Adjust the rowcount estimate in the same way as above, except that we

Done.

> 3) Quite a collection of naming styles here.

Yeah :-( ... we work in an old and none-too-consistent code base.
Do you have any specific suggestions about which of these functions
might fit its surroundings better with a different caps style?

> 4) I think this is missing a "/ SH_FILLFACTOR"
> + /* should be safe to convert to uint64 */
> + size = (uint64) nentries;
> i.e do what SH_CREATE does.

Oh!  I think I got confused because some of that logic is in
SH_CREATE and some in SH_COMPUTE_SIZE :-(.  Good catch.

> 5) Is it switching "Max(nbuckets, 1);" to "nbuckets" in
> hash_choose_num_buckets(). Looks like BuildTupleHashTable() will do
> that part for us.

Yeah, we could do that.  I was trying to not touch more of nodeAgg
than I had to, but it seems sensible to not duplicate something
that BuildTupleHashTable will do.

v3 attached.

			regards, tom lane