On Sun, Jan 21, 2024 at 09:51:18PM -0600, Nathan Bossart wrote:
> I did notice a cfbot failure [0]. After a quick glance, I'm assuming this
> is caused by the memcpy() in insert_into_bucket(). Even if the string is
> short, it will copy the maximum key size, which is bad. So, 0002 is
> totally broken at the moment, and we'd need to teach insert_into_bucket()
> to strcpy() instead for string keys to fix it. Perhaps we could add a
> field to dshash_parameters for this purpose...
I attempted to fix this in v2 of the patch set.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From af203e6e80f7a2843890e51fd3e3641aef8bd901 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Fri, 19 Jan 2024 15:23:35 -0600
Subject: [PATCH v2 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 1d328f0be681da7e43574350a1d3c8da8ae8ddcc Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Fri, 19 Jan 2024 15:38:34 -0600
Subject: [PATCH v2 2/2] Add hash/compare functions for dshash tables with
string keys.
---
src/backend/lib/dshash.c | 58 +++++++++++++++++++++-
src/backend/replication/logical/launcher.c | 1 +
src/backend/storage/ipc/dsm_registry.c | 9 ++--
src/backend/utils/activity/pgstat_shmem.c | 1 +
src/backend/utils/cache/typcache.c | 2 +
src/include/lib/dshash.h | 19 ++++++-
6 files changed, 83 insertions(+), 7 deletions(-)
diff --git a/src/backend/lib/dshash.c b/src/backend/lib/dshash.c
index b0bc0abda0..cc49b4ca51 100644
--- a/src/backend/lib/dshash.c
+++ b/src/backend/lib/dshash.c
@@ -188,6 +188,8 @@ static bool delete_item_from_bucket(dshash_table *hash_table,
static inline dshash_hash hash_key(dshash_table *hash_table, const void *key);
static inline bool equal_keys(dshash_table *hash_table,
const void *a, const void *b);
+static inline void copy_key(dshash_table *hash_table, void *dest,
+ const void *src);
#define PARTITION_LOCK(hash_table, i) \
(&(hash_table)->control->partitions[(i)].lock)
@@ -583,6 +585,49 @@ dshash_memhash(const void *v, size_t size, void *arg)
return tag_hash(v, size);
}
+/*
+ * A copy function that forwards to memcpy.
+ */
+void
+dshash_memcpy(void *dest, const void *src, size_t size, void *arg)
+{
+ (void) memcpy(dest, src, 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);
+}
+
+/*
+ * A copy function that forwards to strcpy.
+ */
+void
+dshash_strcpy(void *dest, const void *src, size_t size, void *arg)
+{
+ Assert(strlen((const char *) src) < size);
+
+ (void) strcpy((char *) dest, (const char *) src);
+}
+
/*
* Sequentially scan through dshash table and return all the elements one by
* one, return NULL when all elements have been returned.
@@ -949,7 +994,7 @@ insert_into_bucket(dshash_table *hash_table,
hash_table->params.entry_size +
MAXALIGN(sizeof(dshash_table_item)));
item = dsa_get_address(hash_table->area, item_pointer);
- memcpy(ENTRY_FROM_ITEM(item), key, hash_table->params.key_size);
+ copy_key(hash_table, ENTRY_FROM_ITEM(item), key);
insert_item_into_bucket(hash_table, item_pointer, item, bucket);
return item;
}
@@ -1032,3 +1077,14 @@ equal_keys(dshash_table *hash_table, const void *a, const void *b)
hash_table->params.key_size,
hash_table->arg) == 0;
}
+
+/*
+ * Copy a key.
+ */
+static inline void
+copy_key(dshash_table *hash_table, void *dest, const void *src)
+{
+ hash_table->params.copy_function(dest, src,
+ hash_table->params.key_size,
+ hash_table->arg);
+}
diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
index ee66c292bd..487f141a59 100644
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -88,6 +88,7 @@ static const dshash_parameters dsh_params = {
sizeof(LauncherLastStartTimesEntry),
dshash_memcmp,
dshash_memhash,
+ dshash_memcpy,
LWTRANCHE_LAUNCHER_HASH
};
diff --git a/src/backend/storage/ipc/dsm_registry.c b/src/backend/storage/ipc/dsm_registry.c
index ac11f51375..e49edf52b4 100644
--- a/src/backend/storage/ipc/dsm_registry.c
+++ b/src/backend/storage/ipc/dsm_registry.c
@@ -50,8 +50,9 @@ typedef struct DSMRegistryEntry
static const dshash_parameters dsh_params = {
offsetof(DSMRegistryEntry, handle),
sizeof(DSMRegistryEntry),
- dshash_memcmp,
- dshash_memhash,
+ dshash_strcmp,
+ dshash_strhash,
+ dshash_strcpy,
LWTRANCHE_DSM_REGISTRY_HASH
};
@@ -132,7 +133,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 +155,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/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c
index 71410ddd3f..91591da395 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -64,6 +64,7 @@ static const dshash_parameters dsh_params = {
sizeof(PgStatShared_HashEntry),
pgstat_cmp_hash_key,
pgstat_hash_hash_key,
+ dshash_memcpy,
LWTRANCHE_PGSTATS_HASH
};
diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c
index 84fc83cb11..bd7bd798f0 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -258,6 +258,7 @@ static const dshash_parameters srtr_record_table_params = {
sizeof(SharedRecordTableEntry),
shared_record_table_compare,
shared_record_table_hash,
+ dshash_memcpy,
LWTRANCHE_PER_SESSION_RECORD_TYPE
};
@@ -267,6 +268,7 @@ static const dshash_parameters srtr_typmod_table_params = {
sizeof(SharedTypmodTableEntry),
dshash_memcmp,
dshash_memhash,
+ dshash_memcpy,
LWTRANCHE_PER_SESSION_RECORD_TYPMOD
};
diff --git a/src/include/lib/dshash.h b/src/include/lib/dshash.h
index 91f70ac2b5..2ff1ba6c24 100644
--- a/src/include/lib/dshash.h
+++ b/src/include/lib/dshash.h
@@ -37,6 +37,10 @@ typedef int (*dshash_compare_function) (const void *a, const void *b,
typedef dshash_hash (*dshash_hash_function) (const void *v, size_t size,
void *arg);
+/* A function type for copying keys. */
+typedef void (*dshash_copy_function) (void *dest, const void *src, size_t size,
+ void *arg);
+
/*
* The set of parameters needed to create or attach to a hash table. The
* members tranche_id and tranche_name do not need to be initialized when
@@ -55,6 +59,7 @@ typedef struct dshash_parameters
size_t entry_size; /* Total size of entry */
dshash_compare_function compare_function; /* Compare function */
dshash_hash_function hash_function; /* Hash function */
+ dshash_copy_function copy_function; /* Copy function */
int tranche_id; /* The tranche ID to use for locks */
} dshash_parameters;
@@ -105,9 +110,21 @@ extern void *dshash_seq_next(dshash_seq_status *status);
extern void dshash_seq_term(dshash_seq_status *status);
extern void dshash_delete_current(dshash_seq_status *status);
-/* Convenience hash and compare functions wrapping memcmp and tag_hash. */
+/*
+ * Convenience hash, compare, and copy functions wrapping memcmp, tag_hash, and
+ * memcpy.
+ */
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);
+extern void dshash_memcpy(void *dest, const void *src, size_t size, void *arg);
+
+/*
+ * Convenience hash, compare, and copy functions wrapping strcmp, string_hash,
+ * and strcpy.
+ */
+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);
+extern void dshash_strcpy(void *dest, const void *src, size_t size, void *arg);
/* Debugging support. */
extern void dshash_dump(dshash_table *hash_table);
--
2.25.1
