David Rowley <dgrowle...@gmail.com> writes:
> On Fri, 20 Dec 2024 at 11:23, Tom Lane <t...@sss.pgh.pa.us> wrote:
>> Attached is a patch to take it out and then rename
>> BuildTupleHashTableExt() back to BuildTupleHashTable().
> No complaints here. Thanks for cleaning that up.
Thanks, will push.
> I couldn't help but also notice the nbuckets parameter is using the
> "long" datatype. The code in BuildTupleHashTable seems to think it's
> fine to pass the long as the uint32 parameter to tuplehash_create().
> I just thought if we're in the area of adjusting this API function's
> signature then it might be worth fixing the "long" issue at the same
> time, or at least in the same release.
I'm in favor of doing that, but it seems like a separate patch,
because we'd have to chase things back a fairly long way.
For instance, the numGroups fields in Agg, RecursiveUnion, and
SetOp are all "long" at the moment, and some of the callers
are getting their arguments via clamp_cardinality_to_long()
which'd need adjustment, etc etc.
> I'm also quite keen to see less use of long as it's not a very
> consistently sized datatype on all platforms which can lead to
> platform dependent bugs.
Yeah. Switching all these places to int64 likely would be
worthwhile cleanup (but I'm not volunteering). Also, if
tuplehash_create expects a uint32, that isn't consistent
either.
regards, tom lane
