On Mon, 1 Sept 2025 at 04:26, Michael Paquier <michael@paquier.xyz> wrote:
>
> So, taking a step back, I don't know what would be a good fit for
> these duplicates of the "next power" routines upper-bounded on input
> when attached to pg_bitutils.h. However, I do see that we can get rid
> of pg_log2() and dynahash.h with a consistent interface in
> pg_bitutils.h, by reducing my proposal to the introduction of
> pg_ceil_log2_32_bound() and pg_ceil_log2_64_bound().
>
> At the end, next_pow2_int64() and next_pow2_int() are a lesser deal to
> me, being static to dynahash.c. With that in mind, I am finishing
> with the attached. Less ambitious, still it's a nice cleanup IMO.
>
> What do you think?
+/*
+ * pg_ceil_log2_32_bound
+ * Returns equivalent of ceil(log2(num)), with overflow safeguard
+ * for pg_leftmost_one_pos32.
+ */
+static inline uint32
+pg_ceil_log2_32_bound(uint32 num)
+{
+ if (num > PG_INT32_MAX / 2)
+ num = PG_INT32_MAX / 2;
+
+ if (num < 2)
+ return 0;
+ else
+ return pg_leftmost_one_pos32(num - 1) + 1;
+}
So this is capping the result to be at most 30 (for any input greater
than or equal to 2^30, it will return 30). That's not at all clear
from the comments.
Also, why not have it call pg_ceil_log2_32(), instead of duplicating that code?
Alternatively, why not just impose the upper bound at the call sites
if needed, so there may be no need for these functions at all. For
example, looking at nodeHash.c, it would seem much more logical to
have ExecChooseHashTableSize() put an upper bound on nbuckets, rather
than capping log2_nbuckets after nbuckets has been chosen, risking
them getting out-of-sync.
Regards,
Dean
