While working on the dynamic shared memory registry, I noticed a couple of potential improvements for code that uses dshash tables.
* A couple of dshash_create() callers pass in 0 for the "void *arg" parameter, which seemed weird. I incorrectly assumed this was some sort of flags parameter until I actually looked at the function signature. IMHO we should specify NULL here if arg is not used. 0001 makes this change. This is admittedly a nitpick. * There are no dshash compare/hash functions for string keys. 0002 introduces some that simply forward to strcmp()/string_hash(), and it uses them for the DSM registry's dshash table. This allows us to remove the hacky key padding code for lookups on this table. Thoughts? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
>From 5fe6d05f2cfa1401c2fb967fe4bb52cae3706228 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nat...@postgresql.org> Date: Fri, 19 Jan 2024 15:23:35 -0600 Subject: [PATCH v1 1/2] Use NULL instead of 0 for arg parameter in dshash_create(). --- src/backend/replication/logical/launcher.c | 2 +- src/backend/utils/activity/pgstat_shmem.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c index 122db0bb13..ee66c292bd 100644 --- a/src/backend/replication/logical/launcher.c +++ b/src/backend/replication/logical/launcher.c @@ -1013,7 +1013,7 @@ logicalrep_launcher_attach_dshmem(void) last_start_times_dsa = dsa_create(LWTRANCHE_LAUNCHER_DSA); dsa_pin(last_start_times_dsa); dsa_pin_mapping(last_start_times_dsa); - last_start_times = dshash_create(last_start_times_dsa, &dsh_params, 0); + last_start_times = dshash_create(last_start_times_dsa, &dsh_params, NULL); /* Store handles in shared memory for other backends to use. */ LogicalRepCtx->last_start_dsa = dsa_get_handle(last_start_times_dsa); diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c index 3ce48e88a3..71410ddd3f 100644 --- a/src/backend/utils/activity/pgstat_shmem.c +++ b/src/backend/utils/activity/pgstat_shmem.c @@ -180,7 +180,7 @@ StatsShmemInit(void) * With the limit in place, create the dshash table. XXX: It'd be nice * if there were dshash_create_in_place(). */ - dsh = dshash_create(dsa, &dsh_params, 0); + dsh = dshash_create(dsa, &dsh_params, NULL); ctl->hash_handle = dshash_get_hash_table_handle(dsh); /* lift limit set above */ -- 2.25.1
>From 6c6760e6553ac86c334e7c35d2ddbac8fdc1cb9b Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nat...@postgresql.org> Date: Fri, 19 Jan 2024 15:38:34 -0600 Subject: [PATCH v1 2/2] Add hash/compare functions for dshash tables with string keys. --- src/backend/lib/dshash.c | 23 +++++++++++++++++++++++ src/backend/storage/ipc/dsm_registry.c | 8 +++----- src/include/lib/dshash.h | 4 ++++ 3 files changed, 30 insertions(+), 5 deletions(-) diff --git a/src/backend/lib/dshash.c b/src/backend/lib/dshash.c index b0bc0abda0..e497238e8f 100644 --- a/src/backend/lib/dshash.c +++ b/src/backend/lib/dshash.c @@ -583,6 +583,29 @@ dshash_memhash(const void *v, size_t size, void *arg) return tag_hash(v, size); } +/* + * A compare function that forwards to strcmp. + */ +int +dshash_strcmp(const void *a, const void *b, size_t size, void *arg) +{ + Assert(strlen((const char *) a) < size); + Assert(strlen((const char *) b) < size); + + return strcmp((const char *) a, (const char *) b); +} + +/* + * A hash function that forwards to string_hash. + */ +dshash_hash +dshash_strhash(const void *v, size_t size, void *arg) +{ + Assert(strlen((const char *) v) < size); + + return string_hash((const char *) v, size); +} + /* * Sequentially scan through dshash table and return all the elements one by * one, return NULL when all elements have been returned. diff --git a/src/backend/storage/ipc/dsm_registry.c b/src/backend/storage/ipc/dsm_registry.c index ac11f51375..9dce41e8ab 100644 --- a/src/backend/storage/ipc/dsm_registry.c +++ b/src/backend/storage/ipc/dsm_registry.c @@ -50,8 +50,8 @@ typedef struct DSMRegistryEntry static const dshash_parameters dsh_params = { offsetof(DSMRegistryEntry, handle), sizeof(DSMRegistryEntry), - dshash_memcmp, - dshash_memhash, + dshash_strcmp, + dshash_strhash, LWTRANCHE_DSM_REGISTRY_HASH }; @@ -132,7 +132,6 @@ GetNamedDSMSegment(const char *name, size_t size, { DSMRegistryEntry *entry; MemoryContext oldcontext; - char name_padded[offsetof(DSMRegistryEntry, handle)] = {0}; void *ret; Assert(found); @@ -155,8 +154,7 @@ GetNamedDSMSegment(const char *name, size_t size, /* Connect to the registry. */ init_dsm_registry(); - strcpy(name_padded, name); - entry = dshash_find_or_insert(dsm_registry_table, name_padded, found); + entry = dshash_find_or_insert(dsm_registry_table, name, found); if (!(*found)) { /* Initialize the segment. */ diff --git a/src/include/lib/dshash.h b/src/include/lib/dshash.h index 91f70ac2b5..1861e5a8b1 100644 --- a/src/include/lib/dshash.h +++ b/src/include/lib/dshash.h @@ -109,6 +109,10 @@ extern void dshash_delete_current(dshash_seq_status *status); extern int dshash_memcmp(const void *a, const void *b, size_t size, void *arg); extern dshash_hash dshash_memhash(const void *v, size_t size, void *arg); +/* Convenience hash and compare functions wrapping strcmp and string_hash. */ +extern int dshash_strcmp(const void *a, const void *b, size_t size, void *arg); +extern dshash_hash dshash_strhash(const void *v, size_t size, void *arg); + /* Debugging support. */ extern void dshash_dump(dshash_table *hash_table); -- 2.25.1