Re: Should HashSetOp go away

On Sat, 1 Nov 2025 at 08:22, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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