Horton

Re: Should HashSetOp go away

David Rowley <dgrowleyml@gmail.com>

View thread
From: David Rowley <dgrowleyml@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Jeff Janes <jeff.janes@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2025-11-02T10:03:42Z
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.

On Sat, 1 Nov 2025 at 08:22, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > Here's a pair of patches to try to do better.  The first one
> > is concerned with getting more realistic size estimates for
> > TupleHashTables in the planner.  The second is some mop-up
> > that's been pending for a long time in the same area, namely
> > getting rid of "long int" field types in Plan nodes.

I had a look at the v2 patches. Mostly quibbles, but #4 seems like an oversight.

0001:

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), but in reality it should come out the same
since SizeofMinimalTupleHeader is 16. If that were to change then I
believe the extra MAXALIGN would start overestimating the memory.

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

I think "above" is ok when it's the same function, but when it's
talking about another function, it's a recipe for becoming outdated.
It'd be better using the function name so we can grep for that when do
refactor work, else we end up with commits like e3a0304eb...

3) Quite a collection of naming styles here.

+Size
+EstimateTupleHashTableSpace(double nentries,
+                            Size tupleWidth,
+                            Size additionalsize)
+{
+    Size        sh_space;
+    double        tuples_space;

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.

0002:

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

David