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
