Remove traces of long in dynahash.c
Hi all,
While looking at the recent business with dynahash.c in [1]/messages/by-id/aKF8mPDqgQb3uBjG@paquier.xyz -- Michael, I have
been reminded of the fact that this code still depends on long. Based
on my lookup of the code, I don't think that there is any issue with
the current uses, but I've never been a fan of using a type that's 8
byte everywhere except on WIN32, and we have been bitten by that in
the past depending on the code paths where long is used, even if WIN32
should be niche these days.
So I have looked at this file, and finished with the attached. The
result is nice, removing long from hsearch.h, as well as dynahash.c,
cleaning up some variable declarations on the way.
While monitoring the callers of the function signatures updated in
this patch, there is a mix of Size, int or int32 used in the variable
declarations used with the callers. There were a couple of long
declared in a couple of places like pgss, locking, predicate, shmem
that were declared as such to map with dynahash. These can be
replaced.
Thoughts?
[1]: /messages/by-id/aKF8mPDqgQb3uBjG@paquier.xyz -- Michael
--
Michael
Attachments:
0001-Replace-uses-of-long-by-uint64-in-dynahash.c-and-hse.patchtext/x-diff; charset=us-asciiDownload
From 0c516dab045030a710a41e596a4d54fd9a5fa6ac Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 19 Aug 2025 15:22:56 +0900
Subject: [PATCH] Replace uses of long by uint64 in dynahash.c and hsearch.h
---
src/include/storage/shmem.h | 2 +-
src/include/utils/dynahash.h | 2 +-
src/include/utils/hsearch.h | 16 ++--
src/backend/catalog/storage.c | 2 +-
src/backend/storage/ipc/shmem.c | 4 +-
src/backend/storage/lmgr/lock.c | 2 +-
src/backend/storage/lmgr/predicate.c | 2 +-
src/backend/utils/hash/dynahash.c | 94 +++++++++----------
.../pg_stat_statements/pg_stat_statements.c | 4 +-
9 files changed, 62 insertions(+), 66 deletions(-)
diff --git a/src/include/storage/shmem.h b/src/include/storage/shmem.h
index c1f668ded952..ce50829141bf 100644
--- a/src/include/storage/shmem.h
+++ b/src/include/storage/shmem.h
@@ -35,7 +35,7 @@ extern void *ShmemAllocNoError(Size size);
extern void *ShmemAllocUnlocked(Size size);
extern bool ShmemAddrIsValid(const void *addr);
extern void InitShmemIndex(void);
-extern HTAB *ShmemInitHash(const char *name, long init_size, long max_size,
+extern HTAB *ShmemInitHash(const char *name, uint64 init_size, uint64 max_size,
HASHCTL *infoP, int hash_flags);
extern void *ShmemInitStruct(const char *name, Size size, bool *foundPtr);
extern Size add_size(Size s1, Size s2);
diff --git a/src/include/utils/dynahash.h b/src/include/utils/dynahash.h
index 8a31d9524e2a..7d4fd0c6ce3d 100644
--- a/src/include/utils/dynahash.h
+++ b/src/include/utils/dynahash.h
@@ -15,6 +15,6 @@
#ifndef DYNAHASH_H
#define DYNAHASH_H
-extern int my_log2(long num);
+extern int my_log2(uint64 num);
#endif /* DYNAHASH_H */
diff --git a/src/include/utils/hsearch.h b/src/include/utils/hsearch.h
index 80deb8e543e6..a4f73d24ed4f 100644
--- a/src/include/utils/hsearch.h
+++ b/src/include/utils/hsearch.h
@@ -65,12 +65,12 @@ typedef struct HTAB HTAB;
typedef struct HASHCTL
{
/* Used if HASH_PARTITION flag is set: */
- long num_partitions; /* # partitions (must be power of 2) */
+ uint64 num_partitions; /* # partitions (must be power of 2) */
/* Used if HASH_SEGMENT flag is set: */
- long ssize; /* segment size */
+ uint64 ssize; /* segment size */
/* Used if HASH_DIRSIZE flag is set: */
- long dsize; /* (initial) directory size */
- long max_dsize; /* limit to dsize if dir size is limited */
+ uint64 dsize; /* (initial) directory size */
+ uint64 max_dsize; /* limit to dsize if dir size is limited */
/* Used if HASH_ELEM flag is set (which is now required): */
Size keysize; /* hash key length in bytes */
Size entrysize; /* total user element size in bytes */
@@ -129,7 +129,7 @@ typedef struct
/*
* prototypes for functions in dynahash.c
*/
-extern HTAB *hash_create(const char *tabname, long nelem,
+extern HTAB *hash_create(const char *tabname, uint64 nelem,
const HASHCTL *info, int flags);
extern void hash_destroy(HTAB *hashp);
extern void hash_stats(const char *caller, HTAB *hashp);
@@ -141,7 +141,7 @@ extern void *hash_search_with_hash_value(HTAB *hashp, const void *keyPtr,
bool *foundPtr);
extern bool hash_update_hash_key(HTAB *hashp, void *existingEntry,
const void *newKeyPtr);
-extern long hash_get_num_entries(HTAB *hashp);
+extern uint64 hash_get_num_entries(HTAB *hashp);
extern void hash_seq_init(HASH_SEQ_STATUS *status, HTAB *hashp);
extern void hash_seq_init_with_hash_value(HASH_SEQ_STATUS *status,
HTAB *hashp,
@@ -149,8 +149,8 @@ extern void hash_seq_init_with_hash_value(HASH_SEQ_STATUS *status,
extern void *hash_seq_search(HASH_SEQ_STATUS *status);
extern void hash_seq_term(HASH_SEQ_STATUS *status);
extern void hash_freeze(HTAB *hashp);
-extern Size hash_estimate_size(long num_entries, Size entrysize);
-extern long hash_select_dirsize(long num_entries);
+extern Size hash_estimate_size(uint64 num_entries, Size entrysize);
+extern uint64 hash_select_dirsize(uint64 num_entries);
extern Size hash_get_shared_size(HASHCTL *info, int flags);
extern void AtEOXact_HashTables(bool isCommit);
extern void AtEOSubXact_HashTables(bool isCommit, int nestDepth);
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index 227df90f89c9..2fad9db0d94a 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -586,7 +586,7 @@ RelFileLocatorSkippingWAL(RelFileLocator rlocator)
Size
EstimatePendingSyncsSpace(void)
{
- long entries;
+ uint64 entries;
entries = pendingSyncHash ? hash_get_num_entries(pendingSyncHash) : 0;
return mul_size(1 + entries, sizeof(RelFileLocator));
diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c
index d12a3ca06842..ef2e66ccd937 100644
--- a/src/backend/storage/ipc/shmem.c
+++ b/src/backend/storage/ipc/shmem.c
@@ -330,8 +330,8 @@ InitShmemIndex(void)
*/
HTAB *
ShmemInitHash(const char *name, /* table string name for shmem index */
- long init_size, /* initial table size */
- long max_size, /* max size of the table */
+ uint64 init_size, /* initial table size */
+ uint64 max_size, /* max size of the table */
HASHCTL *infoP, /* info about key and bucket size */
int hash_flags) /* info about infoP */
{
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index f8c88147160e..369390f765f9 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -443,7 +443,7 @@ void
LockManagerShmemInit(void)
{
HASHCTL info;
- long init_table_size,
+ uint64 init_table_size,
max_table_size;
bool found;
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index c07fb5883555..fb49b16cc72c 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -1145,7 +1145,7 @@ void
PredicateLockShmemInit(void)
{
HASHCTL info;
- long max_table_size;
+ uint64 max_table_size;
Size requestSize;
bool found;
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index a7094917c20c..e1714c5c5ab6 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -154,7 +154,7 @@ typedef HASHBUCKET *HASHSEGMENT;
typedef struct
{
slock_t mutex; /* spinlock for this freelist */
- long nentries; /* number of entries in associated buckets */
+ uint64 nentries; /* number of entries in associated buckets */
HASHELEMENT *freeList; /* chain of free elements */
} FreeListData;
@@ -182,8 +182,8 @@ struct HASHHDR
/* These fields can change, but not in a partitioned table */
/* Also, dsize can't change in a shared table, even if unpartitioned */
- long dsize; /* directory size */
- long nsegs; /* number of allocated segments (<= dsize) */
+ uint64 dsize; /* directory size */
+ uint64 nsegs; /* number of allocated segments (<= dsize) */
uint32 max_bucket; /* ID of maximum bucket in use */
uint32 high_mask; /* mask to modulo into entire table */
uint32 low_mask; /* mask to modulo into lower half of table */
@@ -191,9 +191,9 @@ struct HASHHDR
/* These fields are fixed at hashtable creation */
Size keysize; /* hash key length in bytes */
Size entrysize; /* total user element size in bytes */
- long num_partitions; /* # partitions (must be power of 2), or 0 */
- long max_dsize; /* 'dsize' limit if directory is fixed size */
- long ssize; /* segment size --- must be power of 2 */
+ uint64 num_partitions; /* # partitions (must be power of 2), or 0 */
+ uint64 max_dsize; /* 'dsize' limit if directory is fixed size */
+ uint64 ssize; /* segment size --- must be power of 2 */
int sshift; /* segment shift = log2(ssize) */
int nelem_alloc; /* number of entries to allocate at once */
bool isfixed; /* if true, don't enlarge */
@@ -236,7 +236,7 @@ struct HTAB
/* We keep local copies of these fixed values to reduce contention */
Size keysize; /* hash key length in bytes */
- long ssize; /* segment size --- must be power of 2 */
+ uint64 ssize; /* segment size --- must be power of 2 */
int sshift; /* segment shift = log2(ssize) */
/*
@@ -277,12 +277,12 @@ static bool expand_table(HTAB *hashp);
static HASHBUCKET get_hash_entry(HTAB *hashp, int freelist_idx);
static void hdefault(HTAB *hashp);
static int choose_nelem_alloc(Size entrysize);
-static bool init_htab(HTAB *hashp, long nelem);
+static bool init_htab(HTAB *hashp, uint64 nelem);
pg_noreturn static void hash_corrupted(HTAB *hashp);
static uint32 hash_initial_lookup(HTAB *hashp, uint32 hashvalue,
HASHBUCKET **bucketptr);
-static long next_pow2_long(long num);
-static int next_pow2_int(long num);
+static uint64 next_pow2_uint64(uint64 num);
+static int next_pow2_int(uint64 num);
static void register_seq_scan(HTAB *hashp);
static void deregister_seq_scan(HTAB *hashp);
static bool has_seq_scans(HTAB *hashp);
@@ -355,7 +355,7 @@ string_compare(const char *key1, const char *key2, Size keysize)
* large nelem will penalize hash_seq_search speed without buying much.
*/
HTAB *
-hash_create(const char *tabname, long nelem, const HASHCTL *info, int flags)
+hash_create(const char *tabname, uint64 nelem, const HASHCTL *info, int flags)
{
HTAB *hashp;
HASHHDR *hctl;
@@ -697,7 +697,7 @@ choose_nelem_alloc(Size entrysize)
* arrays
*/
static bool
-init_htab(HTAB *hashp, long nelem)
+init_htab(HTAB *hashp, uint64 nelem)
{
HASHHDR *hctl = hashp->hctl;
HASHSEGMENT *segp;
@@ -780,10 +780,10 @@ init_htab(HTAB *hashp, long nelem)
* NB: assumes that all hash structure parameters have default values!
*/
Size
-hash_estimate_size(long num_entries, Size entrysize)
+hash_estimate_size(uint64 num_entries, Size entrysize)
{
Size size;
- long nBuckets,
+ uint64 nBuckets,
nSegments,
nDirEntries,
nElementAllocs,
@@ -791,9 +791,9 @@ hash_estimate_size(long num_entries, Size entrysize)
elementAllocCnt;
/* estimate number of buckets wanted */
- nBuckets = next_pow2_long(num_entries);
+ nBuckets = next_pow2_uint64(num_entries);
/* # of segments needed for nBuckets */
- nSegments = next_pow2_long((nBuckets - 1) / DEF_SEGSIZE + 1);
+ nSegments = next_pow2_uint64((nBuckets - 1) / DEF_SEGSIZE + 1);
/* directory entries */
nDirEntries = DEF_DIRSIZE;
while (nDirEntries < nSegments)
@@ -826,17 +826,17 @@ hash_estimate_size(long num_entries, Size entrysize)
*
* XXX this had better agree with the behavior of init_htab()...
*/
-long
-hash_select_dirsize(long num_entries)
+uint64
+hash_select_dirsize(uint64 num_entries)
{
- long nBuckets,
+ uint64 nBuckets,
nSegments,
nDirEntries;
/* estimate number of buckets wanted */
- nBuckets = next_pow2_long(num_entries);
+ nBuckets = next_pow2_uint64(num_entries);
/* # of segments needed for nBuckets */
- nSegments = next_pow2_long((nBuckets - 1) / DEF_SEGSIZE + 1);
+ nSegments = next_pow2_uint64((nBuckets - 1) / DEF_SEGSIZE + 1);
/* directory entries */
nDirEntries = DEF_DIRSIZE;
while (nDirEntries < nSegments)
@@ -887,7 +887,7 @@ hash_stats(const char *caller, HTAB *hashp)
HASHHDR *hctl = hashp->hctl;
elog(DEBUG4,
- "hash_stats: Caller: %s Table Name: \"%s\" Accesses: " UINT64_FORMAT " Collisions: " UINT64_FORMAT " Expansions: " UINT64_FORMAT " Entries: %ld Key Size: %zu Max Bucket: %u Segment Count: %ld",
+ "hash_stats: Caller: %s Table Name: \"%s\" Accesses: " UINT64_FORMAT " Collisions: " UINT64_FORMAT " Expansions: " UINT64_FORMAT " Entries: " UINT64_FORMAT " Key Size: %zu Max Bucket: %u Segment Count: %ld",
caller != NULL ? caller : "(unknown)", hashp->tabname, hctl->accesses,
hctl->collisions, hctl->expansions, hash_get_num_entries(hashp),
hctl->keysize, hctl->max_bucket, hctl->nsegs);
@@ -993,7 +993,7 @@ hash_search_with_hash_value(HTAB *hashp,
* Can't split if running in partitioned mode, nor if frozen, nor if
* table is the subject of any active hash_seq_search scans.
*/
- if (hctl->freeList[0].nentries > (long) hctl->max_bucket &&
+ if (hctl->freeList[0].nentries > (uint64) hctl->max_bucket &&
!IS_PARTITIONED(hctl) && !hashp->frozen &&
!has_seq_scans(hashp))
(void) expand_table(hashp);
@@ -1332,11 +1332,11 @@ get_hash_entry(HTAB *hashp, int freelist_idx)
/*
* hash_get_num_entries -- get the number of entries in a hashtable
*/
-long
+uint64
hash_get_num_entries(HTAB *hashp)
{
int i;
- long sum = hashp->hctl->freeList[0].nentries;
+ uint64 sum = hashp->hctl->freeList[0].nentries;
/*
* We currently don't bother with acquiring the mutexes; it's only
@@ -1417,9 +1417,9 @@ hash_seq_search(HASH_SEQ_STATUS *status)
HTAB *hashp;
HASHHDR *hctl;
uint32 max_bucket;
- long ssize;
- long segment_num;
- long segment_ndx;
+ uint64 ssize;
+ uint64 segment_num;
+ uint64 segment_ndx;
HASHSEGMENT segp;
uint32 curBucket;
HASHELEMENT *curElem;
@@ -1518,7 +1518,7 @@ hash_seq_term(HASH_SEQ_STATUS *status)
* still allowed)
*
* The reason for doing this is that by preventing any more bucket splits,
- * we no longer need to worry about registering hash_seq_search scans,
+ * we no uint64er need to worry about registering hash_seq_search scans,
* and thus caller need not be careful about ensuring hash_seq_term gets
* called at the right times.
*
@@ -1548,11 +1548,11 @@ expand_table(HTAB *hashp)
HASHHDR *hctl = hashp->hctl;
HASHSEGMENT old_seg,
new_seg;
- long old_bucket,
+ uint64 old_bucket,
new_bucket;
- long new_segnum,
+ uint64 new_segnum,
new_segndx;
- long old_segnum,
+ uint64 old_segnum,
old_segndx;
HASHBUCKET *oldlink,
*newlink;
@@ -1620,7 +1620,7 @@ expand_table(HTAB *hashp)
currElement = nextElement)
{
nextElement = currElement->link;
- if ((long) calc_bucket(hctl, currElement->hashvalue) == old_bucket)
+ if ((uint64) calc_bucket(hctl, currElement->hashvalue) == old_bucket)
{
*oldlink = currElement;
oldlink = &currElement->link;
@@ -1644,9 +1644,9 @@ dir_realloc(HTAB *hashp)
{
HASHSEGMENT *p;
HASHSEGMENT *old_p;
- long new_dsize;
- long old_dirsize;
- long new_dirsize;
+ uint64 new_dsize;
+ uint64 old_dirsize;
+ uint64 new_dirsize;
if (hashp->hctl->max_dsize != NO_MAX_DSIZE)
return false;
@@ -1780,8 +1780,8 @@ hash_initial_lookup(HTAB *hashp, uint32 hashvalue, HASHBUCKET **bucketptr)
{
HASHHDR *hctl = hashp->hctl;
HASHSEGMENT segp;
- long segment_num;
- long segment_ndx;
+ uint64 segment_num;
+ uint64 segment_ndx;
uint32 bucket;
bucket = calc_bucket(hctl, hashvalue);
@@ -1814,25 +1814,21 @@ hash_corrupted(HTAB *hashp)
/* calculate ceil(log base 2) of num */
int
-my_log2(long num)
+my_log2(uint64 num)
{
/*
* guard against too-large input, which would be invalid for
* pg_ceil_log2_*()
*/
- if (num > LONG_MAX / 2)
- num = LONG_MAX / 2;
+ if (num > PG_UINT64_MAX / 2)
+ num = PG_UINT64_MAX / 2;
-#if SIZEOF_LONG < 8
- return pg_ceil_log2_32(num);
-#else
return pg_ceil_log2_64(num);
-#endif
}
-/* calculate first power of 2 >= num, bounded to what will fit in a long */
-static long
-next_pow2_long(long num)
+/* calculate first power of 2 >= num, bounded to what will fit in a uint64 */
+static uint64
+next_pow2_uint64(uint64 num)
{
/* my_log2's internal range check is sufficient */
return 1L << my_log2(num);
@@ -1840,7 +1836,7 @@ next_pow2_long(long num)
/* calculate first power of 2 >= num, bounded to what will fit in an int */
static int
-next_pow2_int(long num)
+next_pow2_int(uint64 num)
{
if (num > INT_MAX / 2)
num = INT_MAX / 2;
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 9fc9635d3300..11824b997074 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -2713,8 +2713,8 @@ entry_reset(Oid userid, Oid dbid, int64 queryid, bool minmax_only)
HASH_SEQ_STATUS hash_seq;
pgssEntry *entry;
FILE *qfile;
- long num_entries;
- long num_remove = 0;
+ uint64 num_entries;
+ uint64 num_remove = 0;
pgssHashKey key;
TimestampTz stats_reset;
--
2.50.0
On Aug 19, 2025, at 14:24, Michael Paquier <michael@paquier.xyz> wrote:
--
Michael
<0001-Replace-uses-of-long-by-uint64-in-dynahash.c-and-hse.patch>
-static long next_pow2_long(long num);
-static int next_pow2_int(long num);
+static uint64 next_pow2_uint64(uint64 num);
+static int next_pow2_int(uint64 num);There are already pg_nextpower2_64() and pg_nextpower2_32() in pg_bitutils.h, feels like the new replacement functions are duplicate to the existing ones.
- * we no longer need to worry about registering hash_seq_search scans,
+ * we no uint64er need to worry about registering hash_seq_search scans,Looks like a bad auto replacement: “no longer”, here “long" should be kept.
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Chao Li <li.evan.chao@gmail.com> writes:
On Aug 19, 2025, at 14:24, Michael Paquier <michael@paquier.xyz> wrote:
<0001-Replace-uses-of-long-by-uint64-in-dynahash.c-and-hse.patch>
There are already pg_nextpower2_64() and pg_nextpower2_32() in pg_bitutils.h, feels like the new replacement functions are duplicate to the existing ones.
It always seemed weird to me that dynahash.c has its own bit-twiddling
functions. (There are indications in the source code that it was once
a standalone test program, which perhaps explains that.)
+1 for getting rid of those while we're doing janitorial work here.
They're not *quite* duplicates though, for instance next_pow2_int has
different response to out-of-range values than pg_nextpower2_32.
regards, tom lane
On 19.08.25 08:24, Michael Paquier wrote:
While looking at the recent business with dynahash.c in [1], I have
been reminded of the fact that this code still depends on long.
It's definitely a good idea to get rid of "long" usage. But you can
also replace it with long long instead of int64. I suppose this is a
stylistic question, but I would tend to use the intNN types only when I
need exactly that many bits.
Also, your patch changes from signed to unsigned types. Maybe that's
ok, but you didn't explain it.
On Tue, Aug 19, 2025 at 10:46:58AM -0400, Tom Lane wrote:
+1 for getting rid of those while we're doing janitorial work here.
They're not *quite* duplicates though, for instance next_pow2_int has
different response to out-of-range values than pg_nextpower2_32.
This would mean introducing more flavors in pg_bitutils.h with limit
checks. That does not seem completely right to do in this file, which
is a wrapper for all the __builtin_*() calls? A second point is on
the signedness but we could just cap the maximum at
(PG_UINT{32,64}_MAX / 2), I guess, with two new routines like:
uint64 pg_nextpower2_64_max(uint64 num);
uint32 pg_prevpower2_32_max(uint32 num);
And then cast the unsigned results back to signed in dynahash.c.
Without this point, I have switched the patch to use int64, keeping
the signedness the same as the original. I have missed that there was
one spot where we relied on NO_MAX_DSIZE.
--
Michael
Attachments:
v2-0001-Replace-uses-of-long-by-int64-in-dynahash.c-and-h.patchtext/x-diff; charset=us-asciiDownload
From 53d7438423d53cb6f65549b93ec82cd5f14a72d7 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Wed, 20 Aug 2025 16:27:11 +0900
Subject: [PATCH v2] Replace uses of long by int64 in dynahash.c and hsearch.h
---
src/include/storage/shmem.h | 2 +-
src/include/utils/dynahash.h | 2 +-
src/include/utils/hsearch.h | 16 ++--
src/backend/catalog/storage.c | 2 +-
src/backend/storage/ipc/shmem.c | 4 +-
src/backend/storage/lmgr/lock.c | 2 +-
src/backend/storage/lmgr/predicate.c | 2 +-
src/backend/utils/hash/dynahash.c | 92 +++++++++----------
.../pg_stat_statements/pg_stat_statements.c | 4 +-
9 files changed, 61 insertions(+), 65 deletions(-)
diff --git a/src/include/storage/shmem.h b/src/include/storage/shmem.h
index c1f668ded952..8604feca93ba 100644
--- a/src/include/storage/shmem.h
+++ b/src/include/storage/shmem.h
@@ -35,7 +35,7 @@ extern void *ShmemAllocNoError(Size size);
extern void *ShmemAllocUnlocked(Size size);
extern bool ShmemAddrIsValid(const void *addr);
extern void InitShmemIndex(void);
-extern HTAB *ShmemInitHash(const char *name, long init_size, long max_size,
+extern HTAB *ShmemInitHash(const char *name, int64 init_size, int64 max_size,
HASHCTL *infoP, int hash_flags);
extern void *ShmemInitStruct(const char *name, Size size, bool *foundPtr);
extern Size add_size(Size s1, Size s2);
diff --git a/src/include/utils/dynahash.h b/src/include/utils/dynahash.h
index 8a31d9524e2a..a4362d3f65e5 100644
--- a/src/include/utils/dynahash.h
+++ b/src/include/utils/dynahash.h
@@ -15,6 +15,6 @@
#ifndef DYNAHASH_H
#define DYNAHASH_H
-extern int my_log2(long num);
+extern int my_log2(int64 num);
#endif /* DYNAHASH_H */
diff --git a/src/include/utils/hsearch.h b/src/include/utils/hsearch.h
index 80deb8e543e6..cb09a4cbe8cb 100644
--- a/src/include/utils/hsearch.h
+++ b/src/include/utils/hsearch.h
@@ -65,12 +65,12 @@ typedef struct HTAB HTAB;
typedef struct HASHCTL
{
/* Used if HASH_PARTITION flag is set: */
- long num_partitions; /* # partitions (must be power of 2) */
+ int64 num_partitions; /* # partitions (must be power of 2) */
/* Used if HASH_SEGMENT flag is set: */
- long ssize; /* segment size */
+ int64 ssize; /* segment size */
/* Used if HASH_DIRSIZE flag is set: */
- long dsize; /* (initial) directory size */
- long max_dsize; /* limit to dsize if dir size is limited */
+ int64 dsize; /* (initial) directory size */
+ int64 max_dsize; /* limit to dsize if dir size is limited */
/* Used if HASH_ELEM flag is set (which is now required): */
Size keysize; /* hash key length in bytes */
Size entrysize; /* total user element size in bytes */
@@ -129,7 +129,7 @@ typedef struct
/*
* prototypes for functions in dynahash.c
*/
-extern HTAB *hash_create(const char *tabname, long nelem,
+extern HTAB *hash_create(const char *tabname, int64 nelem,
const HASHCTL *info, int flags);
extern void hash_destroy(HTAB *hashp);
extern void hash_stats(const char *caller, HTAB *hashp);
@@ -141,7 +141,7 @@ extern void *hash_search_with_hash_value(HTAB *hashp, const void *keyPtr,
bool *foundPtr);
extern bool hash_update_hash_key(HTAB *hashp, void *existingEntry,
const void *newKeyPtr);
-extern long hash_get_num_entries(HTAB *hashp);
+extern int64 hash_get_num_entries(HTAB *hashp);
extern void hash_seq_init(HASH_SEQ_STATUS *status, HTAB *hashp);
extern void hash_seq_init_with_hash_value(HASH_SEQ_STATUS *status,
HTAB *hashp,
@@ -149,8 +149,8 @@ extern void hash_seq_init_with_hash_value(HASH_SEQ_STATUS *status,
extern void *hash_seq_search(HASH_SEQ_STATUS *status);
extern void hash_seq_term(HASH_SEQ_STATUS *status);
extern void hash_freeze(HTAB *hashp);
-extern Size hash_estimate_size(long num_entries, Size entrysize);
-extern long hash_select_dirsize(long num_entries);
+extern Size hash_estimate_size(int64 num_entries, Size entrysize);
+extern int64 hash_select_dirsize(int64 num_entries);
extern Size hash_get_shared_size(HASHCTL *info, int flags);
extern void AtEOXact_HashTables(bool isCommit);
extern void AtEOSubXact_HashTables(bool isCommit, int nestDepth);
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index 227df90f89c9..fb784acf4af2 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -586,7 +586,7 @@ RelFileLocatorSkippingWAL(RelFileLocator rlocator)
Size
EstimatePendingSyncsSpace(void)
{
- long entries;
+ int64 entries;
entries = pendingSyncHash ? hash_get_num_entries(pendingSyncHash) : 0;
return mul_size(1 + entries, sizeof(RelFileLocator));
diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c
index d12a3ca06842..63ac63bd6d49 100644
--- a/src/backend/storage/ipc/shmem.c
+++ b/src/backend/storage/ipc/shmem.c
@@ -330,8 +330,8 @@ InitShmemIndex(void)
*/
HTAB *
ShmemInitHash(const char *name, /* table string name for shmem index */
- long init_size, /* initial table size */
- long max_size, /* max size of the table */
+ int64 init_size, /* initial table size */
+ int64 max_size, /* max size of the table */
HASHCTL *infoP, /* info about key and bucket size */
int hash_flags) /* info about infoP */
{
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index f8c88147160e..233b85b623d5 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -443,7 +443,7 @@ void
LockManagerShmemInit(void)
{
HASHCTL info;
- long init_table_size,
+ int64 init_table_size,
max_table_size;
bool found;
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index c07fb5883555..c1d8511ad17a 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -1145,7 +1145,7 @@ void
PredicateLockShmemInit(void)
{
HASHCTL info;
- long max_table_size;
+ int64 max_table_size;
Size requestSize;
bool found;
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index a7094917c20c..1aeee5be42ac 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -154,7 +154,7 @@ typedef HASHBUCKET *HASHSEGMENT;
typedef struct
{
slock_t mutex; /* spinlock for this freelist */
- long nentries; /* number of entries in associated buckets */
+ int64 nentries; /* number of entries in associated buckets */
HASHELEMENT *freeList; /* chain of free elements */
} FreeListData;
@@ -182,8 +182,8 @@ struct HASHHDR
/* These fields can change, but not in a partitioned table */
/* Also, dsize can't change in a shared table, even if unpartitioned */
- long dsize; /* directory size */
- long nsegs; /* number of allocated segments (<= dsize) */
+ int64 dsize; /* directory size */
+ int64 nsegs; /* number of allocated segments (<= dsize) */
uint32 max_bucket; /* ID of maximum bucket in use */
uint32 high_mask; /* mask to modulo into entire table */
uint32 low_mask; /* mask to modulo into lower half of table */
@@ -191,9 +191,9 @@ struct HASHHDR
/* These fields are fixed at hashtable creation */
Size keysize; /* hash key length in bytes */
Size entrysize; /* total user element size in bytes */
- long num_partitions; /* # partitions (must be power of 2), or 0 */
- long max_dsize; /* 'dsize' limit if directory is fixed size */
- long ssize; /* segment size --- must be power of 2 */
+ int64 num_partitions; /* # partitions (must be power of 2), or 0 */
+ int64 max_dsize; /* 'dsize' limit if directory is fixed size */
+ int64 ssize; /* segment size --- must be power of 2 */
int sshift; /* segment shift = log2(ssize) */
int nelem_alloc; /* number of entries to allocate at once */
bool isfixed; /* if true, don't enlarge */
@@ -236,7 +236,7 @@ struct HTAB
/* We keep local copies of these fixed values to reduce contention */
Size keysize; /* hash key length in bytes */
- long ssize; /* segment size --- must be power of 2 */
+ int64 ssize; /* segment size --- must be power of 2 */
int sshift; /* segment shift = log2(ssize) */
/*
@@ -277,12 +277,12 @@ static bool expand_table(HTAB *hashp);
static HASHBUCKET get_hash_entry(HTAB *hashp, int freelist_idx);
static void hdefault(HTAB *hashp);
static int choose_nelem_alloc(Size entrysize);
-static bool init_htab(HTAB *hashp, long nelem);
+static bool init_htab(HTAB *hashp, int64 nelem);
pg_noreturn static void hash_corrupted(HTAB *hashp);
static uint32 hash_initial_lookup(HTAB *hashp, uint32 hashvalue,
HASHBUCKET **bucketptr);
-static long next_pow2_long(long num);
-static int next_pow2_int(long num);
+static int64 next_pow2_int64(int64 num);
+static int next_pow2_int(int64 num);
static void register_seq_scan(HTAB *hashp);
static void deregister_seq_scan(HTAB *hashp);
static bool has_seq_scans(HTAB *hashp);
@@ -355,7 +355,7 @@ string_compare(const char *key1, const char *key2, Size keysize)
* large nelem will penalize hash_seq_search speed without buying much.
*/
HTAB *
-hash_create(const char *tabname, long nelem, const HASHCTL *info, int flags)
+hash_create(const char *tabname, int64 nelem, const HASHCTL *info, int flags)
{
HTAB *hashp;
HASHHDR *hctl;
@@ -697,7 +697,7 @@ choose_nelem_alloc(Size entrysize)
* arrays
*/
static bool
-init_htab(HTAB *hashp, long nelem)
+init_htab(HTAB *hashp, int64 nelem)
{
HASHHDR *hctl = hashp->hctl;
HASHSEGMENT *segp;
@@ -780,10 +780,10 @@ init_htab(HTAB *hashp, long nelem)
* NB: assumes that all hash structure parameters have default values!
*/
Size
-hash_estimate_size(long num_entries, Size entrysize)
+hash_estimate_size(int64 num_entries, Size entrysize)
{
Size size;
- long nBuckets,
+ int64 nBuckets,
nSegments,
nDirEntries,
nElementAllocs,
@@ -791,9 +791,9 @@ hash_estimate_size(long num_entries, Size entrysize)
elementAllocCnt;
/* estimate number of buckets wanted */
- nBuckets = next_pow2_long(num_entries);
+ nBuckets = next_pow2_int64(num_entries);
/* # of segments needed for nBuckets */
- nSegments = next_pow2_long((nBuckets - 1) / DEF_SEGSIZE + 1);
+ nSegments = next_pow2_int64((nBuckets - 1) / DEF_SEGSIZE + 1);
/* directory entries */
nDirEntries = DEF_DIRSIZE;
while (nDirEntries < nSegments)
@@ -826,17 +826,17 @@ hash_estimate_size(long num_entries, Size entrysize)
*
* XXX this had better agree with the behavior of init_htab()...
*/
-long
-hash_select_dirsize(long num_entries)
+int64
+hash_select_dirsize(int64 num_entries)
{
- long nBuckets,
+ int64 nBuckets,
nSegments,
nDirEntries;
/* estimate number of buckets wanted */
- nBuckets = next_pow2_long(num_entries);
+ nBuckets = next_pow2_int64(num_entries);
/* # of segments needed for nBuckets */
- nSegments = next_pow2_long((nBuckets - 1) / DEF_SEGSIZE + 1);
+ nSegments = next_pow2_int64((nBuckets - 1) / DEF_SEGSIZE + 1);
/* directory entries */
nDirEntries = DEF_DIRSIZE;
while (nDirEntries < nSegments)
@@ -887,7 +887,7 @@ hash_stats(const char *caller, HTAB *hashp)
HASHHDR *hctl = hashp->hctl;
elog(DEBUG4,
- "hash_stats: Caller: %s Table Name: \"%s\" Accesses: " UINT64_FORMAT " Collisions: " UINT64_FORMAT " Expansions: " UINT64_FORMAT " Entries: %ld Key Size: %zu Max Bucket: %u Segment Count: %ld",
+ "hash_stats: Caller: %s Table Name: \"%s\" Accesses: " UINT64_FORMAT " Collisions: " UINT64_FORMAT " Expansions: " UINT64_FORMAT " Entries: " INT64_FORMAT " Key Size: %zu Max Bucket: %u Segment Count: " INT64_FORMAT,
caller != NULL ? caller : "(unknown)", hashp->tabname, hctl->accesses,
hctl->collisions, hctl->expansions, hash_get_num_entries(hashp),
hctl->keysize, hctl->max_bucket, hctl->nsegs);
@@ -993,7 +993,7 @@ hash_search_with_hash_value(HTAB *hashp,
* Can't split if running in partitioned mode, nor if frozen, nor if
* table is the subject of any active hash_seq_search scans.
*/
- if (hctl->freeList[0].nentries > (long) hctl->max_bucket &&
+ if (hctl->freeList[0].nentries > (int64) hctl->max_bucket &&
!IS_PARTITIONED(hctl) && !hashp->frozen &&
!has_seq_scans(hashp))
(void) expand_table(hashp);
@@ -1332,11 +1332,11 @@ get_hash_entry(HTAB *hashp, int freelist_idx)
/*
* hash_get_num_entries -- get the number of entries in a hashtable
*/
-long
+int64
hash_get_num_entries(HTAB *hashp)
{
int i;
- long sum = hashp->hctl->freeList[0].nentries;
+ int64 sum = hashp->hctl->freeList[0].nentries;
/*
* We currently don't bother with acquiring the mutexes; it's only
@@ -1417,9 +1417,9 @@ hash_seq_search(HASH_SEQ_STATUS *status)
HTAB *hashp;
HASHHDR *hctl;
uint32 max_bucket;
- long ssize;
- long segment_num;
- long segment_ndx;
+ int64 ssize;
+ int64 segment_num;
+ int64 segment_ndx;
HASHSEGMENT segp;
uint32 curBucket;
HASHELEMENT *curElem;
@@ -1548,11 +1548,11 @@ expand_table(HTAB *hashp)
HASHHDR *hctl = hashp->hctl;
HASHSEGMENT old_seg,
new_seg;
- long old_bucket,
+ int64 old_bucket,
new_bucket;
- long new_segnum,
+ int64 new_segnum,
new_segndx;
- long old_segnum,
+ int64 old_segnum,
old_segndx;
HASHBUCKET *oldlink,
*newlink;
@@ -1620,7 +1620,7 @@ expand_table(HTAB *hashp)
currElement = nextElement)
{
nextElement = currElement->link;
- if ((long) calc_bucket(hctl, currElement->hashvalue) == old_bucket)
+ if ((int64) calc_bucket(hctl, currElement->hashvalue) == old_bucket)
{
*oldlink = currElement;
oldlink = &currElement->link;
@@ -1644,9 +1644,9 @@ dir_realloc(HTAB *hashp)
{
HASHSEGMENT *p;
HASHSEGMENT *old_p;
- long new_dsize;
- long old_dirsize;
- long new_dirsize;
+ int64 new_dsize;
+ int64 old_dirsize;
+ int64 new_dirsize;
if (hashp->hctl->max_dsize != NO_MAX_DSIZE)
return false;
@@ -1780,8 +1780,8 @@ hash_initial_lookup(HTAB *hashp, uint32 hashvalue, HASHBUCKET **bucketptr)
{
HASHHDR *hctl = hashp->hctl;
HASHSEGMENT segp;
- long segment_num;
- long segment_ndx;
+ int64 segment_num;
+ int64 segment_ndx;
uint32 bucket;
bucket = calc_bucket(hctl, hashvalue);
@@ -1814,25 +1814,21 @@ hash_corrupted(HTAB *hashp)
/* calculate ceil(log base 2) of num */
int
-my_log2(long num)
+my_log2(int64 num)
{
/*
* guard against too-large input, which would be invalid for
* pg_ceil_log2_*()
*/
- if (num > LONG_MAX / 2)
- num = LONG_MAX / 2;
+ if (num > PG_INT64_MAX / 2)
+ num = PG_INT64_MAX / 2;
-#if SIZEOF_LONG < 8
- return pg_ceil_log2_32(num);
-#else
return pg_ceil_log2_64(num);
-#endif
}
-/* calculate first power of 2 >= num, bounded to what will fit in a long */
-static long
-next_pow2_long(long num)
+/* calculate first power of 2 >= num, bounded to what will fit in a int64 */
+static int64
+next_pow2_int64(int64 num)
{
/* my_log2's internal range check is sufficient */
return 1L << my_log2(num);
@@ -1840,7 +1836,7 @@ next_pow2_long(long num)
/* calculate first power of 2 >= num, bounded to what will fit in an int */
static int
-next_pow2_int(long num)
+next_pow2_int(int64 num)
{
if (num > INT_MAX / 2)
num = INT_MAX / 2;
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 9fc9635d3300..1cb368c8590b 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -2713,8 +2713,8 @@ entry_reset(Oid userid, Oid dbid, int64 queryid, bool minmax_only)
HASH_SEQ_STATUS hash_seq;
pgssEntry *entry;
FILE *qfile;
- long num_entries;
- long num_remove = 0;
+ int64 num_entries;
+ int64 num_remove = 0;
pgssHashKey key;
TimestampTz stats_reset;
--
2.50.0
On Aug 20, 2025, at 15:40, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Aug 19, 2025 at 10:46:58AM -0400, Tom Lane wrote:
+1 for getting rid of those while we're doing janitorial work here.
They're not *quite* duplicates though, for instance next_pow2_int has
different response to out-of-range values than pg_nextpower2_32.This would mean introducing more flavors in pg_bitutils.h with limit
checks. That does not seem completely right to do in this file, which
is a wrapper for all the __builtin_*() calls? A second point is on
the signedness but we could just cap the maximum at
(PG_UINT{32,64}_MAX / 2), I guess, with two new routines like:
uint64 pg_nextpower2_64_max(uint64 num);
uint32 pg_prevpower2_32_max(uint32 num);
I wonder if we can keep the same naming style to make the new function name like next_pow2_64()?
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Wed, Aug 20, 2025 at 04:14:15PM +0800, Chao Li wrote:
I wonder if we can keep the same naming style to make the new
function name like next_pow2_64()?
I don't think that this would be a good idea to have new routines
published in pg_bitutils.h with names inconsistent with the existing
one. next_pow2_long() and next_pow2_int() are now local to
dynahash.c, so we don't really have to follow their naming pattern.
It would be more important to me to choose a new name, rather in line
with the other ones.
After sleeping on it, I am not sure what to do with these routines. I
don't deny that more refactoring can be done. However, all that can
also happen outside the long -> int64 switch I am suggesting.
Any comments from others?
--
Michael
Hi Michael,
On Aug 21, 2025, at 07:07, Michael Paquier <michael@paquier.xyz> wrote:
After sleeping on it, I am not sure what to do with these routines. I
don't deny that more refactoring can be done. However, all that can
also happen outside the long -> int64 switch I am suggesting.Any comments from others?
--
Michael
I don’t want to block you. Unless others have the same comment about naming, you can go ahead and move forward. I agree further refactoring can belong to a separate commit.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Michael Paquier <michael@paquier.xyz> writes:
On Wed, Aug 20, 2025 at 04:14:15PM +0800, Chao Li wrote:
I wonder if we can keep the same naming style to make the new
function name like next_pow2_64()?
I don't think that this would be a good idea to have new routines
published in pg_bitutils.h with names inconsistent with the existing
one. next_pow2_long() and next_pow2_int() are now local to
dynahash.c, so we don't really have to follow their naming pattern.
It would be more important to me to choose a new name, rather in line
with the other ones.
Yes, the precedent to follow here is the naming conventions in
pg_bitutils.h.
After sleeping on it, I am not sure what to do with these routines. I
don't deny that more refactoring can be done. However, all that can
also happen outside the long -> int64 switch I am suggesting.
If you prefer to regard this as an independent issue, that's okay with
me ... but it's touching most of the same lines of code, so it seems
to me that it'd be about as easy to deal with both items at once.
regards, tom lane
On Thu, Aug 21, 2025 at 12:53:09AM -0400, Tom Lane wrote:
If you prefer to regard this as an independent issue, that's okay with
me ... but it's touching most of the same lines of code, so it seems
to me that it'd be about as easy to deal with both items at once.
I'd rather do that after a second look at the whole picture as this
led to an accumulation of bullet points. The long->int64 switch was
looking OK on its own and I have applied it.
Attached is the second piece of refactoring, where I have introduced a
couple more APIs in pg_bitutils.h (cross-checked the resulting
computations of the old and new routines with some quick hacks, in
case, and they matched):
pg_nextpower2_32_bound, replacing next_pow2_int
pg_nextpower2_64_bound, replacing next_pow2_int64
pg_ceil_log2_64_bound, replacing my_log2()
pg_ceil_log2_32_bound, not used, present for symmetry.
An extra thing is a suggested change for pg_nextpower2_32(), to use a
uint64 instead of a uint32 as argument, which is caused by
next_pow2_int64() and next_pow2_int(), that both used int64
previously.
There's likely some opinion differences according to one's taste;
that's my idea of the refactoring to remove the duplication.
--
Michael
Attachments:
0001-Clean-up-next-pow2-routines-of-dynahash.c-moving-to-.patchtext/x-diff; charset=us-asciiDownload
From e4a6e9da211c7d81b226c3b714449d90bf309470 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Fri, 22 Aug 2025 12:36:04 +0900
Subject: [PATCH] Clean up next-pow2 routines of dynahash.c, moving to
bitutils.h
---
src/include/port/pg_bitutils.h | 66 +++++++++++++++++++++++-
src/include/utils/dynahash.h | 20 -------
src/backend/access/heap/heapam.c | 6 +--
src/backend/executor/nodeAgg.c | 3 +-
src/backend/executor/nodeHash.c | 7 ++-
src/backend/replication/logical/worker.c | 3 +-
src/backend/utils/hash/dynahash.c | 50 +++---------------
7 files changed, 80 insertions(+), 75 deletions(-)
delete mode 100644 src/include/utils/dynahash.h
diff --git a/src/include/port/pg_bitutils.h b/src/include/port/pg_bitutils.h
index c7901bf8ddc0..10478e702c74 100644
--- a/src/include/port/pg_bitutils.h
+++ b/src/include/port/pg_bitutils.h
@@ -186,7 +186,7 @@ pg_rightmost_one_pos64(uint64 word)
* 'num' mustn't be 0 or be above PG_UINT32_MAX / 2 + 1.
*/
static inline uint32
-pg_nextpower2_32(uint32 num)
+pg_nextpower2_32(uint64 num)
{
Assert(num > 0 && num <= PG_UINT32_MAX / 2 + 1);
@@ -198,7 +198,7 @@ pg_nextpower2_32(uint32 num)
if ((num & (num - 1)) == 0)
return num; /* already power 2 */
- return ((uint32) 1) << (pg_leftmost_one_pos32(num) + 1);
+ return ((uint32) 1) << (pg_leftmost_one_pos32((uint32) num) + 1);
}
/*
@@ -224,6 +224,34 @@ pg_nextpower2_64(uint64 num)
return ((uint64) 1) << (pg_leftmost_one_pos64(num) + 1);
}
+/*
+ * pg_nextpower2_32_bound
+ * Returns the next higher power of 2 above 'num', or 'num' if it's
+ * already a power of 2, with upper bound safeguard.
+ */
+static inline uint32
+pg_nextpower2_32_bound(uint64 num)
+{
+ if (num > PG_INT32_MAX / 2)
+ num = PG_INT32_MAX / 2;
+
+ return pg_nextpower2_32(num);
+}
+
+/*
+ * pg_nextpower2_64_bound
+ * Returns the next higher power of 2 above 'num', or 'num' if it's
+ * already a power of 2, with upper bound safeguard.
+ */
+static inline uint64
+pg_nextpower2_64_bound(uint64 num)
+{
+ if (num > PG_INT64_MAX / 2)
+ num = PG_INT64_MAX / 2;
+
+ return pg_nextpower2_64(num);
+}
+
/*
* pg_prevpower2_32
* Returns the next lower power of 2 below 'num', or 'num' if it's
@@ -276,6 +304,40 @@ pg_ceil_log2_64(uint64 num)
return pg_leftmost_one_pos64(num - 1) + 1;
}
+/*
+ * pg_ceil_log2_64_bound
+ * Returns equivalent of ceil(log2(num)), with overflow safeguard
+ * for pg_leftmost_one_pos32.
+ */
+static inline uint32
+pg_ceil_log2_32_bound(uint64 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;
+}
+
+/*
+ * pg_ceil_log2_64_bound
+ * Returns equivalent of ceil(log2(num)), with overflow safeguard
+ * for pg_leftmost_one_pos64.
+ */
+static inline uint64
+pg_ceil_log2_64_bound(uint64 num)
+{
+ if (num > PG_INT64_MAX / 2)
+ num = PG_INT64_MAX / 2;
+
+ if (num < 2)
+ return 0;
+ else
+ return pg_leftmost_one_pos64(num - 1) + 1;
+}
+
/*
* With MSVC on x86_64 builds, try using native popcnt instructions via the
* __popcnt and __popcnt64 intrinsics. These don't work the same as GCC's
diff --git a/src/include/utils/dynahash.h b/src/include/utils/dynahash.h
deleted file mode 100644
index a4362d3f65e5..000000000000
--- a/src/include/utils/dynahash.h
+++ /dev/null
@@ -1,20 +0,0 @@
-/*-------------------------------------------------------------------------
- *
- * dynahash.h
- * POSTGRES dynahash.h file definitions
- *
- *
- * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
- * Portions Copyright (c) 1994, Regents of the University of California
- *
- * IDENTIFICATION
- * src/include/utils/dynahash.h
- *
- *-------------------------------------------------------------------------
- */
-#ifndef DYNAHASH_H
-#define DYNAHASH_H
-
-extern int my_log2(int64 num);
-
-#endif /* DYNAHASH_H */
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 0dcd6ee817e0..9bfe55f9e214 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -8617,8 +8617,8 @@ bottomup_sort_and_shrink_cmp(const void *arg1, const void *arg2)
*/
if (group1->ntids != group2->ntids)
{
- uint32 ntids1 = pg_nextpower2_32((uint32) group1->ntids);
- uint32 ntids2 = pg_nextpower2_32((uint32) group2->ntids);
+ uint32 ntids1 = pg_nextpower2_32((uint64) group1->ntids);
+ uint32 ntids2 = pg_nextpower2_32((uint64) group2->ntids);
if (ntids1 > ntids2)
return -1;
@@ -8745,7 +8745,7 @@ bottomup_sort_and_shrink(TM_IndexDeleteOp *delstate)
group->npromisingtids = 4;
else
group->npromisingtids =
- pg_nextpower2_32((uint32) group->npromisingtids);
+ pg_nextpower2_32((uint64) group->npromisingtids);
}
/* Sort groups and rearrange caller's deltids array */
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 377e016d7322..b5fa9af9d992 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -267,7 +267,6 @@
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/datum.h"
-#include "utils/dynahash.h"
#include "utils/expandeddatum.h"
#include "utils/injection_point.h"
#include "utils/logtape.h"
@@ -2115,7 +2114,7 @@ hash_choose_num_partitions(double input_groups, double hashentrysize,
npartitions = (int) dpartitions;
/* ceil(log2(npartitions)) */
- partition_bits = my_log2(npartitions);
+ partition_bits = pg_ceil_log2_64_bound(npartitions);
/* make sure that we don't exhaust the hash bits */
if (partition_bits + used_bits >= 32)
diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c
index 8d2201ab67fa..14d934ab42b2 100644
--- a/src/backend/executor/nodeHash.c
+++ b/src/backend/executor/nodeHash.c
@@ -36,7 +36,6 @@
#include "executor/nodeHashjoin.h"
#include "miscadmin.h"
#include "port/pg_bitutils.h"
-#include "utils/dynahash.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/syscache.h"
@@ -340,7 +339,7 @@ MultiExecParallelHash(HashState *node)
*/
hashtable->curbatch = -1;
hashtable->nbuckets = pstate->nbuckets;
- hashtable->log2_nbuckets = my_log2(hashtable->nbuckets);
+ hashtable->log2_nbuckets = pg_ceil_log2_64_bound(hashtable->nbuckets);
hashtable->totalTuples = pstate->total_tuples;
/*
@@ -480,7 +479,7 @@ ExecHashTableCreate(HashState *state)
&nbuckets, &nbatch, &num_skew_mcvs);
/* nbuckets must be a power of 2 */
- log2_nbuckets = my_log2(nbuckets);
+ log2_nbuckets = pg_ceil_log2_64_bound(nbuckets);
Assert(nbuckets == (1 << log2_nbuckets));
/*
@@ -3499,7 +3498,7 @@ ExecParallelHashTableSetCurrentBatch(HashJoinTable hashtable, int batchno)
dsa_get_address(hashtable->area,
hashtable->batches[batchno].shared->buckets);
hashtable->nbuckets = hashtable->parallel_state->nbuckets;
- hashtable->log2_nbuckets = my_log2(hashtable->nbuckets);
+ hashtable->log2_nbuckets = pg_ceil_log2_64_bound(hashtable->nbuckets);
hashtable->current_chunk = NULL;
hashtable->current_chunk_shared = InvalidDsaPointer;
hashtable->batches[batchno].at_least_one_chunk = false;
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 22ad9051db3f..664db8096b26 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -268,7 +268,6 @@
#include "storage/procarray.h"
#include "tcop/tcopprot.h"
#include "utils/acl.h"
-#include "utils/dynahash.h"
#include "utils/guc.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
@@ -4911,7 +4910,7 @@ subxact_info_read(Oid subid, TransactionId xid)
len = sizeof(SubXactInfo) * subxact_data.nsubxacts;
/* we keep the maximum as a power of 2 */
- subxact_data.nsubxacts_max = 1 << my_log2(subxact_data.nsubxacts);
+ subxact_data.nsubxacts_max = 1 << pg_ceil_log2_64_bound(subxact_data.nsubxacts);
/*
* Allocate subxact information in the logical streaming context. We need
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index 1aeee5be42ac..81b0239eb69f 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -102,7 +102,6 @@
#include "port/pg_bitutils.h"
#include "storage/shmem.h"
#include "storage/spin.h"
-#include "utils/dynahash.h"
#include "utils/memutils.h"
@@ -281,8 +280,6 @@ static bool init_htab(HTAB *hashp, int64 nelem);
pg_noreturn static void hash_corrupted(HTAB *hashp);
static uint32 hash_initial_lookup(HTAB *hashp, uint32 hashvalue,
HASHBUCKET **bucketptr);
-static int64 next_pow2_int64(int64 num);
-static int next_pow2_int(int64 num);
static void register_seq_scan(HTAB *hashp);
static void deregister_seq_scan(HTAB *hashp);
static bool has_seq_scans(HTAB *hashp);
@@ -539,7 +536,7 @@ hash_create(const char *tabname, int64 nelem, const HASHCTL *info, int flags)
* be less than INT_MAX (see init_htab()), so call the int version of
* next_pow2.
*/
- Assert(info->num_partitions == next_pow2_int(info->num_partitions));
+ Assert(info->num_partitions == pg_nextpower2_32_bound(info->num_partitions));
hctl->num_partitions = info->num_partitions;
}
@@ -547,7 +544,7 @@ hash_create(const char *tabname, int64 nelem, const HASHCTL *info, int flags)
if (flags & HASH_SEGMENT)
{
hctl->ssize = info->ssize;
- hctl->sshift = my_log2(info->ssize);
+ hctl->sshift = pg_ceil_log2_64_bound(info->ssize);
/* ssize had better be a power of 2 */
Assert(hctl->ssize == (1L << hctl->sshift));
}
@@ -716,7 +713,7 @@ init_htab(HTAB *hashp, int64 nelem)
* Allocate space for the next greater power of two number of buckets,
* assuming a desired maximum load factor of 1.
*/
- nbuckets = next_pow2_int(nelem);
+ nbuckets = pg_nextpower2_32_bound(nelem);
/*
* In a partitioned table, nbuckets must be at least equal to
@@ -734,7 +731,7 @@ init_htab(HTAB *hashp, int64 nelem)
* Figure number of directory segments needed, round up to a power of 2
*/
nsegs = (nbuckets - 1) / hctl->ssize + 1;
- nsegs = next_pow2_int(nsegs);
+ nsegs = pg_nextpower2_32_bound(nsegs);
/*
* Make sure directory is big enough. If pre-allocated directory is too
@@ -791,9 +788,9 @@ hash_estimate_size(int64 num_entries, Size entrysize)
elementAllocCnt;
/* estimate number of buckets wanted */
- nBuckets = next_pow2_int64(num_entries);
+ nBuckets = pg_nextpower2_64_bound(num_entries);
/* # of segments needed for nBuckets */
- nSegments = next_pow2_int64((nBuckets - 1) / DEF_SEGSIZE + 1);
+ nSegments = pg_nextpower2_64_bound((nBuckets - 1) / DEF_SEGSIZE + 1);
/* directory entries */
nDirEntries = DEF_DIRSIZE;
while (nDirEntries < nSegments)
@@ -834,9 +831,9 @@ hash_select_dirsize(int64 num_entries)
nDirEntries;
/* estimate number of buckets wanted */
- nBuckets = next_pow2_int64(num_entries);
+ nBuckets = pg_nextpower2_64_bound(num_entries);
/* # of segments needed for nBuckets */
- nSegments = next_pow2_int64((nBuckets - 1) / DEF_SEGSIZE + 1);
+ nSegments = pg_nextpower2_64_bound((nBuckets - 1) / DEF_SEGSIZE + 1);
/* directory entries */
nDirEntries = DEF_DIRSIZE;
while (nDirEntries < nSegments)
@@ -1812,37 +1809,6 @@ hash_corrupted(HTAB *hashp)
elog(FATAL, "hash table \"%s\" corrupted", hashp->tabname);
}
-/* calculate ceil(log base 2) of num */
-int
-my_log2(int64 num)
-{
- /*
- * guard against too-large input, which would be invalid for
- * pg_ceil_log2_*()
- */
- if (num > PG_INT64_MAX / 2)
- num = PG_INT64_MAX / 2;
-
- return pg_ceil_log2_64(num);
-}
-
-/* calculate first power of 2 >= num, bounded to what will fit in a int64 */
-static int64
-next_pow2_int64(int64 num)
-{
- /* my_log2's internal range check is sufficient */
- return 1L << my_log2(num);
-}
-
-/* calculate first power of 2 >= num, bounded to what will fit in an int */
-static int
-next_pow2_int(int64 num)
-{
- if (num > INT_MAX / 2)
- num = INT_MAX / 2;
- return 1 << my_log2(num);
-}
-
/************************* SEQ SCAN TRACKING ************************/
--
2.50.1
On 22.08.25 07:09, Michael Paquier wrote:
An extra thing is a suggested change for pg_nextpower2_32(), to use a
uint64 instead of a uint32 as argument, which is caused by
next_pow2_int64() and next_pow2_int(), that both used int64
previously.
That seems highly confusing. What is the meaning of the "32" then?
If you need 64-bit behavior, use the variant with "64" in the name.
On Wed, Aug 27, 2025 at 10:00:16AM +0200, Peter Eisentraut wrote:
That seems highly confusing. What is the meaning of the "32" then?
If you need 64-bit behavior, use the variant with "64" in the name.
static int
next_pow2_int(int64 num)
{
if (num > INT_MAX / 2)
num = INT_MAX / 2;
return 1 << my_log2(num);
}
The pain point for me is the assumption of this routine on HEAD and
older branches, leading to a more protective overflow pattern for the
number of partitions calculated. I don't see an elegant way to keep
the same calculations for the "next power" routines while making the
int32 flavor more compliant with the fact that it may have a int64
argument (long previously), because it would mean that we would
underestimate the number returned here each time "num" is higher than
(INT_MAX / 2). That's quite dangerous when applied to dynahash.c,
which is a layer that extensions like. That would lead to doubling
the number of "next power" routines in pg_bitutils.h, which is not
cool in the long-term because it would facilitate incorrect uses.
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?
--
Michael
Attachments:
v2-0001-Clean-up-my_log2-in-dynahash.c-adding-equivalents.patchtext/x-diff; charset=us-asciiDownload
From c1f1dd163f671185bc0fba86acdd0b108007b711 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Mon, 1 Sep 2025 12:23:44 +0900
Subject: [PATCH v2] Clean up my_log2() in dynahash.c, adding equivalents to
bitutils.h
---
src/include/port/pg_bitutils.h | 34 ++++++++++++++++++++++++
src/include/utils/dynahash.h | 20 --------------
src/backend/executor/nodeAgg.c | 3 +--
src/backend/executor/nodeHash.c | 7 +++--
src/backend/replication/logical/worker.c | 3 +--
src/backend/utils/hash/dynahash.c | 23 +++-------------
6 files changed, 43 insertions(+), 47 deletions(-)
delete mode 100644 src/include/utils/dynahash.h
diff --git a/src/include/port/pg_bitutils.h b/src/include/port/pg_bitutils.h
index c7901bf8ddc0..5354d152a116 100644
--- a/src/include/port/pg_bitutils.h
+++ b/src/include/port/pg_bitutils.h
@@ -276,6 +276,40 @@ pg_ceil_log2_64(uint64 num)
return pg_leftmost_one_pos64(num - 1) + 1;
}
+/*
+ * 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;
+}
+
+/*
+ * pg_ceil_log2_64_bound
+ * Returns equivalent of ceil(log2(num)), with overflow safeguard
+ * for pg_leftmost_one_pos64.
+ */
+static inline uint64
+pg_ceil_log2_64_bound(uint64 num)
+{
+ if (num > PG_INT64_MAX / 2)
+ num = PG_INT64_MAX / 2;
+
+ if (num < 2)
+ return 0;
+ else
+ return pg_leftmost_one_pos64(num - 1) + 1;
+}
+
/*
* With MSVC on x86_64 builds, try using native popcnt instructions via the
* __popcnt and __popcnt64 intrinsics. These don't work the same as GCC's
diff --git a/src/include/utils/dynahash.h b/src/include/utils/dynahash.h
deleted file mode 100644
index a4362d3f65e5..000000000000
--- a/src/include/utils/dynahash.h
+++ /dev/null
@@ -1,20 +0,0 @@
-/*-------------------------------------------------------------------------
- *
- * dynahash.h
- * POSTGRES dynahash.h file definitions
- *
- *
- * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
- * Portions Copyright (c) 1994, Regents of the University of California
- *
- * IDENTIFICATION
- * src/include/utils/dynahash.h
- *
- *-------------------------------------------------------------------------
- */
-#ifndef DYNAHASH_H
-#define DYNAHASH_H
-
-extern int my_log2(int64 num);
-
-#endif /* DYNAHASH_H */
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 377e016d7322..b5fa9af9d992 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -267,7 +267,6 @@
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/datum.h"
-#include "utils/dynahash.h"
#include "utils/expandeddatum.h"
#include "utils/injection_point.h"
#include "utils/logtape.h"
@@ -2115,7 +2114,7 @@ hash_choose_num_partitions(double input_groups, double hashentrysize,
npartitions = (int) dpartitions;
/* ceil(log2(npartitions)) */
- partition_bits = my_log2(npartitions);
+ partition_bits = pg_ceil_log2_64_bound(npartitions);
/* make sure that we don't exhaust the hash bits */
if (partition_bits + used_bits >= 32)
diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c
index 8d2201ab67fa..14d934ab42b2 100644
--- a/src/backend/executor/nodeHash.c
+++ b/src/backend/executor/nodeHash.c
@@ -36,7 +36,6 @@
#include "executor/nodeHashjoin.h"
#include "miscadmin.h"
#include "port/pg_bitutils.h"
-#include "utils/dynahash.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/syscache.h"
@@ -340,7 +339,7 @@ MultiExecParallelHash(HashState *node)
*/
hashtable->curbatch = -1;
hashtable->nbuckets = pstate->nbuckets;
- hashtable->log2_nbuckets = my_log2(hashtable->nbuckets);
+ hashtable->log2_nbuckets = pg_ceil_log2_64_bound(hashtable->nbuckets);
hashtable->totalTuples = pstate->total_tuples;
/*
@@ -480,7 +479,7 @@ ExecHashTableCreate(HashState *state)
&nbuckets, &nbatch, &num_skew_mcvs);
/* nbuckets must be a power of 2 */
- log2_nbuckets = my_log2(nbuckets);
+ log2_nbuckets = pg_ceil_log2_64_bound(nbuckets);
Assert(nbuckets == (1 << log2_nbuckets));
/*
@@ -3499,7 +3498,7 @@ ExecParallelHashTableSetCurrentBatch(HashJoinTable hashtable, int batchno)
dsa_get_address(hashtable->area,
hashtable->batches[batchno].shared->buckets);
hashtable->nbuckets = hashtable->parallel_state->nbuckets;
- hashtable->log2_nbuckets = my_log2(hashtable->nbuckets);
+ hashtable->log2_nbuckets = pg_ceil_log2_64_bound(hashtable->nbuckets);
hashtable->current_chunk = NULL;
hashtable->current_chunk_shared = InvalidDsaPointer;
hashtable->batches[batchno].at_least_one_chunk = false;
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 22ad9051db3f..664db8096b26 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -268,7 +268,6 @@
#include "storage/procarray.h"
#include "tcop/tcopprot.h"
#include "utils/acl.h"
-#include "utils/dynahash.h"
#include "utils/guc.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
@@ -4911,7 +4910,7 @@ subxact_info_read(Oid subid, TransactionId xid)
len = sizeof(SubXactInfo) * subxact_data.nsubxacts;
/* we keep the maximum as a power of 2 */
- subxact_data.nsubxacts_max = 1 << my_log2(subxact_data.nsubxacts);
+ subxact_data.nsubxacts_max = 1 << pg_ceil_log2_64_bound(subxact_data.nsubxacts);
/*
* Allocate subxact information in the logical streaming context. We need
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index 1aeee5be42ac..6a86572ee61d 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -102,7 +102,6 @@
#include "port/pg_bitutils.h"
#include "storage/shmem.h"
#include "storage/spin.h"
-#include "utils/dynahash.h"
#include "utils/memutils.h"
@@ -547,7 +546,7 @@ hash_create(const char *tabname, int64 nelem, const HASHCTL *info, int flags)
if (flags & HASH_SEGMENT)
{
hctl->ssize = info->ssize;
- hctl->sshift = my_log2(info->ssize);
+ hctl->sshift = pg_ceil_log2_64_bound(info->ssize);
/* ssize had better be a power of 2 */
Assert(hctl->ssize == (1L << hctl->sshift));
}
@@ -1812,26 +1811,12 @@ hash_corrupted(HTAB *hashp)
elog(FATAL, "hash table \"%s\" corrupted", hashp->tabname);
}
-/* calculate ceil(log base 2) of num */
-int
-my_log2(int64 num)
-{
- /*
- * guard against too-large input, which would be invalid for
- * pg_ceil_log2_*()
- */
- if (num > PG_INT64_MAX / 2)
- num = PG_INT64_MAX / 2;
-
- return pg_ceil_log2_64(num);
-}
-
/* calculate first power of 2 >= num, bounded to what will fit in a int64 */
static int64
next_pow2_int64(int64 num)
{
- /* my_log2's internal range check is sufficient */
- return 1L << my_log2(num);
+ /* pg_ceil_log2_64_bound's internal range check is sufficient */
+ return 1L << pg_ceil_log2_64_bound(num);
}
/* calculate first power of 2 >= num, bounded to what will fit in an int */
@@ -1840,7 +1825,7 @@ next_pow2_int(int64 num)
{
if (num > INT_MAX / 2)
num = INT_MAX / 2;
- return 1 << my_log2(num);
+ return 1 << pg_ceil_log2_32_bound(num);
}
--
2.51.0
On 01.09.25 05:25, Michael Paquier 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.
pg_bitutils.h is aligned with standard compiler intrinsics and in the
long term C23 <stdbit.h>, so we shouldn't add our own custom stuff in
there without considering that bigger picture.
I would agree with what I think you're saying, we can keep these custom
variants with a particular error-checking behavior local to dynahash.c.
Maybe a comment or two to explain this more clearly would be good.
Taking a look at your previous patch with the changes from long to
int64, I think there is something that still doesn't fit.
For example, taking a look at the callers of hash_estimate_size(int64,
Size), they pass either int as the first argument, or in a few cases
long. Looking around inside dynahash.c, do any of these places actually
need the int64 range? These are all just counters, the memory sizes use
Size correctly it seems. Do we want to support more than INT_MAX
elements? I wonder whether the right solution would be to turn the long
uses into int instead. Then you also don't need to deal with two
next_pow2* variants.
On Wed, Sep 03, 2025 at 02:48:40PM +0200, Peter Eisentraut wrote:
Taking a look at your previous patch with the changes from long to int64, I
think there is something that still doesn't fit.For example, taking a look at the callers of hash_estimate_size(int64,
Size), they pass either int as the first argument, or in a few cases long.
Looking around inside dynahash.c, do any of these places actually need the
int64 range? These are all just counters, the memory sizes use Size
correctly it seems. Do we want to support more than INT_MAX elements? I
wonder whether the right solution would be to turn the long uses into int
instead. Then you also don't need to deal with two next_pow2* variants.
In terms of in-core callers, I am not worried. The highest cap that
can be reached would be I think PGSS, and we are capped by the
pgss_max GUC at (INT_MAX / 2).
hash_create() is too generic to offer hints at Postgres-related uses
outside of core. hash_get_num_entries() and hash_select_dirsize()
offer a couple of more dedicated hints, but everything I am seeing
seems to point out to the argument that the tables are capped due to
integer GUCs being 4 bytes, or use some hardcoded values which are
much lower than the 2^32 limit, like some stuff in this repo:
https://github.com/jithinjose2004/postgres_cluster/
So I could agree with your argument of dropping the 8-byte part
altogether argument and restrict all the interfaces to 4 bytes. Any
comments or opinions from others in favor or against taking this Leap
of Faith?
--
Michael
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
On Fri, Sep 05, 2025 at 10:40:46AM +0100, Dean Rasheed wrote:
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.
Yep, that may be the best course of action. As far as I can see, this
is capped by palloc() and HashJoinTuple, so we should be OK with
putting a hard limit at (INT_MAX / 2) and call it a day, I guess?
The two other call sites of my_log2() are worker.c, for the number of
subxacts, which relies on int32. The other call site is nodeAgg.c,
capped at HASHAGG_MAX_PARTITIONS (1024).
As of the attached, dynahash.h can be removed, which is the minimal
goal I had in mind. I am not sure about the need to tweak more
dynahash.c, as we've relied on long in this file for many years. We
could bite the bullet and do it, of course, but I am not sure.. So
I would be happy with only the attached changes.
What do you think?
--
Michael
Attachments:
v3-0001-Replace-callers-of-dynahash.h-s-my_log-by-equival.patchtext/x-diff; charset=us-asciiDownload
From 5c81cf6b035368ac2dc9c4b20881a9e13eff5360 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 9 Sep 2025 12:40:04 +0900
Subject: [PATCH v3 1/2] Replace callers of dynahash.h's my_log() by equivalent
in pg_bitutils.h
All these callers use 4-byte signed integers for their variables, hence
they do not require my_log2() maximum based on int64. This eases an
upcoming removal of my_log2().
ExecChooseHashTableSize(), that calculates the number of buckets to use
for hash tables, is now capped at 2^30, to keep pg_ceil_log2_32() on the
same side. nodeAgg.c is already capped at 1024, and worker.c relies on
the maximum number of subxacts.
---
src/backend/executor/nodeAgg.c | 3 +--
src/backend/executor/nodeHash.c | 14 ++++++++++----
src/backend/replication/logical/worker.c | 3 +--
3 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 377e016d7322..a4f3d30f307c 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -267,7 +267,6 @@
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/datum.h"
-#include "utils/dynahash.h"
#include "utils/expandeddatum.h"
#include "utils/injection_point.h"
#include "utils/logtape.h"
@@ -2115,7 +2114,7 @@ hash_choose_num_partitions(double input_groups, double hashentrysize,
npartitions = (int) dpartitions;
/* ceil(log2(npartitions)) */
- partition_bits = my_log2(npartitions);
+ partition_bits = pg_ceil_log2_32(npartitions);
/* make sure that we don't exhaust the hash bits */
if (partition_bits + used_bits >= 32)
diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c
index 8d2201ab67fa..db8f4722ebb2 100644
--- a/src/backend/executor/nodeHash.c
+++ b/src/backend/executor/nodeHash.c
@@ -36,7 +36,6 @@
#include "executor/nodeHashjoin.h"
#include "miscadmin.h"
#include "port/pg_bitutils.h"
-#include "utils/dynahash.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/syscache.h"
@@ -340,7 +339,7 @@ MultiExecParallelHash(HashState *node)
*/
hashtable->curbatch = -1;
hashtable->nbuckets = pstate->nbuckets;
- hashtable->log2_nbuckets = my_log2(hashtable->nbuckets);
+ hashtable->log2_nbuckets = pg_ceil_log2_32(hashtable->nbuckets);
hashtable->totalTuples = pstate->total_tuples;
/*
@@ -480,7 +479,7 @@ ExecHashTableCreate(HashState *state)
&nbuckets, &nbatch, &num_skew_mcvs);
/* nbuckets must be a power of 2 */
- log2_nbuckets = my_log2(nbuckets);
+ log2_nbuckets = pg_ceil_log2_32(nbuckets);
Assert(nbuckets == (1 << log2_nbuckets));
/*
@@ -935,6 +934,13 @@ ExecChooseHashTableSize(double ntuples, int tupwidth, bool useskew,
Assert(nbuckets > 0);
Assert(nbatch > 0);
+ /*
+ * Cap nbuckets, for power of 2 calculations. This maximum is safe
+ * for pg_ceil_log2_32().
+ */
+ if (nbuckets > PG_INT32_MAX / 2)
+ nbuckets = PG_INT32_MAX / 2;
+
*numbuckets = nbuckets;
*numbatches = nbatch;
}
@@ -3499,7 +3505,7 @@ ExecParallelHashTableSetCurrentBatch(HashJoinTable hashtable, int batchno)
dsa_get_address(hashtable->area,
hashtable->batches[batchno].shared->buckets);
hashtable->nbuckets = hashtable->parallel_state->nbuckets;
- hashtable->log2_nbuckets = my_log2(hashtable->nbuckets);
+ hashtable->log2_nbuckets = pg_ceil_log2_32(hashtable->nbuckets);
hashtable->current_chunk = NULL;
hashtable->current_chunk_shared = InvalidDsaPointer;
hashtable->batches[batchno].at_least_one_chunk = false;
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index c0f6bef5c282..c6d7c1fcde71 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -276,7 +276,6 @@
#include "storage/procarray.h"
#include "tcop/tcopprot.h"
#include "utils/acl.h"
-#include "utils/dynahash.h"
#include "utils/guc.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
@@ -5109,7 +5108,7 @@ subxact_info_read(Oid subid, TransactionId xid)
len = sizeof(SubXactInfo) * subxact_data.nsubxacts;
/* we keep the maximum as a power of 2 */
- subxact_data.nsubxacts_max = 1 << my_log2(subxact_data.nsubxacts);
+ subxact_data.nsubxacts_max = 1 << pg_ceil_log2_32(subxact_data.nsubxacts);
/*
* Allocate subxact information in the logical streaming context. We need
--
2.51.0
v3-0002-Remove-dynahash.h.patchtext/x-diff; charset=us-asciiDownload
From c6a492debc0d4c74c26f48db18aeeb6d4db8a088 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 9 Sep 2025 12:42:48 +0900
Subject: [PATCH v3 2/2] Remove dynahash.h
All the callers of my_log2() are now limited to dynahash.c, so let's
remove its header. This capability is provided by pg_bitutils.h
already.
---
src/include/utils/dynahash.h | 20 --------------------
src/backend/utils/hash/dynahash.c | 4 ++--
2 files changed, 2 insertions(+), 22 deletions(-)
delete mode 100644 src/include/utils/dynahash.h
diff --git a/src/include/utils/dynahash.h b/src/include/utils/dynahash.h
deleted file mode 100644
index a4362d3f65e5..000000000000
--- a/src/include/utils/dynahash.h
+++ /dev/null
@@ -1,20 +0,0 @@
-/*-------------------------------------------------------------------------
- *
- * dynahash.h
- * POSTGRES dynahash.h file definitions
- *
- *
- * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
- * Portions Copyright (c) 1994, Regents of the University of California
- *
- * IDENTIFICATION
- * src/include/utils/dynahash.h
- *
- *-------------------------------------------------------------------------
- */
-#ifndef DYNAHASH_H
-#define DYNAHASH_H
-
-extern int my_log2(int64 num);
-
-#endif /* DYNAHASH_H */
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index 1aeee5be42ac..ac94b9e93c6e 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -102,7 +102,6 @@
#include "port/pg_bitutils.h"
#include "storage/shmem.h"
#include "storage/spin.h"
-#include "utils/dynahash.h"
#include "utils/memutils.h"
@@ -281,6 +280,7 @@ static bool init_htab(HTAB *hashp, int64 nelem);
pg_noreturn static void hash_corrupted(HTAB *hashp);
static uint32 hash_initial_lookup(HTAB *hashp, uint32 hashvalue,
HASHBUCKET **bucketptr);
+static int my_log2(int64 num);
static int64 next_pow2_int64(int64 num);
static int next_pow2_int(int64 num);
static void register_seq_scan(HTAB *hashp);
@@ -1813,7 +1813,7 @@ hash_corrupted(HTAB *hashp)
}
/* calculate ceil(log base 2) of num */
-int
+static int
my_log2(int64 num)
{
/*
--
2.51.0
On Tue, 9 Sept 2025 at 04:58, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Sep 05, 2025 at 10:40:46AM +0100, Dean Rasheed wrote:
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.Yep, that may be the best course of action. As far as I can see, this
is capped by palloc() and HashJoinTuple, so we should be OK with
putting a hard limit at (INT_MAX / 2) and call it a day, I guess?
+1
In ExecChooseHashTableSize():
+ /*
+ * Cap nbuckets, for power of 2 calculations. This maximum is safe
+ * for pg_ceil_log2_32().
+ */
+ if (nbuckets > PG_INT32_MAX / 2)
+ nbuckets = PG_INT32_MAX / 2;That comment is not really right, because pg_ceil_log2_32() can accept
larger inputs than that. But in case, that cap is wrong because
nbuckets should always be a power of 2, and PG_INT32_MAX / 2 is 2^30 -
1.
Looking more closely, I don't think a cap is needed at all, given the
prior computations. Further up, there's this code:
/* Also ensure we avoid integer overflow in nbatch and nbuckets */
/* (this step is redundant given the current value of MaxAllocSize) */
max_pointers = Min(max_pointers, INT_MAX / 2 + 1);
and in practice, it's constrained to be much less than that, based on
this earlier code:
max_pointers = Min(max_pointers, MaxAllocSize / sizeof(HashJoinTuple));
So I think in theory that ensures that nbuckets can never get anywhere
near overflowing a 32-bit integer. Given that nbuckets is a 32-bit
signed integer, and it is a power of 2, it is automatically less than
or equal to 2^30, or else if somehow there was an error in the
preceding logic and an attempt had been made to make it larger than
that, integer wrap-around would have occurred (e.g., it might have
become -2^31), in which case the "Assert(nbuckets > 0)" would trap it.
So I think there's no point in adding that cap, or any additional
checks in ExecChooseHashTableSize().
Regards,
Dean
On Tue, Sep 09, 2025 at 10:28:13AM +0100, Dean Rasheed wrote:
So I think there's no point in adding that cap, or any additional
checks in ExecChooseHashTableSize().
You are right that this hardcoded limit introduced in the previous
patch was useless. So I have removed that, and applied the result.
Thanks for the suggestion.
Regarding removing the next power functions in dynahash.c, I am not
sure if it is worth bothering much. Now that dynahash.h is removed,
all the code duplication that was in the backend, without the
hardcoded thresholds, is removed.
--
Michael