Hi Peter, > > 0001: hash_freeze() has not been used since 5dfc198146 (Oct 2016). I > > suggest removing it. > > This seems alright. If someone needs it, we can put it back. > > > 0002: hash_destroy() is the only caller of hash_stats(). I suggest > > making the function static and place it under `#ifdef > > HASH_STATISTICS`. > > I don't think this is an improvement. In general, I would like to keep > the amount of code covered by #ifdef's as small as possible. That way, > we reduce the chances that some code rots because it is not compiled by > default. We could make the function static, but I would avoid the > additional #ifdef's.
Thanks for the feedback. Here are the corrected patches. -- Best regards, Aleksander Alekseev
From 2ca171cdb7fcb19938dd45096ceb373f47805258 Mon Sep 17 00:00:00 2001 From: Aleksander Alekseev <[email protected]> Date: Thu, 29 Oct 2026 13:26:11 +0300 Subject: [PATCH v2 1/2] Remove hash_freeze() hash_freeze() was added by a2e923a652. Its only caller was execGrouping.c, which stopped using dynahash in 5dfc198146. Nothing in the tree has called the function since, so remove it. Author: Aleksander Alekseev <[email protected]> Reviewed-by: Peter Eisentraut <[email protected]> Discussion: https://postgr.es/m/CAJ7c6TPGc1eXGp%2BtzjfVBZnSCmYkdMozKaGxDhVaw5xHdGaeUA%40mail.gmail.com --- src/backend/utils/hash/dynahash.c | 55 +++---------------------------- src/include/utils/hsearch.h | 1 - 2 files changed, 5 insertions(+), 51 deletions(-) diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c index 0db984a26c0..47d3cacc532 100644 --- a/src/backend/utils/hash/dynahash.c +++ b/src/backend/utils/hash/dynahash.c @@ -234,9 +234,6 @@ struct HTAB char *tabname; /* table name (for error messages) */ bool isshared; /* true if table is in shared memory */ - /* freezing a shared table isn't allowed, so we can keep state here */ - bool frozen; /* true = no more inserts allowed */ - /* We keep local copies of these fixed values to reduce contention */ Size keysize; /* hash key length in bytes */ @@ -534,8 +531,6 @@ hash_create(const char *tabname, int64 nelem, const HASHCTL *info, int flags) (errcode(ERRCODE_OUT_OF_MEMORY), errmsg("out of memory"))); - hashp->frozen = false; - hdefault(hashp); hctl = hashp->hctl; @@ -928,11 +923,11 @@ hash_search_with_hash_value(HTAB *hashp, if (action == HASH_ENTER || action == HASH_ENTER_NULL) { /* - * Can't split if running in partitioned mode, nor if frozen, nor if - * table is the subject of any active hash_seq_search scans. + * Can't split if running in partitioned mode, nor if table is the + * subject of any active hash_seq_search scans. */ if (hctl->freeList[0].nentries > (int64) hctl->max_bucket && - !hctl->isfixed && !IS_PARTITIONED(hctl) && !hashp->frozen && + !hctl->isfixed && !IS_PARTITIONED(hctl) && !has_seq_scans(hashp)) (void) expand_table(hashp); } @@ -1010,11 +1005,6 @@ hash_search_with_hash_value(HTAB *hashp, if (currBucket != NULL) return ELEMENTKEY(currBucket); - /* disallow inserts if frozen */ - if (hashp->frozen) - elog(ERROR, "cannot insert into frozen hashtable \"%s\"", - hashp->tabname); - currBucket = get_hash_entry(hashp, freelist_idx); if (currBucket == NULL) { @@ -1096,11 +1086,6 @@ hash_update_hash_key(HTAB *hashp, hctl->accesses++; #endif - /* disallow updates if frozen */ - if (hashp->frozen) - elog(ERROR, "cannot update in frozen hashtable \"%s\"", - hashp->tabname); - /* * Lookup the existing element using its saved hash value. We need to do * this to be able to unlink it from its hash chain, but as a side benefit @@ -1306,10 +1291,6 @@ hash_get_num_entries(HTAB *hashp) * if elements are added to the table while the scan is in progress, it is * unspecified whether they will be visited by the scan or not. * - * NOTE: it is possible to use hash_seq_init/hash_seq_search without any - * worry about hash_seq_term cleanup, if the hashtable is first locked against - * further insertions by calling hash_freeze. - * * NOTE: to use this with a partitioned hashtable, caller had better hold * at least shared lock on all partitions of the table throughout the scan! * We can cope with insertions or deletions by our own backend, but *not* @@ -1322,8 +1303,7 @@ hash_seq_init(HASH_SEQ_STATUS *status, HTAB *hashp) status->curBucket = 0; status->curEntry = NULL; status->hasHashvalue = false; - if (!hashp->frozen) - register_seq_scan(hashp); + register_seq_scan(hashp); } /* @@ -1445,32 +1425,7 @@ hash_seq_search(HASH_SEQ_STATUS *status) void hash_seq_term(HASH_SEQ_STATUS *status) { - if (!status->hashp->frozen) - deregister_seq_scan(status->hashp); -} - -/* - * hash_freeze - * Freeze a hashtable against future insertions (deletions are - * 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, - * and thus caller need not be careful about ensuring hash_seq_term gets - * called at the right times. - * - * Multiple calls to hash_freeze() are allowed, but you can't freeze a table - * with active scans (since hash_seq_term would then do the wrong thing). - */ -void -hash_freeze(HTAB *hashp) -{ - if (hashp->isshared) - elog(ERROR, "cannot freeze shared hashtable \"%s\"", hashp->tabname); - if (!hashp->frozen && has_seq_scans(hashp)) - elog(ERROR, "cannot freeze hashtable \"%s\" because it has active scans", - hashp->tabname); - hashp->frozen = true; + deregister_seq_scan(status->hashp); } diff --git a/src/include/utils/hsearch.h b/src/include/utils/hsearch.h index 896a22a2142..c238466c3eb 100644 --- a/src/include/utils/hsearch.h +++ b/src/include/utils/hsearch.h @@ -143,7 +143,6 @@ extern void hash_seq_init_with_hash_value(HASH_SEQ_STATUS *status, uint32 hashvalue); 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(int64 num_entries, Size entrysize); extern void AtEOXact_HashTables(bool isCommit); extern void AtEOSubXact_HashTables(bool isCommit, int nestDepth); -- 2.43.0
From 755faab8ec28445d3538cf995004797e0a3e36b8 Mon Sep 17 00:00:00 2001 From: Aleksander Alekseev <[email protected]> Date: Thu, 29 Oct 2026 13:31:32 +0300 Subject: [PATCH v2 2/2] Make hash_stats() static hash_stats() was declared in hsearch.h, but it is not really part of the dynahash API: hash_destroy() has been its only caller for a long time. Make the function static. Author: Aleksander Alekseev <[email protected]> Reviewed-by: Peter Eisentraut <[email protected]> Discussion: https://postgr.es/m/CAJ7c6TPGc1eXGp%2BtzjfVBZnSCmYkdMozKaGxDhVaw5xHdGaeUA%40mail.gmail.com --- src/backend/utils/hash/dynahash.c | 3 ++- src/include/utils/hsearch.h | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c index 47d3cacc532..90f142198a1 100644 --- a/src/backend/utils/hash/dynahash.c +++ b/src/backend/utils/hash/dynahash.c @@ -285,6 +285,7 @@ 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); +static void hash_stats(const char *caller, HTAB *hashp); /* @@ -812,7 +813,7 @@ hash_destroy(HTAB *hashp) } } -void +static void hash_stats(const char *caller, HTAB *hashp) { #ifdef HASH_STATISTICS diff --git a/src/include/utils/hsearch.h b/src/include/utils/hsearch.h index c238466c3eb..59180719b9d 100644 --- a/src/include/utils/hsearch.h +++ b/src/include/utils/hsearch.h @@ -127,7 +127,6 @@ typedef struct 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); extern void *hash_search(HTAB *hashp, const void *keyPtr, HASHACTION action, bool *foundPtr); extern uint32 get_hash_value(HTAB *hashp, const void *keyPtr); -- 2.43.0
