On Fri, 2020-03-13 at 00:34 +0000, Andrew Gierth wrote:
> > > > > > "Justin" == Justin Pryzby <pryzby@telsasoft.com> writes:
>
> > On Thu, Mar 12, 2020 at 12:16:26PM -0700, Andres Freund wrote:
> >> Indeed, that's incorrect. Causes the number of buckets for the
> >> hashtable to be set higher - the size is just used for that. I'm
> a
It's also used to set the 'entrysize' field of the TupleHashTable,
which doesn't appear to be used for anything? Maybe we should just
remove that field... it confused me for a moment as I was looking into
this.
> >> bit wary of changing this in the stable branches - could cause
> >> performance changes?
>
> I think (offhand, not tested) that the number of buckets would only
> be
> affected if the (planner-supplied) numGroups value would cause
> work_mem
> to be exceeded; the planner doesn't plan a hashagg at all in that
>
Now that we have Disk-based HashAgg, which already tries to choose the
number of buckets with work_mem in mind; and no other caller specifies
non-zero additionalsize, why not just get rid of that argument
completely? It can still sanity check against work_mem for the sake of
other callers. But it doesn't need 'additionalsize' to do so.
Or, we can keep the 'additionalsize' argument but put it to work store
the AggStatePerGroupData inline in the hash table. That would allow us
to remove the 'additional' pointer from TupleHashEntryData, saving 8
bytes plus the chunk header for every group. That sounds very tempting.
If we want to get even more clever, we could try to squish
AggStatePerGroupData into 8 bytes by putting the flags
(transValueIsNull and noTransValue) into unused bits of the Datum. That
would work if the transtype is by-ref (low bits if pointer will be
unused), or if the type's size is less than 8, or if the particular
aggregate doesn't need either of those booleans. It would get messy,
but saving 8 bytes per group is non-trivial.
Regards,
Jeff Davis