From 5d504b93f4880c7d1836304b7ad1a212bd5a5c58 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Thu, 27 Mar 2025 20:52:31 +0100 Subject: [PATCH v6 2/7] review --- src/backend/storage/ipc/shmem.c | 3 +- src/backend/utils/hash/dynahash.c | 127 ++++++++++++++++-------------- 2 files changed, 70 insertions(+), 60 deletions(-) diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c index d8aed0bfaaf..7e03a371d22 100644 --- a/src/backend/storage/ipc/shmem.c +++ b/src/backend/storage/ipc/shmem.c @@ -347,7 +347,8 @@ ShmemInitHash(const char *name, /* table string name for shmem index */ /* look it up in the shmem index */ location = ShmemInitStruct(name, - hash_get_init_size(infoP, hash_flags, init_size, 0), + hash_get_init_size(infoP, hash_flags, + init_size, 0), &found); /* diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c index 1f215a16c51..dcdc143e690 100644 --- a/src/backend/utils/hash/dynahash.c +++ b/src/backend/utils/hash/dynahash.c @@ -260,29 +260,35 @@ static long hash_accesses, hash_expansions; #endif - -#define HASH_ELEMENTS_OFFSET(hctl, nsegs) \ - (MAXALIGN(sizeof(HASHHDR)) + \ - ((hctl)->dsize * MAXALIGN(sizeof(HASHSEGMENT))) + \ - ((hctl)->ssize * (nsegs) * MAXALIGN(sizeof(HASHBUCKET)))) - -#define HASH_ELEMENTS(hashp, nsegs) \ - ((char *) (hashp)->hctl + HASH_ELEMENTS_OFFSET((hashp)->hctl, nsegs)) +#define HASH_DIRECTORY_PTR(hashp) \ + (((char *) (hashp)->hctl) + sizeof(HASHHDR)) #define HASH_SEGMENT_OFFSET(hctl, idx) \ - (MAXALIGN(sizeof(HASHHDR)) + \ - ((hctl)->dsize * MAXALIGN(sizeof(HASHSEGMENT))) + \ - ((hctl)->ssize * (idx) * MAXALIGN(sizeof(HASHBUCKET)))) + (sizeof(HASHHDR) + \ + ((hctl)->dsize * sizeof(HASHSEGMENT)) + \ + ((hctl)->ssize * (idx) * sizeof(HASHBUCKET))) #define HASH_SEGMENT_PTR(hashp, idx) \ - (HASHSEGMENT) ((char *) (hashp)->hctl + HASH_SEGMENT_OFFSET((hashp)->hctl, (idx))) + ((char *) (hashp)->hctl + HASH_SEGMENT_OFFSET((hashp)->hctl, (idx))) -#define HASH_SEGMENT_SIZE(hashp) ((hashp)->ssize * MAXALIGN(sizeof(HASHBUCKET))) +#define HASH_SEGMENT_SIZE(hashp) \ + ((hashp)->ssize * sizeof(HASHBUCKET)) + +/* XXX this is exactly the same as HASH_SEGMENT_OFFSET, unite? */ +#define HASH_ELEMENTS_OFFSET(hctl, nsegs) \ + (sizeof(HASHHDR) + \ + ((hctl)->dsize * sizeof(HASHSEGMENT)) + \ + ((hctl)->ssize * (nsegs) * sizeof(HASHBUCKET))) + +#define HASH_ELEMENTS_PTR(hashp, nsegs) \ + ((char *) (hashp)->hctl + HASH_ELEMENTS_OFFSET((hashp)->hctl, nsegs)) -#define HASH_DIRECTORY(hashp) (HASHSEGMENT *) (((char *) (hashp)->hctl) + MAXALIGN(sizeof(HASHHDR))) +/* Each element has a HASHELEMENT header plus user data. */ +#define HASH_ELEMENT_SIZE(hctl) \ + (MAXALIGN(sizeof(HASHELEMENT)) + MAXALIGN((hctl)->entrysize)) #define HASH_ELEMENT_NEXT(hctl, num, ptr) \ - ((char *) (ptr) + ((num) * (MAXALIGN(sizeof(HASHELEMENT)) + MAXALIGN((hctl)->entrysize)))) + ((char *) (ptr) + ((num) * HASH_ELEMENT_SIZE(hctl))) /* * Private function prototypes @@ -305,8 +311,8 @@ static void register_seq_scan(HTAB *hashp); static void deregister_seq_scan(HTAB *hashp); static bool has_seq_scans(HTAB *hashp); -static void compute_buckets_and_segs(long nelem, long num_partitions, - long ssize, /* segment size */ +static void compute_buckets_and_segs(long nelem, long num_partitions, + long ssize, int *nbuckets, int *nsegments); static void element_add(HTAB *hashp, HASHELEMENT *firstElement, int nelem, int freelist_idx); @@ -547,7 +553,7 @@ hash_create(const char *tabname, long nelem, const HASHCTL *info, int flags) */ if (!hashp->hctl) { - Size size = hash_get_init_size(info, flags, nelem, nelem_batch); + Size size = hash_get_init_size(info, flags, nelem, nelem_batch); hashp->hctl = (HASHHDR *) hashp->alloc(size); if (!hashp->hctl) @@ -638,16 +644,6 @@ hash_create(const char *tabname, long nelem, const HASHCTL *info, int flags) else freelist_partitions = 1; - compute_buckets_and_segs(nelem, hctl->num_partitions, hctl->ssize, - &nbuckets, &nsegs); - - /* - * Calculate the offset at which to find the first partition of - * elements. - * We have to skip space for the header, segments and buckets. - */ - ptr = HASH_ELEMENTS(hashp, nsegs); - nelem_alloc = nelem / freelist_partitions; if (nelem_alloc <= 0) nelem_alloc = 1; @@ -662,19 +658,29 @@ hash_create(const char *tabname, long nelem, const HASHCTL *info, int flags) else nelem_alloc_first = nelem_alloc; + /* + * Calculate the offset at which to find the first partition of + * elements. We have to skip space for the header, segments and + * buckets. + */ + compute_buckets_and_segs(nelem, hctl->num_partitions, hctl->ssize, + &nbuckets, &nsegs); + + ptr = HASH_ELEMENTS_PTR(hashp, nsegs); + + /* + * Assign the correct location of each parition within a pre-allocated + * buffer. + * + * Actual memory allocation happens in ShmemInitHash for shared hash + * tables or earlier in this function for non-shared hash tables. + * + * We just need to split that allocation into per-batch freelists. + */ for (i = 0; i < freelist_partitions; i++) { int temp = (i == 0) ? nelem_alloc_first : nelem_alloc; - /* - * Assign the correct location of each parition within a - * pre-allocated buffer. - * - * Actual memory allocation happens in ShmemInitHash for - * shared hash tables or earlier in this function for non-shared - * hash tables. - * We just need to split that allocation per-batch freelists. - */ element_add(hashp, (HASHELEMENT *) ptr, temp, i); ptr = HASH_ELEMENT_NEXT(hctl, temp, ptr); } @@ -789,14 +795,14 @@ init_htab(HTAB *hashp, long nelem) if (!(hashp->dir)) { CurrentDynaHashCxt = hashp->hcxt; - hashp->dir = HASH_DIRECTORY(hashp); + hashp->dir = (HASHSEGMENT *) HASH_DIRECTORY_PTR(hashp); } /* Assign initial segments, which are also pre-allocated */ i = 0; for (segp = hashp->dir; hctl->nsegs < nsegs; hctl->nsegs++, segp++) { - *segp = HASH_SEGMENT_PTR(hashp, i++); + *segp = (HASHSEGMENT) HASH_SEGMENT_PTR(hashp, i++); MemSet(*segp, 0, HASH_SEGMENT_SIZE(hashp)); } @@ -890,19 +896,22 @@ hash_select_dirsize(long num_entries) } /* - * Compute the required initial memory allocation for a shared-memory - * or non-shared memory hashtable with the given parameters. - * We need space for the HASHHDR, for the directory, segments and - * the init_size elements in buckets. - * + * Compute the required initial memory allocation for a hashtable with the given + * parameters. The hash table may be shared or private. We need space for the + * HASHHDR, for the directory, segments and the init_size elements in buckets. + * * For shared hash tables the directory size is non-expansive. - * - * init_size should match the total number of elements allocated - * during hash table creation, it could be zero for non-shared hash - * tables depending on the value of nelem_alloc. For more explanation - * see comments within this function. + * + * init_size should match the total number of elements allocated during hash + * table creation, it could be zero for non-shared hash tables depending on the + * value of nelem_alloc. For more explanation see comments within this function. * * nelem_alloc parameter is not relevant for shared hash tables. + * + * XXX I find this comment hard to understand / confusing. So what is init_size? + * Why should it match the total number of elements allocated during hash table + * creation? What does "it could be zero" say? Should it be zero, or why would I + * want to make it zero in some cases? */ Size hash_get_init_size(const HASHCTL *info, int flags, long init_size, int nelem_alloc) @@ -912,8 +921,8 @@ hash_get_init_size(const HASHCTL *info, int flags, long init_size, int nelem_all int num_partitions; long ssize; long dsize; - bool element_alloc = true; /*Always true for shared hash tables */ - Size elementSize = MAXALIGN(sizeof(HASHELEMENT)) + MAXALIGN(info->entrysize); + bool element_alloc = true; /* Always true for shared hash tables */ + Size elementSize = HASH_ELEMENT_SIZE(info); /* * For non-shared hash tables, the requested number of elements are @@ -931,6 +940,7 @@ hash_get_init_size(const HASHCTL *info, int flags, long init_size, int nelem_all Assert(flags & HASH_DIRSIZE); Assert(info->dsize == info->max_dsize); } + /* Non-shared hash tables may not specify dir size */ if (!(flags & HASH_DIRSIZE)) { @@ -961,8 +971,8 @@ hash_get_init_size(const HASHCTL *info, int flags, long init_size, int nelem_all if (!element_alloc) init_size = 0; - return MAXALIGN(sizeof(HASHHDR)) + dsize * MAXALIGN(sizeof(HASHSEGMENT)) + - + MAXALIGN(sizeof(HASHBUCKET)) * ssize * nsegs + return sizeof(HASHHDR) + dsize * sizeof(HASHSEGMENT) + + sizeof(HASHBUCKET) * ssize * nsegs + init_size * elementSize; } @@ -1393,8 +1403,7 @@ get_hash_entry(HTAB *hashp, int freelist_idx) * Failing because the needed element is in a different freelist is * not acceptable. */ - newElement = element_alloc(hashp, hctl->nelem_alloc); - if (newElement == NULL) + if ((newElement = element_alloc(hashp, hctl->nelem_alloc)) == NULL) { int borrow_from_idx; @@ -1823,7 +1832,7 @@ element_alloc(HTAB *hashp, int nelem) return NULL; /* Each element has a HASHELEMENT header plus user data. */ - elementSize = MAXALIGN(sizeof(HASHELEMENT)) + MAXALIGN(hctl->entrysize); + elementSize = HASH_ELEMENT_SIZE(hctl); CurrentDynaHashCxt = hashp->hcxt; firstElement = (HASHELEMENT *) hashp->alloc(nelem * elementSize); @@ -1834,7 +1843,7 @@ element_alloc(HTAB *hashp, int nelem) } /* - * Link the elements allocated by element_alloc into the indicated free list + * link the elements allocated by element_alloc into the indicated free list */ static void element_add(HTAB *hashp, HASHELEMENT *firstElement, int nelem, int freelist_idx) @@ -1846,7 +1855,8 @@ element_add(HTAB *hashp, HASHELEMENT *firstElement, int nelem, int freelist_idx) int i; /* Each element has a HASHELEMENT header plus user data. */ - elementSize = MAXALIGN(sizeof(HASHELEMENT)) + MAXALIGN(hctl->entrysize); + elementSize = HASH_ELEMENT_SIZE(hctl); + /* prepare to link all the new entries into the freelist */ prevElement = NULL; tmpElement = firstElement; @@ -2103,7 +2113,6 @@ compute_buckets_and_segs(long nelem, long num_partitions, long ssize, while ((*nbuckets) < num_partitions) (*nbuckets) <<= 1; - /* * Figure number of directory segments needed, round up to a power of 2 */ -- 2.49.0