public inbox for [email protected]help / color / mirror / Atom feed
dshash_find_or_insert vs. OOM 15+ messages / 5 participants [nested] [flat]
* dshash_find_or_insert vs. OOM @ 2026-03-17 23:34 Robert Haas <[email protected]> 0 siblings, 2 replies; 15+ messages in thread From: Robert Haas @ 2026-03-17 23:34 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]> Hi, For most memory allocation primitives, we offer a "no OOM" version. dshash_find_or_insert is an exception. Here's a small patch to fix that. I have some interest in slipping this into v19 even though I am submitting it quite late, because it would be useful for pg_stash_advice[1]. Let me know what you think about that. -- Robert Haas EDB: http://www.enterprisedb.com [1] As yet uncommitted. See pg_plan_advice thread. Attachments: [application/octet-stream] v1-0001-dshash-Make-it-possible-to-suppress-out-of-memory.patch (8.8K, 2-v1-0001-dshash-Make-it-possible-to-suppress-out-of-memory.patch) download | inline diff: From 65450b0498a42c26602a99c60a0698c755105dc8 Mon Sep 17 00:00:00 2001 From: Robert Haas <[email protected]> Date: Tue, 17 Mar 2026 17:29:40 -0400 Subject: [PATCH v1] dshash: Make it possible to suppress out of memory errors Introduce dshash_find_or_insert_extended, which is just like dshash_find_or_insert except that it takes a flags argument. Currently, the only supported flag is DSHASH_INSERT_NO_OOM, but I have chosen to use an integer rather than a boolean in case we end up with more flags in the future. --- src/backend/lib/dshash.c | 101 ++++++++++++++++++++++++++++----------- src/include/lib/dshash.h | 12 ++++- 2 files changed, 83 insertions(+), 30 deletions(-) diff --git a/src/backend/lib/dshash.c b/src/backend/lib/dshash.c index 13cef7b894e..e1cd3470712 100644 --- a/src/backend/lib/dshash.c +++ b/src/backend/lib/dshash.c @@ -167,7 +167,8 @@ struct dshash_table static void delete_item(dshash_table *hash_table, dshash_table_item *item); -static void resize(dshash_table *hash_table, size_t new_size_log2); +static bool resize(dshash_table *hash_table, size_t new_size_log2, + int flags); static inline void ensure_valid_bucket_pointers(dshash_table *hash_table); static inline dshash_table_item *find_in_bucket(dshash_table *hash_table, const void *key, @@ -178,7 +179,8 @@ static void insert_item_into_bucket(dshash_table *hash_table, dsa_pointer *bucket); static dshash_table_item *insert_into_bucket(dshash_table *hash_table, const void *key, - dsa_pointer *bucket); + dsa_pointer *bucket, + int flags); static bool delete_key_from_bucket(dshash_table *hash_table, const void *key, dsa_pointer *bucket_head); @@ -422,19 +424,25 @@ dshash_find(dshash_table *hash_table, const void *key, bool exclusive) } /* - * Returns a pointer to an exclusively locked item which must be released with - * dshash_release_lock. If the key is found in the hash table, 'found' is set - * to true and a pointer to the existing entry is returned. If the key is not - * found, 'found' is set to false, and a pointer to a newly created entry is - * returned. + * Find an existing entry in a dshash_table, or insert a new one. + * + * DSHASH_INSERT_NO_OOM causes this function to return NULL when no memory is + * available for the new entry. Otherwise, such allocations will result in + * an ERROR. + * + * Any entry returned by this function is exclusively locked, and the caller + * must release that lock using dshash_release_lock. Notes above dshash_find() + * regarding locking and error handling equally apply here. + * + * On return, *found is set to true if an existing entry was found in the + * hash table, and otherwise false. * - * Notes above dshash_find() regarding locking and error handling equally - * apply here. */ void * -dshash_find_or_insert(dshash_table *hash_table, - const void *key, - bool *found) +dshash_find_or_insert_extended(dshash_table *hash_table, + const void *key, + bool *found, + int flags) { dshash_hash hash; size_t partition_index; @@ -477,14 +485,22 @@ restart: * reacquire all the locks in the right order to avoid deadlocks. */ LWLockRelease(PARTITION_LOCK(hash_table, partition_index)); - resize(hash_table, hash_table->size_log2 + 1); + if (!resize(hash_table, hash_table->size_log2 + 1, flags)) + return NULL; goto restart; } /* Finally we can try to insert the new item. */ item = insert_into_bucket(hash_table, key, - &BUCKET_FOR_HASH(hash_table, hash)); + &BUCKET_FOR_HASH(hash_table, hash), + flags); + if (item == NULL) + { + Assert((flags & DSHASH_INSERT_NO_OOM) != 0); + LWLockRelease(PARTITION_LOCK(hash_table, partition_index)); + return NULL; + } item->hash = hash; /* Adjust per-lock-partition counter for load factor knowledge. */ ++partition->count; @@ -854,10 +870,14 @@ delete_item(dshash_table *hash_table, dshash_table_item *item) * Grow the hash table if necessary to the requested number of buckets. The * requested size must be double some previously observed size. * + * If an out-of-memory condition is observed, this function returns false if + * flags includes DSHASH_INSERT_NO_OOM, and otherwise throws an ERROR. In all + * other cases, it returns true. + * * Must be called without any partition lock held. */ -static void -resize(dshash_table *hash_table, size_t new_size_log2) +static bool +resize(dshash_table *hash_table, size_t new_size_log2, int flags) { dsa_pointer old_buckets; dsa_pointer new_buckets_shared; @@ -882,23 +902,39 @@ resize(dshash_table *hash_table, size_t new_size_log2) * obtaining all the locks and return early. */ LWLockRelease(PARTITION_LOCK(hash_table, 0)); - return; + return true; } } Assert(new_size_log2 == hash_table->control->size_log2 + 1); /* Allocate the space for the new table. */ - new_buckets_shared = - dsa_allocate_extended(hash_table->area, - sizeof(dsa_pointer) * new_size, - DSA_ALLOC_HUGE | DSA_ALLOC_ZERO); - new_buckets = dsa_get_address(hash_table->area, new_buckets_shared); + { + int dsa_flags = DSA_ALLOC_HUGE | DSA_ALLOC_ZERO; + + if (flags & DSHASH_INSERT_NO_OOM) + dsa_flags |= DSA_ALLOC_NO_OOM; + new_buckets_shared = + dsa_allocate_extended(hash_table->area, + sizeof(dsa_pointer) * new_size, + dsa_flags); + } + + /* If DSHASH_INSERT_NO_OOM was specified, allocation may have failed. */ + if (!DsaPointerIsValid(new_buckets_shared)) + { + /* Release all the locks and return without resizing. */ + Assert((flags & DSHASH_INSERT_NO_OOM) != 0); + for (i = 0; i < DSHASH_NUM_PARTITIONS; ++i) + LWLockRelease(PARTITION_LOCK(hash_table, i)); + return false; + } /* * We've allocated the new bucket array; all that remains to do now is to * reinsert all items, which amounts to adjusting all the pointers. */ + new_buckets = dsa_get_address(hash_table->area, new_buckets_shared); size = ((size_t) 1) << hash_table->control->size_log2; for (i = 0; i < size; ++i) { @@ -928,6 +964,8 @@ resize(dshash_table *hash_table, size_t new_size_log2) /* Release all the locks. */ for (i = 0; i < DSHASH_NUM_PARTITIONS; ++i) LWLockRelease(PARTITION_LOCK(hash_table, i)); + + return true; } /* @@ -982,19 +1020,26 @@ insert_item_into_bucket(dshash_table *hash_table, /* * Allocate space for an entry with the given key and insert it into the - * provided bucket. + * provided bucket. Returns NULL if out of memory and DSHASH_INSERT_NO_OOM + * was specified in flags. */ static dshash_table_item * insert_into_bucket(dshash_table *hash_table, const void *key, - dsa_pointer *bucket) + dsa_pointer *bucket, + int flags) { dsa_pointer item_pointer; dshash_table_item *item; - - item_pointer = dsa_allocate(hash_table->area, - hash_table->params.entry_size + - MAXALIGN(sizeof(dshash_table_item))); + int dsa_flags; + + dsa_flags = (flags & DSHASH_INSERT_NO_OOM) ? DSA_ALLOC_NO_OOM : 0; + item_pointer = dsa_allocate_extended(hash_table->area, + hash_table->params.entry_size + + MAXALIGN(sizeof(dshash_table_item)), + dsa_flags); + if (!DsaPointerIsValid(item_pointer)) + return NULL; item = dsa_get_address(hash_table->area, item_pointer); copy_key(hash_table, ENTRY_FROM_ITEM(item), key); insert_item_into_bucket(hash_table, item_pointer, item, bucket); diff --git a/src/include/lib/dshash.h b/src/include/lib/dshash.h index 46a3ca7884f..64b758b381b 100644 --- a/src/include/lib/dshash.h +++ b/src/include/lib/dshash.h @@ -92,15 +92,23 @@ extern void dshash_detach(dshash_table *hash_table); extern dshash_table_handle dshash_get_hash_table_handle(dshash_table *hash_table); extern void dshash_destroy(dshash_table *hash_table); +/* Flags for dshash_find_or_insert_extended. */ +#define DSHASH_INSERT_NO_OOM 0x01 /* no failure if out-of-memory */ + /* Finding, creating, deleting entries. */ extern void *dshash_find(dshash_table *hash_table, const void *key, bool exclusive); -extern void *dshash_find_or_insert(dshash_table *hash_table, - const void *key, bool *found); +extern void *dshash_find_or_insert_extended(dshash_table *hash_table, + const void *key, bool *found, + int flags); extern bool dshash_delete_key(dshash_table *hash_table, const void *key); extern void dshash_delete_entry(dshash_table *hash_table, void *entry); extern void dshash_release_lock(dshash_table *hash_table, void *entry); +/* Find or insert with error on out-of-memory. */ +#define dshash_find_or_insert(hash_table, key, found) \ + dshash_find_or_insert_extended(hash_table, key, found, 0) + /* seq scan support */ extern void dshash_seq_init(dshash_seq_status *status, dshash_table *hash_table, bool exclusive); -- 2.51.0 ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: dshash_find_or_insert vs. OOM @ 2026-03-18 15:20 Sami Imseih <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 1 reply; 15+ messages in thread From: Sami Imseih @ 2026-03-18 15:20 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]> > Hi, > > For most memory allocation primitives, we offer a "no OOM" version. > dshash_find_or_insert is an exception. Here's a small patch to fix > that. I have some interest in slipping this into v19 even though I am > submitting it quite late, because it would be useful for > pg_stash_advice[1]. Let me know what you think about that. Thanks for the patch! I agree with this idea, as it could act as a good triggering point for a caller to perform an eviction of the dshash if they run out of space, and avoid a hard failure. Passing this as a flag seems OK with me. Not sure what other flags could be added in the future, but the flexibility is a good thing, IMO. I was debating if this should just be a dsh_params option, but maybe for the same dshash a caller may want OOM in some code path, and retry in some other. maybe? > + &BUCKET_FOR_HASH(hash_table, hash), > + flags); > + if (item == NULL) > + { > + Assert((flags & DSHASH_INSERT_NO_OOM) != 0); > + LWLockRelease(PARTITION_LOCK(hash_table, partition_index)); > + return NULL; > + } > ``` > > When OOM happens, Assert((flags & DSHASH_INSERT_NO_OOM) != 0); makes sense. But for resize(), the assert is inside resize(), while for insert_into_bucket(), the assert is in the caller. > That feels a bit inconsistent to me, and I think it hurts readability a little. A reader might wonder why there is no corresponding assert after resize() unless they go read the function body. > > I think either style is fine, but using the same style in both places would be better. > I agree with this. The assert should be if (!resize(hash_table, hash_table->size_log2 + 1, flags)) { Assert((flags & DSHASH_INSERT_NO_OOM) != 0); return NULL; } instead of inside resize(). I did some testing by triggering an OOM, a no-OOM, and an OOM with eviction afterwards, as a sanity check. All looks good. I've attached the tests I created with other basic testing, as dshash lacks direct testing in general, if you're inclined to add them. -- Sami Imseih Amazon Web Services (AWS) Attachments: [application/octet-stream] v1-0001-Add-test-module-for-dshash.patch (12.4K, 2-v1-0001-Add-test-module-for-dshash.patch) download | inline diff: From 85d28f9df061e43a02c302def82a371677b115cb Mon Sep 17 00:00:00 2001 From: "Sami Imseih (AWS)" <[email protected]> Date: Wed, 18 Mar 2026 14:34:32 +0000 Subject: [PATCH v1 1/1] Add test module for dshash This introduces src/test/modules/test_dshash, which exercises the dshash API: insert, find, sequential scan, delete_current, and delete_key. Additionally, it tests the OOM behavior of dshash_find_or_insert, both with and without the DSHASH_INSERT_NO_OOM flag, including verifying that inserts succeed after evicting entries to free space. --- src/test/modules/Makefile | 1 + src/test/modules/meson.build | 1 + src/test/modules/test_dshash/.gitignore | 4 + src/test/modules/test_dshash/Makefile | 23 ++ .../test_dshash/expected/test_dshash.out | 21 ++ src/test/modules/test_dshash/meson.build | 33 +++ .../modules/test_dshash/sql/test_dshash.sql | 13 ++ .../modules/test_dshash/test_dshash--1.0.sql | 13 ++ src/test/modules/test_dshash/test_dshash.c | 218 ++++++++++++++++++ .../modules/test_dshash/test_dshash.control | 4 + 10 files changed, 331 insertions(+) create mode 100644 src/test/modules/test_dshash/.gitignore create mode 100644 src/test/modules/test_dshash/Makefile create mode 100644 src/test/modules/test_dshash/expected/test_dshash.out create mode 100644 src/test/modules/test_dshash/meson.build create mode 100644 src/test/modules/test_dshash/sql/test_dshash.sql create mode 100644 src/test/modules/test_dshash/test_dshash--1.0.sql create mode 100644 src/test/modules/test_dshash/test_dshash.c create mode 100644 src/test/modules/test_dshash/test_dshash.control diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile index a1540269cf5..f7adaef0b65 100644 --- a/src/test/modules/Makefile +++ b/src/test/modules/Makefile @@ -26,6 +26,7 @@ SUBDIRS = \ test_custom_types \ test_ddl_deparse \ test_dsa \ + test_dshash \ test_dsm_registry \ test_escape \ test_extensions \ diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build index 7c052803c98..af499045eca 100644 --- a/src/test/modules/meson.build +++ b/src/test/modules/meson.build @@ -27,6 +27,7 @@ subdir('test_custom_stats') subdir('test_custom_types') subdir('test_ddl_deparse') subdir('test_dsa') +subdir('test_dshash') subdir('test_dsm_registry') subdir('test_escape') subdir('test_extensions') diff --git a/src/test/modules/test_dshash/.gitignore b/src/test/modules/test_dshash/.gitignore new file mode 100644 index 00000000000..5dcb3ff9723 --- /dev/null +++ b/src/test/modules/test_dshash/.gitignore @@ -0,0 +1,4 @@ +# Generated subdirectories +/log/ +/results/ +/tmp_check/ diff --git a/src/test/modules/test_dshash/Makefile b/src/test/modules/test_dshash/Makefile new file mode 100644 index 00000000000..aefba3af5fe --- /dev/null +++ b/src/test/modules/test_dshash/Makefile @@ -0,0 +1,23 @@ +# src/test/modules/test_dshash/Makefile + +MODULE_big = test_dshash +OBJS = \ + $(WIN32RES) \ + test_dshash.o +PGFILEDESC = "test_dshash - test code for dshash" + +EXTENSION = test_dshash +DATA = test_dshash--1.0.sql + +REGRESS = test_dshash + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = src/test/modules/test_dshash +top_builddir = ../../../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif diff --git a/src/test/modules/test_dshash/expected/test_dshash.out b/src/test/modules/test_dshash/expected/test_dshash.out new file mode 100644 index 00000000000..23dd5c4270b --- /dev/null +++ b/src/test/modules/test_dshash/expected/test_dshash.out @@ -0,0 +1,21 @@ +CREATE EXTENSION test_dshash; +-- Basic insert, find, seq scan, delete_current. +SELECT test_dshash_basic(); + test_dshash_basic +------------------- + +(1 row) + +-- Regular dshash_find_or_insert raises ERROR on OOM. +-- Use terse for verbosity to ignore the DETAIL message. +\set VERBOSITY terse +SELECT test_dshash_find_or_insert_oom_error(); +ERROR: out of memory +\set VERBOSITY default +-- DSHASH_INSERT_NO_OOM: fill until OOM, evict 50%, retry insert. +SELECT test_dshash_find_or_insert_oom_retry(); + test_dshash_find_or_insert_oom_retry +-------------------------------------- + +(1 row) + diff --git a/src/test/modules/test_dshash/meson.build b/src/test/modules/test_dshash/meson.build new file mode 100644 index 00000000000..4c0ce938379 --- /dev/null +++ b/src/test/modules/test_dshash/meson.build @@ -0,0 +1,33 @@ +# Copyright (c) 2024-2026, PostgreSQL Global Development Group + +test_dshash_sources = files( + 'test_dshash.c', +) + +if host_system == 'windows' + test_dshash_sources += rc_lib_gen.process(win32ver_rc, extra_args: [ + '--NAME', 'test_dshash', + '--FILEDESC', 'test_dshash - test code for dshash',]) +endif + +test_dshash = shared_module('test_dshash', + test_dshash_sources, + kwargs: pg_test_mod_args, +) +test_install_libs += test_dshash + +test_install_data += files( + 'test_dshash.control', + 'test_dshash--1.0.sql', +) + +tests += { + 'name': 'test_dshash', + 'sd': meson.current_source_dir(), + 'bd': meson.current_build_dir(), + 'regress': { + 'sql': [ + 'test_dshash', + ], + }, +} diff --git a/src/test/modules/test_dshash/sql/test_dshash.sql b/src/test/modules/test_dshash/sql/test_dshash.sql new file mode 100644 index 00000000000..8c3428c4d9f --- /dev/null +++ b/src/test/modules/test_dshash/sql/test_dshash.sql @@ -0,0 +1,13 @@ +CREATE EXTENSION test_dshash; + +-- Basic insert, find, seq scan, delete_current. +SELECT test_dshash_basic(); + +-- Regular dshash_find_or_insert raises ERROR on OOM. +-- Use terse for verbosity to ignore the DETAIL message. +\set VERBOSITY terse +SELECT test_dshash_find_or_insert_oom_error(); +\set VERBOSITY default + +-- DSHASH_INSERT_NO_OOM: fill until OOM, evict 50%, retry insert. +SELECT test_dshash_find_or_insert_oom_retry(); diff --git a/src/test/modules/test_dshash/test_dshash--1.0.sql b/src/test/modules/test_dshash/test_dshash--1.0.sql new file mode 100644 index 00000000000..45a5541cd68 --- /dev/null +++ b/src/test/modules/test_dshash/test_dshash--1.0.sql @@ -0,0 +1,13 @@ +/* src/test/modules/test_dshash/test_dshash--1.0.sql */ + +-- complain if script is sourced in psql, rather than via CREATE EXTENSION +\echo Use "CREATE EXTENSION test_dshash" to load this file. \quit + +CREATE FUNCTION test_dshash_basic() RETURNS VOID + AS 'MODULE_PATHNAME' LANGUAGE C; + +CREATE FUNCTION test_dshash_find_or_insert_oom_error() RETURNS VOID + AS 'MODULE_PATHNAME' LANGUAGE C; + +CREATE FUNCTION test_dshash_find_or_insert_oom_retry() RETURNS VOID + AS 'MODULE_PATHNAME' LANGUAGE C; diff --git a/src/test/modules/test_dshash/test_dshash.c b/src/test/modules/test_dshash/test_dshash.c new file mode 100644 index 00000000000..b16554dae49 --- /dev/null +++ b/src/test/modules/test_dshash/test_dshash.c @@ -0,0 +1,218 @@ +/*-------------------------------------------------------------------------- + * + * test_dshash.c + * Test dynamic shared hash tables (dshash). + * + * Copyright (c) 2024-2026, PostgreSQL Global Development Group + * + * IDENTIFICATION + * src/test/modules/test_dshash/test_dshash.c + * + * ------------------------------------------------------------------------- + */ +#include "postgres.h" + +#include "fmgr.h" +#include "lib/dshash.h" +#include "storage/dsm_registry.h" +#include "storage/lwlock.h" +#include "utils/dsa.h" + +PG_MODULE_MAGIC; + +typedef struct TestDshashEntry +{ + int key; +} TestDshashEntry; + +static const dshash_parameters test_params = { + sizeof(int), /* key_size */ + sizeof(TestDshashEntry), /* entry_size */ + dshash_memcmp, + dshash_memhash, + dshash_memcpy, + LWTRANCHE_FIRST_USER_DEFINED /* tranche_id, overwritten at runtime */ +}; + +static void +init_tranche(void *ptr, void *arg) +{ + int *tranche_id = (int *) ptr; + + *tranche_id = LWLockNewTrancheId("test_dshash"); +} + +/* + * test_dshash_basic + * + * Test insert, find, sequential scan, delete_current, and delete_key. + */ +PG_FUNCTION_INFO_V1(test_dshash_basic); +Datum +test_dshash_basic(PG_FUNCTION_ARGS) +{ + int *tranche_id; + bool found; + dsa_area *area; + dshash_table *ht; + dshash_parameters params = test_params; + dshash_seq_status status; + TestDshashEntry *entry; + int count = 10; + int scanned; + + tranche_id = GetNamedDSMSegment("test_dshash", sizeof(int), + init_tranche, &found, NULL); + params.tranche_id = *tranche_id; + + area = dsa_create(*tranche_id); + ht = dshash_create(area, ¶ms, NULL); + + /* Insert entries. */ + for (int i = 0; i < count; i++) + { + entry = dshash_find_or_insert(ht, &i, &found); + if (found) + elog(ERROR, "unexpected duplicate key %d", i); + dshash_release_lock(ht, entry); + } + + /* Verify all entries via find. */ + for (int i = 0; i < count; i++) + { + entry = dshash_find(ht, &i, false); + if (entry == NULL) + elog(ERROR, "key %d not found", i); + dshash_release_lock(ht, entry); + } + + /* Verify entry count via sequential scan. */ + scanned = 0; + dshash_seq_init(&status, ht, false); + while ((entry = dshash_seq_next(&status)) != NULL) + scanned++; + dshash_seq_term(&status); + + if (scanned != count) + elog(ERROR, "seq scan returned %d entries, expected %d", scanned, count); + + /* Delete all entries via delete_current. */ + dshash_seq_init(&status, ht, true); + while ((entry = dshash_seq_next(&status)) != NULL) + dshash_delete_current(&status); + dshash_seq_term(&status); + + /* Verify table is empty. */ + scanned = 0; + dshash_seq_init(&status, ht, false); + while ((entry = dshash_seq_next(&status)) != NULL) + scanned++; + dshash_seq_term(&status); + + if (scanned != 0) + elog(ERROR, "expected empty table, got %d entries", scanned); + + dshash_destroy(ht); + dsa_detach(area); + + PG_RETURN_VOID(); +} + +/* + * test_dshash_find_or_insert_oom_error + * + * Test that the regular dshash_find_or_insert (without DSHASH_INSERT_NO_OOM) + * raises ERROR when the DSA area is "out of memory". + */ +PG_FUNCTION_INFO_V1(test_dshash_find_or_insert_oom_error); +Datum +test_dshash_find_or_insert_oom_error(PG_FUNCTION_ARGS) +{ + int *tranche_id; + bool found; + dsa_area *area; + dshash_table *ht; + dshash_parameters params = test_params; + int key = 0; + + tranche_id = GetNamedDSMSegment("test_dshash", sizeof(int), + init_tranche, &found, NULL); + params.tranche_id = *tranche_id; + + area = dsa_create(*tranche_id); + dsa_set_size_limit(area, dsa_minimum_size()); + ht = dshash_create(area, ¶ms, NULL); + + /* Insert until OOM — without NO_OOM flag, this should raise ERROR. */ + for (;;) + { + TestDshashEntry *entry; + + entry = dshash_find_or_insert(ht, &key, &found); + dshash_release_lock(ht, entry); + key++; + } + + PG_RETURN_VOID(); +} + +/* + * test_dshash_find_or_insert_oom_retry + * + * Test DSHASH_INSERT_NO_OOM: fill until OOM, evict 50% of entries, then + * retry the insert to verify recovery works. + */ +PG_FUNCTION_INFO_V1(test_dshash_find_or_insert_oom_retry); +Datum +test_dshash_find_or_insert_oom_retry(PG_FUNCTION_ARGS) +{ + int *tranche_id; + bool found; + dsa_area *area; + dshash_table *ht; + dshash_parameters params = test_params; + int inserted = 0; + int key = 0; + + tranche_id = GetNamedDSMSegment("test_dshash", sizeof(int), + init_tranche, &found, NULL); + params.tranche_id = *tranche_id; + + area = dsa_create(*tranche_id); + dsa_set_size_limit(area, dsa_minimum_size()); + ht = dshash_create(area, ¶ms, NULL); + + /* Fill until OOM. */ + for (;;) + { + TestDshashEntry *entry; + + entry = dshash_find_or_insert_extended(ht, &key, &found, + DSHASH_INSERT_NO_OOM); + if (entry == NULL) + break; + dshash_release_lock(ht, entry); + key++; + inserted++; + } + + /* Evict 50% of entries. */ + for (int i = 0; i < inserted / 2; i++) + dshash_delete_key(ht, &i); + + /* Retry — should succeed now that there's free space. */ + { + TestDshashEntry *entry; + + entry = dshash_find_or_insert_extended(ht, &key, &found, + DSHASH_INSERT_NO_OOM); + if (entry == NULL) + elog(ERROR, "insert after eviction failed"); + dshash_release_lock(ht, entry); + } + + dshash_destroy(ht); + dsa_detach(area); + + PG_RETURN_VOID(); +} diff --git a/src/test/modules/test_dshash/test_dshash.control b/src/test/modules/test_dshash/test_dshash.control new file mode 100644 index 00000000000..7ab0666d227 --- /dev/null +++ b/src/test/modules/test_dshash/test_dshash.control @@ -0,0 +1,4 @@ +comment = 'Test code for dshash' +default_version = '1.0' +module_pathname = '$libdir/test_dshash' +relocatable = true -- 2.47.3 ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: dshash_find_or_insert vs. OOM @ 2026-03-18 17:46 Robert Haas <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 2 replies; 15+ messages in thread From: Robert Haas @ 2026-03-18 17:46 UTC (permalink / raw) To: Chao Li <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]> On Tue, Mar 17, 2026 at 9:34 PM Chao Li <[email protected]> wrote: > When OOM happens, Assert((flags & DSHASH_INSERT_NO_OOM) != 0); makes sense. But for resize(), the assert is inside resize(), while for insert_into_bucket(), the assert is in the caller. That feels a bit inconsistent to me, and I think it hurts readability a little. A reader might wonder why there is no corresponding assert after resize() unless they go read the function body. Adjusted. > Making this a nested block does have the benefit of keeping dsa_flags close to where it is used. But from my impression, this style is still fairly uncommon in the codebase. I worry it may implicitly signal to other hackers that this is an acceptable pattern. So unless we intentionally want to encourage that style, I would lean toward avoiding it here. Yeah, that was dumb. Fixed. Thanks for the review; here's v2. -- Robert Haas EDB: http://www.enterprisedb.com Attachments: [application/octet-stream] v2-0001-dshash-Make-it-possible-to-suppress-out-of-memory.patch (9.0K, 2-v2-0001-dshash-Make-it-possible-to-suppress-out-of-memory.patch) download | inline diff: From ea1c037dfa936dfe436b4499985e7aa99cbfb50b Mon Sep 17 00:00:00 2001 From: Robert Haas <[email protected]> Date: Wed, 18 Mar 2026 13:39:47 -0400 Subject: [PATCH v2] dshash: Make it possible to suppress out of memory errors Introduce dshash_find_or_insert_extended, which is just like dshash_find_or_insert except that it takes a flags argument. Currently, the only supported flag is DSHASH_INSERT_NO_OOM, but I have chosen to use an integer rather than a boolean in case we end up with more flags in the future. Reviewed-by: Chao Li <[email protected]> Reviewed-by: Sami Imseih <[email protected]> --- src/backend/lib/dshash.c | 94 +++++++++++++++++++++++++++++----------- src/include/lib/dshash.h | 12 ++++- 2 files changed, 79 insertions(+), 27 deletions(-) diff --git a/src/backend/lib/dshash.c b/src/backend/lib/dshash.c index 13cef7b894e..1999989c14f 100644 --- a/src/backend/lib/dshash.c +++ b/src/backend/lib/dshash.c @@ -167,7 +167,8 @@ struct dshash_table static void delete_item(dshash_table *hash_table, dshash_table_item *item); -static void resize(dshash_table *hash_table, size_t new_size_log2); +static bool resize(dshash_table *hash_table, size_t new_size_log2, + int flags); static inline void ensure_valid_bucket_pointers(dshash_table *hash_table); static inline dshash_table_item *find_in_bucket(dshash_table *hash_table, const void *key, @@ -178,7 +179,8 @@ static void insert_item_into_bucket(dshash_table *hash_table, dsa_pointer *bucket); static dshash_table_item *insert_into_bucket(dshash_table *hash_table, const void *key, - dsa_pointer *bucket); + dsa_pointer *bucket, + int flags); static bool delete_key_from_bucket(dshash_table *hash_table, const void *key, dsa_pointer *bucket_head); @@ -422,19 +424,25 @@ dshash_find(dshash_table *hash_table, const void *key, bool exclusive) } /* - * Returns a pointer to an exclusively locked item which must be released with - * dshash_release_lock. If the key is found in the hash table, 'found' is set - * to true and a pointer to the existing entry is returned. If the key is not - * found, 'found' is set to false, and a pointer to a newly created entry is - * returned. + * Find an existing entry in a dshash_table, or insert a new one. + * + * DSHASH_INSERT_NO_OOM causes this function to return NULL when no memory is + * available for the new entry. Otherwise, such allocations will result in + * an ERROR. + * + * Any entry returned by this function is exclusively locked, and the caller + * must release that lock using dshash_release_lock. Notes above dshash_find() + * regarding locking and error handling equally apply here. + * + * On return, *found is set to true if an existing entry was found in the + * hash table, and otherwise false. * - * Notes above dshash_find() regarding locking and error handling equally - * apply here. */ void * -dshash_find_or_insert(dshash_table *hash_table, - const void *key, - bool *found) +dshash_find_or_insert_extended(dshash_table *hash_table, + const void *key, + bool *found, + int flags) { dshash_hash hash; size_t partition_index; @@ -477,14 +485,25 @@ restart: * reacquire all the locks in the right order to avoid deadlocks. */ LWLockRelease(PARTITION_LOCK(hash_table, partition_index)); - resize(hash_table, hash_table->size_log2 + 1); + if (!resize(hash_table, hash_table->size_log2 + 1, flags)) + { + Assert((flags & DSHASH_INSERT_NO_OOM) != 0); + return NULL; + } goto restart; } /* Finally we can try to insert the new item. */ item = insert_into_bucket(hash_table, key, - &BUCKET_FOR_HASH(hash_table, hash)); + &BUCKET_FOR_HASH(hash_table, hash), + flags); + if (item == NULL) + { + Assert((flags & DSHASH_INSERT_NO_OOM) != 0); + LWLockRelease(PARTITION_LOCK(hash_table, partition_index)); + return NULL; + } item->hash = hash; /* Adjust per-lock-partition counter for load factor knowledge. */ ++partition->count; @@ -854,10 +873,14 @@ delete_item(dshash_table *hash_table, dshash_table_item *item) * Grow the hash table if necessary to the requested number of buckets. The * requested size must be double some previously observed size. * + * If an out-of-memory condition is observed, this function returns false if + * flags includes DSHASH_INSERT_NO_OOM, and otherwise throws an ERROR. In all + * other cases, it returns true. + * * Must be called without any partition lock held. */ -static void -resize(dshash_table *hash_table, size_t new_size_log2) +static bool +resize(dshash_table *hash_table, size_t new_size_log2, int flags) { dsa_pointer old_buckets; dsa_pointer new_buckets_shared; @@ -865,6 +888,7 @@ resize(dshash_table *hash_table, size_t new_size_log2) size_t size; size_t new_size = ((size_t) 1) << new_size_log2; size_t i; + int dsa_flags = DSA_ALLOC_HUGE | DSA_ALLOC_ZERO; /* * Acquire the locks for all lock partitions. This is expensive, but we @@ -882,23 +906,34 @@ resize(dshash_table *hash_table, size_t new_size_log2) * obtaining all the locks and return early. */ LWLockRelease(PARTITION_LOCK(hash_table, 0)); - return; + return true; } } Assert(new_size_log2 == hash_table->control->size_log2 + 1); /* Allocate the space for the new table. */ + if (flags & DSHASH_INSERT_NO_OOM) + dsa_flags |= DSA_ALLOC_NO_OOM; new_buckets_shared = dsa_allocate_extended(hash_table->area, sizeof(dsa_pointer) * new_size, - DSA_ALLOC_HUGE | DSA_ALLOC_ZERO); - new_buckets = dsa_get_address(hash_table->area, new_buckets_shared); + dsa_flags); + + /* If DSHASH_INSERT_NO_OOM was specified, allocation may have failed. */ + if (!DsaPointerIsValid(new_buckets_shared)) + { + /* Release all the locks and return without resizing. */ + for (i = 0; i < DSHASH_NUM_PARTITIONS; ++i) + LWLockRelease(PARTITION_LOCK(hash_table, i)); + return false; + } /* * We've allocated the new bucket array; all that remains to do now is to * reinsert all items, which amounts to adjusting all the pointers. */ + new_buckets = dsa_get_address(hash_table->area, new_buckets_shared); size = ((size_t) 1) << hash_table->control->size_log2; for (i = 0; i < size; ++i) { @@ -928,6 +963,8 @@ resize(dshash_table *hash_table, size_t new_size_log2) /* Release all the locks. */ for (i = 0; i < DSHASH_NUM_PARTITIONS; ++i) LWLockRelease(PARTITION_LOCK(hash_table, i)); + + return true; } /* @@ -982,19 +1019,26 @@ insert_item_into_bucket(dshash_table *hash_table, /* * Allocate space for an entry with the given key and insert it into the - * provided bucket. + * provided bucket. Returns NULL if out of memory and DSHASH_INSERT_NO_OOM + * was specified in flags. */ static dshash_table_item * insert_into_bucket(dshash_table *hash_table, const void *key, - dsa_pointer *bucket) + dsa_pointer *bucket, + int flags) { dsa_pointer item_pointer; dshash_table_item *item; - - item_pointer = dsa_allocate(hash_table->area, - hash_table->params.entry_size + - MAXALIGN(sizeof(dshash_table_item))); + int dsa_flags; + + dsa_flags = (flags & DSHASH_INSERT_NO_OOM) ? DSA_ALLOC_NO_OOM : 0; + item_pointer = dsa_allocate_extended(hash_table->area, + hash_table->params.entry_size + + MAXALIGN(sizeof(dshash_table_item)), + dsa_flags); + if (!DsaPointerIsValid(item_pointer)) + return NULL; item = dsa_get_address(hash_table->area, item_pointer); copy_key(hash_table, ENTRY_FROM_ITEM(item), key); insert_item_into_bucket(hash_table, item_pointer, item, bucket); diff --git a/src/include/lib/dshash.h b/src/include/lib/dshash.h index 46a3ca7884f..64b758b381b 100644 --- a/src/include/lib/dshash.h +++ b/src/include/lib/dshash.h @@ -92,15 +92,23 @@ extern void dshash_detach(dshash_table *hash_table); extern dshash_table_handle dshash_get_hash_table_handle(dshash_table *hash_table); extern void dshash_destroy(dshash_table *hash_table); +/* Flags for dshash_find_or_insert_extended. */ +#define DSHASH_INSERT_NO_OOM 0x01 /* no failure if out-of-memory */ + /* Finding, creating, deleting entries. */ extern void *dshash_find(dshash_table *hash_table, const void *key, bool exclusive); -extern void *dshash_find_or_insert(dshash_table *hash_table, - const void *key, bool *found); +extern void *dshash_find_or_insert_extended(dshash_table *hash_table, + const void *key, bool *found, + int flags); extern bool dshash_delete_key(dshash_table *hash_table, const void *key); extern void dshash_delete_entry(dshash_table *hash_table, void *entry); extern void dshash_release_lock(dshash_table *hash_table, void *entry); +/* Find or insert with error on out-of-memory. */ +#define dshash_find_or_insert(hash_table, key, found) \ + dshash_find_or_insert_extended(hash_table, key, found, 0) + /* seq scan support */ extern void dshash_seq_init(dshash_seq_status *status, dshash_table *hash_table, bool exclusive); -- 2.51.0 ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: dshash_find_or_insert vs. OOM @ 2026-03-18 18:26 Sami Imseih <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 0 replies; 15+ messages in thread From: Sami Imseih @ 2026-03-18 18:26 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Chao Li <[email protected]>; PostgreSQL Hackers <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]> > Thanks for the review; here's v2. LGTM -- Sami Imseih Amazon Web Services (AWS) ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: dshash_find_or_insert vs. OOM @ 2026-03-18 18:42 Robert Haas <[email protected]> parent: Sami Imseih <[email protected]> 0 siblings, 1 reply; 15+ messages in thread From: Robert Haas @ 2026-03-18 18:42 UTC (permalink / raw) To: Sami Imseih <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]> On Wed, Mar 18, 2026 at 11:21 AM Sami Imseih <[email protected]> wrote: > Passing this as a flag seems OK with me. Not sure what other > flags could be added in the future, but the flexibility is a good > thing, IMO. I was debating if this should just be a dsh_params > option, but maybe for the same dshash a caller may want OOM > in some code path, and retry in some other. maybe? Yeah, I think this is a property of the individual call to dshash_find_or_insert(), rather than a property of the table. It does seem likely that most callers will want the same behavior in all cases, but it would be very sad if we embedded that idea into the structure of the code and then somebody has a case where it's not what they want, and I see no real reason why that couldn't happen. Also, doing it that way wouldn't really follow existing precedents (pg_malloc_extended, palloc_extended, MemoryContextAllocExtended, dsa_allocate_extended). > I did some testing by triggering an OOM, a no-OOM, and an OOM with > eviction afterwards, as a sanity check. All looks good. > I've attached the tests I created with other basic testing, as dshash > lacks direct testing in general, if you're inclined to add them. I tried running a coverage report for this. It's pretty good. It doesn't cover these things: - dshash_create: OOM while allocating the bucket array. - dshash_find_or_insert_extended: OOM while inserting an item (apparently, at least here, it instead hits OOM while resizing) - dshash_delete_key: the case where the entry not found - dshash_dump: not called at all - resize: the case where we exit early due to a concurrent resize - delete_item_from_bucket: the case where there is more than one item in the bucket I think that adding a call to dshash_dump() somewhere would probably make sense, and I'd suggest also trying to delete a nonexistent key. The rest don't seem worth worrying about. On the code itself: + /* Verify all entries via find. */ + for (int i = 0; i < count; i++) + { + entry = dshash_find(ht, &i, false); + if (entry == NULL) + elog(ERROR, "key %d not found", i); + dshash_release_lock(ht, entry); + } You could verify that entry->key == i. The current code wouldn't notice if the hash table returned a garbage pointer. You could possibly also include some kind of a payload in each record and verify that, e.g. set entry->value = some_homomorphism_over_0_to_9(entry->key). + dsa_set_size_limit(area, dsa_minimum_size()); This is an abuse of the documented purpose of dsa_set_size_limit(). Seems better to just pick a constant here. + /* Insert until OOM — without NO_OOM flag, this should raise ERROR. */ + for (;;) I think it would be safer to code this as a fixed iteration count. For example, if you choose the size limit so that we should run out of memory after 10 entries, you could terminate this loop after 1000 iterations. That way, if something goes wrong, we're more likely to see "expected out-of-memory, but completed all %d iterations" in the log rather than a core dump or whatever. I+ { + TestDshashEntry *entry; + + entry = dshash_find_or_insert(ht, &key, &found); + dshash_release_lock(ht, entry); + key++; + } In other places, you check for entry == NULL, but not here. + { + TestDshashEntry *entry; + + entry = dshash_find_or_insert_extended(ht, &key, &found, + DSHASH_INSERT_NO_OOM); + if (entry == NULL) + elog(ERROR, "insert after eviction failed"); + dshash_release_lock(ht, entry); + } I just got in trouble for letting a bare block sneak into my code, so now it's my turn to complain. I suggest git config user.email in whatever directory you use to generate patches like this, just to make life a bit easier for anyone who might eventually be committing one of them. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: dshash_find_or_insert vs. OOM @ 2026-03-19 00:08 Chao Li <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 1 reply; 15+ messages in thread From: Chao Li @ 2026-03-19 00:08 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]> > On Mar 19, 2026, at 01:46, Robert Haas <[email protected]> wrote: > > On Tue, Mar 17, 2026 at 9:34 PM Chao Li <[email protected]> wrote: >> When OOM happens, Assert((flags & DSHASH_INSERT_NO_OOM) != 0); makes sense. But for resize(), the assert is inside resize(), while for insert_into_bucket(), the assert is in the caller. That feels a bit inconsistent to me, and I think it hurts readability a little. A reader might wonder why there is no corresponding assert after resize() unless they go read the function body. > > Adjusted. > >> Making this a nested block does have the benefit of keeping dsa_flags close to where it is used. But from my impression, this style is still fairly uncommon in the codebase. I worry it may implicitly signal to other hackers that this is an acceptable pattern. So unless we intentionally want to encourage that style, I would lean toward avoiding it here. > > Yeah, that was dumb. Fixed. > > Thanks for the review; here's v2. > > -- > Robert Haas > EDB: http://www.enterprisedb.com > <v2-0001-dshash-Make-it-possible-to-suppress-out-of-memory.patch> Thanks for updating the patch. V2 LGTM. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: dshash_find_or_insert vs. OOM @ 2026-03-19 16:26 Robert Haas <[email protected]> parent: Chao Li <[email protected]> 0 siblings, 0 replies; 15+ messages in thread From: Robert Haas @ 2026-03-19 16:26 UTC (permalink / raw) To: Chao Li <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]> On Wed, Mar 18, 2026 at 8:09 PM Chao Li <[email protected]> wrote: > Thanks for updating the patch. V2 LGTM. Committed, thanks to both of you. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: dshash_find_or_insert vs. OOM @ 2026-03-19 19:15 Sami Imseih <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 15+ messages in thread From: Sami Imseih @ 2026-03-19 19:15 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]> Thanks for the review! > > I did some testing by triggering an OOM, a no-OOM, and an OOM with > > eviction afterwards, as a sanity check. All looks good. > > I've attached the tests I created with other basic testing, as dshash > > lacks direct testing in general, if you're inclined to add them. > > I tried running a coverage report for this. It's pretty good. It > doesn't cover these things: > > - dshash_create: OOM while allocating the bucket array. > - dshash_find_or_insert_extended: OOM while inserting an item > (apparently, at least here, it instead hits OOM while resizing) > - dshash_delete_key: the case where the entry not found > - dshash_dump: not called at all > - resize: the case where we exit early due to a concurrent resize > - delete_item_from_bucket: the case where there is more than one item > in the bucket These are good to add. I also included delete_entry and dshash_dump for more coverage. delete_item_from_bucket will require us to hash the item to the same bucket, and I'm not sure it's worth the hassle. resize() will occur in the OOM test. > I think that adding a call to dshash_dump() somewhere would probably Done > make sense, and I'd suggest also trying to delete a nonexistent key. Done > > On the code itself: > > + /* Verify all entries via find. */ > + for (int i = 0; i < count; i++) > + { > + entry = dshash_find(ht, &i, false); > + if (entry == NULL) > + elog(ERROR, "key %d not found", i); > + dshash_release_lock(ht, entry); > + } > > You could verify that entry->key == i. The current code wouldn't > notice if the hash table returned a garbage pointer. You could > possibly also include some kind of a payload in each record and verify > that, e.g. set entry->value = > some_homomorphism_over_0_to_9(entry->key). I added key verification as you suggested. > + dsa_set_size_limit(area, dsa_minimum_size()); > > This is an abuse of the documented purpose of dsa_set_size_limit(). > Seems better to just pick a constant here. I changed this to use a different limit. > + /* Insert until OOM — without NO_OOM flag, this should raise ERROR. */ > + for (;;) > > I think it would be safer to code this as a fixed iteration count. For > example, if you choose the size limit so that we should run out of > memory after 10 entries, you could terminate this loop after 1000 > iterations. That way, if something goes wrong, we're more likely to > see "expected out-of-memory, but completed all %d iterations" in the > log rather than a core dump or whatever. Done. I also removed the OOM retry test and just kept a simple test. Adding entries after a delete is now happening in the basic test. > I+ { > + TestDshashEntry *entry; > + > + entry = dshash_find_or_insert(ht, &key, &found); > + dshash_release_lock(ht, entry); > + key++; > + } > > In other places, you check for entry == NULL, but not here. Fixed. > I just got in trouble for letting a bare block sneak into my code, so > now it's my turn to complain. ugggh. fixed. > I suggest git config user.email in whatever directory you use to > generate patches like this, just to make life a bit easier for anyone > who might eventually be committing one of them. Sorry. I usually do, but I started using a new machine :( -- Sami Imseih Amazon Web Services (AWS) Attachments: [application/octet-stream] v2-0001-Add-test-module-for-dshash.patch (12.3K, 2-v2-0001-Add-test-module-for-dshash.patch) download | inline diff: From 4993045532760593bce948577209b9285e0ca01c Mon Sep 17 00:00:00 2001 From: Sami Imseih <[email protected]> Date: Thu, 19 Mar 2026 15:26:46 +0000 Subject: [PATCH v2 1/1] Add test module for dshash This introduces comprehensive tests to the dshash APIs: insert, find, sequential scan, delete_entry, delete_current, delete_key, and dshash_dump, payload verification, resize and OOM handling. --- src/test/modules/Makefile | 1 + src/test/modules/meson.build | 1 + src/test/modules/test_dshash/.gitignore | 4 + src/test/modules/test_dshash/Makefile | 23 ++ .../test_dshash/expected/test_dshash.out | 14 ++ src/test/modules/test_dshash/meson.build | 33 +++ .../modules/test_dshash/sql/test_dshash.sql | 10 + .../modules/test_dshash/test_dshash--1.0.sql | 10 + src/test/modules/test_dshash/test_dshash.c | 218 ++++++++++++++++++ .../modules/test_dshash/test_dshash.control | 4 + 10 files changed, 318 insertions(+) create mode 100644 src/test/modules/test_dshash/.gitignore create mode 100644 src/test/modules/test_dshash/Makefile create mode 100644 src/test/modules/test_dshash/expected/test_dshash.out create mode 100644 src/test/modules/test_dshash/meson.build create mode 100644 src/test/modules/test_dshash/sql/test_dshash.sql create mode 100644 src/test/modules/test_dshash/test_dshash--1.0.sql create mode 100644 src/test/modules/test_dshash/test_dshash.c create mode 100644 src/test/modules/test_dshash/test_dshash.control diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile index 28ce3b35eda..51f2b1b48c5 100644 --- a/src/test/modules/Makefile +++ b/src/test/modules/Makefile @@ -26,6 +26,7 @@ SUBDIRS = \ test_custom_types \ test_ddl_deparse \ test_dsa \ + test_dshash \ test_dsm_registry \ test_escape \ test_extensions \ diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build index 3ac291656c1..dcf813b0823 100644 --- a/src/test/modules/meson.build +++ b/src/test/modules/meson.build @@ -27,6 +27,7 @@ subdir('test_custom_stats') subdir('test_custom_types') subdir('test_ddl_deparse') subdir('test_dsa') +subdir('test_dshash') subdir('test_dsm_registry') subdir('test_escape') subdir('test_extensions') diff --git a/src/test/modules/test_dshash/.gitignore b/src/test/modules/test_dshash/.gitignore new file mode 100644 index 00000000000..5dcb3ff9723 --- /dev/null +++ b/src/test/modules/test_dshash/.gitignore @@ -0,0 +1,4 @@ +# Generated subdirectories +/log/ +/results/ +/tmp_check/ diff --git a/src/test/modules/test_dshash/Makefile b/src/test/modules/test_dshash/Makefile new file mode 100644 index 00000000000..aefba3af5fe --- /dev/null +++ b/src/test/modules/test_dshash/Makefile @@ -0,0 +1,23 @@ +# src/test/modules/test_dshash/Makefile + +MODULE_big = test_dshash +OBJS = \ + $(WIN32RES) \ + test_dshash.o +PGFILEDESC = "test_dshash - test code for dshash" + +EXTENSION = test_dshash +DATA = test_dshash--1.0.sql + +REGRESS = test_dshash + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = src/test/modules/test_dshash +top_builddir = ../../../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif diff --git a/src/test/modules/test_dshash/expected/test_dshash.out b/src/test/modules/test_dshash/expected/test_dshash.out new file mode 100644 index 00000000000..69f4628bd5e --- /dev/null +++ b/src/test/modules/test_dshash/expected/test_dshash.out @@ -0,0 +1,14 @@ +CREATE EXTENSION test_dshash; +-- Exercise core dshash operations. +SELECT test_dshash_basic(); + test_dshash_basic +------------------- + +(1 row) + +-- Regular dshash_find_or_insert raises ERROR on OOM. +-- Use terse for verbosity to ignore the DETAIL message. +\set VERBOSITY terse +SELECT test_dshash_find_or_insert_oom_error(); +ERROR: out of memory +\set VERBOSITY default diff --git a/src/test/modules/test_dshash/meson.build b/src/test/modules/test_dshash/meson.build new file mode 100644 index 00000000000..4c0ce938379 --- /dev/null +++ b/src/test/modules/test_dshash/meson.build @@ -0,0 +1,33 @@ +# Copyright (c) 2024-2026, PostgreSQL Global Development Group + +test_dshash_sources = files( + 'test_dshash.c', +) + +if host_system == 'windows' + test_dshash_sources += rc_lib_gen.process(win32ver_rc, extra_args: [ + '--NAME', 'test_dshash', + '--FILEDESC', 'test_dshash - test code for dshash',]) +endif + +test_dshash = shared_module('test_dshash', + test_dshash_sources, + kwargs: pg_test_mod_args, +) +test_install_libs += test_dshash + +test_install_data += files( + 'test_dshash.control', + 'test_dshash--1.0.sql', +) + +tests += { + 'name': 'test_dshash', + 'sd': meson.current_source_dir(), + 'bd': meson.current_build_dir(), + 'regress': { + 'sql': [ + 'test_dshash', + ], + }, +} diff --git a/src/test/modules/test_dshash/sql/test_dshash.sql b/src/test/modules/test_dshash/sql/test_dshash.sql new file mode 100644 index 00000000000..672b87ac035 --- /dev/null +++ b/src/test/modules/test_dshash/sql/test_dshash.sql @@ -0,0 +1,10 @@ +CREATE EXTENSION test_dshash; + +-- Exercise core dshash operations. +SELECT test_dshash_basic(); + +-- Regular dshash_find_or_insert raises ERROR on OOM. +-- Use terse for verbosity to ignore the DETAIL message. +\set VERBOSITY terse +SELECT test_dshash_find_or_insert_oom_error(); +\set VERBOSITY default diff --git a/src/test/modules/test_dshash/test_dshash--1.0.sql b/src/test/modules/test_dshash/test_dshash--1.0.sql new file mode 100644 index 00000000000..85a6289850b --- /dev/null +++ b/src/test/modules/test_dshash/test_dshash--1.0.sql @@ -0,0 +1,10 @@ +/* src/test/modules/test_dshash/test_dshash--1.0.sql */ + +-- complain if script is sourced in psql, rather than via CREATE EXTENSION +\echo Use "CREATE EXTENSION test_dshash" to load this file. \quit + +CREATE FUNCTION test_dshash_basic() RETURNS VOID + AS 'MODULE_PATHNAME' LANGUAGE C; + +CREATE FUNCTION test_dshash_find_or_insert_oom_error() RETURNS VOID + AS 'MODULE_PATHNAME' LANGUAGE C; diff --git a/src/test/modules/test_dshash/test_dshash.c b/src/test/modules/test_dshash/test_dshash.c new file mode 100644 index 00000000000..bfd9abdf164 --- /dev/null +++ b/src/test/modules/test_dshash/test_dshash.c @@ -0,0 +1,218 @@ +/*-------------------------------------------------------------------------- + * + * test_dshash.c + * Test dynamic shared hash tables (dshash). + * + * Copyright (c) 2024-2026, PostgreSQL Global Development Group + * + * IDENTIFICATION + * src/test/modules/test_dshash/test_dshash.c + * + * ------------------------------------------------------------------------- + */ +#include "postgres.h" + +#include "fmgr.h" +#include "lib/dshash.h" +#include "storage/dsm_registry.h" +#include "storage/lwlock.h" +#include "utils/dsa.h" + +PG_MODULE_MAGIC; + +/* Size limit for OOM tests */ +#define TEST_DSHASH_SIZE_LIMIT (128 * 1024) + +/* More than enough to exhaust TEST_DSHASH_SIZE_LIMIT */ +#define TEST_DSHASH_MAX_OOM_ITERATIONS 10000 + +typedef struct TestDshashEntry +{ + int key; + int value; +} TestDshashEntry; + +/* To verify payload integrity */ +#define KEY_TO_VALUE(k) ((k) ^ 0x12345678) + +static const dshash_parameters test_params = { + sizeof(int), /* key_size */ + sizeof(TestDshashEntry), /* entry_size */ + dshash_memcmp, + dshash_memhash, + dshash_memcpy, + LWTRANCHE_FIRST_USER_DEFINED /* tranche_id, overwritten at runtime */ +}; + +static void +init_tranche(void *ptr, void *arg) +{ + int *tranche_id = (int *) ptr; + + *tranche_id = LWLockNewTrancheId("test_dshash"); +} + +/* + * test_dshash_basic + * + * Test insert, find, sequential scan, delete_current, delete_key, + * delete of a nonexistent key, and dshash_dump, and re-insertions + * after deletes. + */ +PG_FUNCTION_INFO_V1(test_dshash_basic); +Datum +test_dshash_basic(PG_FUNCTION_ARGS) +{ + int *tranche_id; + bool found; + dsa_area *area; + dshash_table *ht; + dshash_parameters params = test_params; + dshash_seq_status status; + TestDshashEntry *entry; + int count = 10; + int scanned; + int nonexistent_key; + + tranche_id = GetNamedDSMSegment("test_dshash", sizeof(int), + init_tranche, &found, NULL); + params.tranche_id = *tranche_id; + + area = dsa_create(*tranche_id); + ht = dshash_create(area, ¶ms, NULL); + + /* Insert entries with a payload. */ + for (int i = 0; i < count; i++) + { + entry = dshash_find_or_insert(ht, &i, &found); + if (found) + elog(ERROR, "unexpected duplicate key %d", i); + entry->value = KEY_TO_VALUE(i); + dshash_release_lock(ht, entry); + } + + /* Verify all entries via find, checking both key and value. */ + for (int i = 0; i < count; i++) + { + entry = dshash_find(ht, &i, false); + if (entry == NULL || entry->key != i || entry->value != KEY_TO_VALUE(i)) + elog(ERROR, "key %d not found or corrupted", i); + dshash_release_lock(ht, entry); + } + + /* Dump the hash table. */ + dshash_dump(ht); + + /* Try to delete a key that does not exist. */ + nonexistent_key = count + 1; + found = dshash_delete_key(ht, &nonexistent_key); + if (found) + elog(ERROR, "delete of nonexistent key %d reported found", nonexistent_key); + + /* Verify entry count via sequential scan. */ + scanned = 0; + dshash_seq_init(&status, ht, false); + while ((entry = dshash_seq_next(&status)) != NULL) + scanned++; + dshash_seq_term(&status); + + if (scanned != count) + elog(ERROR, "seq scan returned %d entries, expected %d", scanned, count); + + /* Delete one entry via dshash_delete_entry. */ + { + int delete_key = 0; + + entry = dshash_find(ht, &delete_key, true); + if (entry == NULL) + elog(ERROR, "key %d not found for delete_entry", delete_key); + dshash_delete_entry(ht, entry); + } + + /* Verify it's gone. */ + { + int delete_key = 0; + + entry = dshash_find(ht, &delete_key, false); + if (entry != NULL) + { + dshash_release_lock(ht, entry); + elog(ERROR, "key %d still present after delete_entry", delete_key); + } + } + + /* Delete remaining entries via delete_current. */ + dshash_seq_init(&status, ht, true); + while ((entry = dshash_seq_next(&status)) != NULL) + dshash_delete_current(&status); + dshash_seq_term(&status); + + /* Verify table is empty. */ + scanned = 0; + dshash_seq_init(&status, ht, false); + while ((entry = dshash_seq_next(&status)) != NULL) + scanned++; + dshash_seq_term(&status); + + if (scanned != 0) + elog(ERROR, "expected empty table, got %d entries", scanned); + + /* Re-insert to verify the table is reusable after being emptied. */ + for (int i = 0; i < count; i++) + { + entry = dshash_find_or_insert(ht, &i, &found); + if (found) + elog(ERROR, "unexpected duplicate key %d", i); + entry->value = KEY_TO_VALUE(i); + dshash_release_lock(ht, entry); + } + + dshash_destroy(ht); + dsa_detach(area); + + PG_RETURN_VOID(); +} + +/* + * test_dshash_find_or_insert_oom_error + * + * Test that the regular dshash_find_or_insert (without DSHASH_INSERT_NO_OOM) + * raises ERROR when the DSA area is "out of memory". This also exercises + * resize() along the way as entries are inserted. + */ +PG_FUNCTION_INFO_V1(test_dshash_find_or_insert_oom_error); +Datum +test_dshash_find_or_insert_oom_error(PG_FUNCTION_ARGS) +{ + int *tranche_id; + bool found; + dsa_area *area; + dshash_table *ht; + dshash_parameters params = test_params; + int key = 0; + + tranche_id = GetNamedDSMSegment("test_dshash", sizeof(int), + init_tranche, &found, NULL); + params.tranche_id = *tranche_id; + + area = dsa_create(*tranche_id); + dsa_set_size_limit(area, TEST_DSHASH_SIZE_LIMIT); + ht = dshash_create(area, ¶ms, NULL); + + /* Insert until OOM — without NO_OOM flag, this should raise ERROR. */ + for (key = 0; key < TEST_DSHASH_MAX_OOM_ITERATIONS; key++) + { + TestDshashEntry *entry; + + entry = dshash_find_or_insert(ht, &key, &found); + if (entry == NULL) + elog(ERROR, "dshash_find_or_insert returned NULL unexpectedly"); + dshash_release_lock(ht, entry); + } + + /* Should not reach here — OOM error is expected above. */ + elog(ERROR, "expected out-of-memory, but completed all %d iterations", + TEST_DSHASH_MAX_OOM_ITERATIONS); + + PG_RETURN_VOID(); +} diff --git a/src/test/modules/test_dshash/test_dshash.control b/src/test/modules/test_dshash/test_dshash.control new file mode 100644 index 00000000000..7ab0666d227 --- /dev/null +++ b/src/test/modules/test_dshash/test_dshash.control @@ -0,0 +1,4 @@ +comment = 'Test code for dshash' +default_version = '1.0' +module_pathname = '$libdir/test_dshash' +relocatable = true -- 2.47.3 ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: dshash_find_or_insert vs. OOM @ 2026-03-20 00:26 Sami Imseih <[email protected]> parent: Sami Imseih <[email protected]> 0 siblings, 1 reply; 15+ messages in thread From: Sami Imseih @ 2026-03-20 00:26 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]> I just realized v2 removed the test for dshash_find_or_insert_extended with the NO_OOM flag. Corrected in v3. -- Sami Imseih Amazon Web Services (AWS) Attachments: [application/octet-stream] v3-0001-Add-test-module-for-dshash.patch (13.0K, 2-v3-0001-Add-test-module-for-dshash.patch) download | inline diff: From 7b8c2ccb5b4744078e96f087ffc6f453d52770c9 Mon Sep 17 00:00:00 2001 From: Sami Imseih <[email protected]> Date: Thu, 19 Mar 2026 15:26:46 +0000 Subject: [PATCH v3 1/1] Add test module for dshash This exercises the core dshash APIs including insert, find, sequential scan, various delete paths, and dshash_dump. It also tests OOM behavior both with and without the DSHASH_INSERT_NO_OOM flag. Discussion:https://postgr.es/m/CAA5RZ0tWjsthTod0ODPEW4noNrSvY%2BgRWTMxiQ89-yY-k92B8Q%40mail.gmail.com --- src/test/modules/Makefile | 1 + src/test/modules/meson.build | 1 + src/test/modules/test_dshash/.gitignore | 4 + src/test/modules/test_dshash/Makefile | 23 ++ .../test_dshash/expected/test_dshash.out | 14 ++ src/test/modules/test_dshash/meson.build | 33 +++ .../modules/test_dshash/sql/test_dshash.sql | 10 + .../modules/test_dshash/test_dshash--1.0.sql | 10 + src/test/modules/test_dshash/test_dshash.c | 234 ++++++++++++++++++ .../modules/test_dshash/test_dshash.control | 4 + 10 files changed, 334 insertions(+) create mode 100644 src/test/modules/test_dshash/.gitignore create mode 100644 src/test/modules/test_dshash/Makefile create mode 100644 src/test/modules/test_dshash/expected/test_dshash.out create mode 100644 src/test/modules/test_dshash/meson.build create mode 100644 src/test/modules/test_dshash/sql/test_dshash.sql create mode 100644 src/test/modules/test_dshash/test_dshash--1.0.sql create mode 100644 src/test/modules/test_dshash/test_dshash.c create mode 100644 src/test/modules/test_dshash/test_dshash.control diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile index 28ce3b35eda..51f2b1b48c5 100644 --- a/src/test/modules/Makefile +++ b/src/test/modules/Makefile @@ -26,6 +26,7 @@ SUBDIRS = \ test_custom_types \ test_ddl_deparse \ test_dsa \ + test_dshash \ test_dsm_registry \ test_escape \ test_extensions \ diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build index 3ac291656c1..dcf813b0823 100644 --- a/src/test/modules/meson.build +++ b/src/test/modules/meson.build @@ -27,6 +27,7 @@ subdir('test_custom_stats') subdir('test_custom_types') subdir('test_ddl_deparse') subdir('test_dsa') +subdir('test_dshash') subdir('test_dsm_registry') subdir('test_escape') subdir('test_extensions') diff --git a/src/test/modules/test_dshash/.gitignore b/src/test/modules/test_dshash/.gitignore new file mode 100644 index 00000000000..5dcb3ff9723 --- /dev/null +++ b/src/test/modules/test_dshash/.gitignore @@ -0,0 +1,4 @@ +# Generated subdirectories +/log/ +/results/ +/tmp_check/ diff --git a/src/test/modules/test_dshash/Makefile b/src/test/modules/test_dshash/Makefile new file mode 100644 index 00000000000..aefba3af5fe --- /dev/null +++ b/src/test/modules/test_dshash/Makefile @@ -0,0 +1,23 @@ +# src/test/modules/test_dshash/Makefile + +MODULE_big = test_dshash +OBJS = \ + $(WIN32RES) \ + test_dshash.o +PGFILEDESC = "test_dshash - test code for dshash" + +EXTENSION = test_dshash +DATA = test_dshash--1.0.sql + +REGRESS = test_dshash + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = src/test/modules/test_dshash +top_builddir = ../../../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif diff --git a/src/test/modules/test_dshash/expected/test_dshash.out b/src/test/modules/test_dshash/expected/test_dshash.out new file mode 100644 index 00000000000..e093d6fa458 --- /dev/null +++ b/src/test/modules/test_dshash/expected/test_dshash.out @@ -0,0 +1,14 @@ +CREATE EXTENSION test_dshash; +-- Exercise core dshash operations. +SELECT test_dshash_basic(); + test_dshash_basic +------------------- + +(1 row) + +-- Exercise OOM handling: NO_OOM returns NULL, regular insert raises ERROR. +-- Use terse verbosity to ignore the DETAIL message. +\set VERBOSITY terse +SELECT test_dshash_find_or_insert_oom_error(); +ERROR: out of memory +\set VERBOSITY default diff --git a/src/test/modules/test_dshash/meson.build b/src/test/modules/test_dshash/meson.build new file mode 100644 index 00000000000..4c0ce938379 --- /dev/null +++ b/src/test/modules/test_dshash/meson.build @@ -0,0 +1,33 @@ +# Copyright (c) 2024-2026, PostgreSQL Global Development Group + +test_dshash_sources = files( + 'test_dshash.c', +) + +if host_system == 'windows' + test_dshash_sources += rc_lib_gen.process(win32ver_rc, extra_args: [ + '--NAME', 'test_dshash', + '--FILEDESC', 'test_dshash - test code for dshash',]) +endif + +test_dshash = shared_module('test_dshash', + test_dshash_sources, + kwargs: pg_test_mod_args, +) +test_install_libs += test_dshash + +test_install_data += files( + 'test_dshash.control', + 'test_dshash--1.0.sql', +) + +tests += { + 'name': 'test_dshash', + 'sd': meson.current_source_dir(), + 'bd': meson.current_build_dir(), + 'regress': { + 'sql': [ + 'test_dshash', + ], + }, +} diff --git a/src/test/modules/test_dshash/sql/test_dshash.sql b/src/test/modules/test_dshash/sql/test_dshash.sql new file mode 100644 index 00000000000..40ee3f1c39a --- /dev/null +++ b/src/test/modules/test_dshash/sql/test_dshash.sql @@ -0,0 +1,10 @@ +CREATE EXTENSION test_dshash; + +-- Exercise core dshash operations. +SELECT test_dshash_basic(); + +-- Exercise OOM handling: NO_OOM returns NULL, regular insert raises ERROR. +-- Use terse verbosity to ignore the DETAIL message. +\set VERBOSITY terse +SELECT test_dshash_find_or_insert_oom_error(); +\set VERBOSITY default diff --git a/src/test/modules/test_dshash/test_dshash--1.0.sql b/src/test/modules/test_dshash/test_dshash--1.0.sql new file mode 100644 index 00000000000..85a6289850b --- /dev/null +++ b/src/test/modules/test_dshash/test_dshash--1.0.sql @@ -0,0 +1,10 @@ +/* src/test/modules/test_dshash/test_dshash--1.0.sql */ + +-- complain if script is sourced in psql, rather than via CREATE EXTENSION +\echo Use "CREATE EXTENSION test_dshash" to load this file. \quit + +CREATE FUNCTION test_dshash_basic() RETURNS VOID + AS 'MODULE_PATHNAME' LANGUAGE C; + +CREATE FUNCTION test_dshash_find_or_insert_oom_error() RETURNS VOID + AS 'MODULE_PATHNAME' LANGUAGE C; diff --git a/src/test/modules/test_dshash/test_dshash.c b/src/test/modules/test_dshash/test_dshash.c new file mode 100644 index 00000000000..f66329f7949 --- /dev/null +++ b/src/test/modules/test_dshash/test_dshash.c @@ -0,0 +1,234 @@ +/*-------------------------------------------------------------------------- + * + * test_dshash.c + * Test dynamic shared hash tables (dshash). + * + * Copyright (c) 2024-2026, PostgreSQL Global Development Group + * + * IDENTIFICATION + * src/test/modules/test_dshash/test_dshash.c + * + * ------------------------------------------------------------------------- + */ +#include "postgres.h" + +#include "fmgr.h" +#include "lib/dshash.h" +#include "storage/dsm_registry.h" +#include "storage/lwlock.h" +#include "utils/dsa.h" + +PG_MODULE_MAGIC; + +/* Size limit for OOM tests */ +#define TEST_DSHASH_SIZE_LIMIT (128 * 1024) + +/* More than enough to exhaust TEST_DSHASH_SIZE_LIMIT */ +#define TEST_DSHASH_MAX_OOM_ITERATIONS 10000 + +typedef struct TestDshashEntry +{ + int key; + int value; +} TestDshashEntry; + +/* To verify payload integrity */ +#define KEY_TO_VALUE(k) ((k) ^ 0x12345678) + +static const dshash_parameters test_params = { + sizeof(int), /* key_size */ + sizeof(TestDshashEntry), /* entry_size */ + dshash_memcmp, + dshash_memhash, + dshash_memcpy, + LWTRANCHE_FIRST_USER_DEFINED /* tranche_id, overwritten at runtime */ +}; + +static void +init_tranche(void *ptr, void *arg) +{ + int *tranche_id = (int *) ptr; + + *tranche_id = LWLockNewTrancheId("test_dshash"); +} + +/* + * test_dshash_basic + * + * Test insert, find, sequential scan, delete_current, delete_key, + * delete of a nonexistent key, and dshash_dump, and re-insertions + * after deletes. + */ +PG_FUNCTION_INFO_V1(test_dshash_basic); +Datum +test_dshash_basic(PG_FUNCTION_ARGS) +{ + int *tranche_id; + bool found; + dsa_area *area; + dshash_table *ht; + dshash_parameters params = test_params; + dshash_seq_status status; + TestDshashEntry *entry; + int count = 10; + int scanned; + int nonexistent_key; + + tranche_id = GetNamedDSMSegment("test_dshash", sizeof(int), + init_tranche, &found, NULL); + params.tranche_id = *tranche_id; + + area = dsa_create(*tranche_id); + ht = dshash_create(area, ¶ms, NULL); + + /* Insert entries with a payload. */ + for (int i = 0; i < count; i++) + { + entry = dshash_find_or_insert(ht, &i, &found); + if (found) + elog(ERROR, "unexpected duplicate key %d", i); + entry->value = KEY_TO_VALUE(i); + dshash_release_lock(ht, entry); + } + + /* Verify all entries via find, checking both key and value. */ + for (int i = 0; i < count; i++) + { + entry = dshash_find(ht, &i, false); + if (entry == NULL || entry->key != i || entry->value != KEY_TO_VALUE(i)) + elog(ERROR, "key %d not found or corrupted", i); + dshash_release_lock(ht, entry); + } + + /* Dump the hash table. */ + dshash_dump(ht); + + /* Try to delete a key that does not exist. */ + nonexistent_key = count + 1; + found = dshash_delete_key(ht, &nonexistent_key); + if (found) + elog(ERROR, "delete of nonexistent key %d reported found", nonexistent_key); + + /* Verify entry count via sequential scan. */ + scanned = 0; + dshash_seq_init(&status, ht, false); + while ((entry = dshash_seq_next(&status)) != NULL) + scanned++; + dshash_seq_term(&status); + + if (scanned != count) + elog(ERROR, "seq scan returned %d entries, expected %d", scanned, count); + + /* Delete one entry via dshash_delete_entry. */ + { + int delete_key = 0; + + entry = dshash_find(ht, &delete_key, true); + if (entry == NULL) + elog(ERROR, "key %d not found for delete_entry", delete_key); + dshash_delete_entry(ht, entry); + } + + /* Verify it's gone. */ + { + int delete_key = 0; + + entry = dshash_find(ht, &delete_key, false); + if (entry != NULL) + { + dshash_release_lock(ht, entry); + elog(ERROR, "key %d still present after delete_entry", delete_key); + } + } + + /* Delete remaining entries via delete_current. */ + dshash_seq_init(&status, ht, true); + while ((entry = dshash_seq_next(&status)) != NULL) + dshash_delete_current(&status); + dshash_seq_term(&status); + + /* Verify table is empty. */ + scanned = 0; + dshash_seq_init(&status, ht, false); + while ((entry = dshash_seq_next(&status)) != NULL) + scanned++; + dshash_seq_term(&status); + + if (scanned != 0) + elog(ERROR, "expected empty table, got %d entries", scanned); + + /* Re-insert to verify the table is reusable after being emptied. */ + for (int i = 0; i < count; i++) + { + entry = dshash_find_or_insert(ht, &i, &found); + if (found) + elog(ERROR, "unexpected duplicate key %d", i); + entry->value = KEY_TO_VALUE(i); + dshash_release_lock(ht, entry); + } + + dshash_destroy(ht); + dsa_detach(area); + + PG_RETURN_VOID(); +} + +/* + * test_dshash_find_or_insert_oom_error + * + * First, fill the hash table using DSHASH_INSERT_NO_OOM until OOM is hit + * and handled gracefully. Then, insert without the flag to verify that OOM + * raises ERROR. This also exercises resize() along the way. + */ +PG_FUNCTION_INFO_V1(test_dshash_find_or_insert_oom_error); +Datum +test_dshash_find_or_insert_oom_error(PG_FUNCTION_ARGS) +{ + int *tranche_id; + bool found; + dsa_area *area; + dshash_table *ht; + dshash_parameters params = test_params; + int key = 0; + + tranche_id = GetNamedDSMSegment("test_dshash", sizeof(int), + init_tranche, &found, NULL); + params.tranche_id = *tranche_id; + + area = dsa_create(*tranche_id); + dsa_set_size_limit(area, TEST_DSHASH_SIZE_LIMIT); + ht = dshash_create(area, ¶ms, NULL); + + /* Insert until OOM — with NO_OOM flag, returns NULL instead of ERROR. */ + for (key = 0; key < TEST_DSHASH_MAX_OOM_ITERATIONS; key++) + { + TestDshashEntry *entry; + + entry = dshash_find_or_insert_extended(ht, &key, &found, + DSHASH_INSERT_NO_OOM); + if (entry == NULL) + break; + dshash_release_lock(ht, entry); + } + + if (key >= TEST_DSHASH_MAX_OOM_ITERATIONS) + elog(ERROR, "expected out-of-memory, but completed all %d iterations", + TEST_DSHASH_MAX_OOM_ITERATIONS); + + /* Insert without NO_OOM flag — this should raise ERROR. */ + for (key = 0; key < TEST_DSHASH_MAX_OOM_ITERATIONS; key++) + { + TestDshashEntry *entry; + + entry = dshash_find_or_insert(ht, &key, &found); + if (entry == NULL) + elog(ERROR, "dshash_find_or_insert returned NULL unexpectedly"); + dshash_release_lock(ht, entry); + } + + /* Should not reach here — OOM error is expected above. */ + elog(ERROR, "expected out-of-memory, but completed all %d iterations", + TEST_DSHASH_MAX_OOM_ITERATIONS); + + PG_RETURN_VOID(); +} diff --git a/src/test/modules/test_dshash/test_dshash.control b/src/test/modules/test_dshash/test_dshash.control new file mode 100644 index 00000000000..7ab0666d227 --- /dev/null +++ b/src/test/modules/test_dshash/test_dshash.control @@ -0,0 +1,4 @@ +comment = 'Test code for dshash' +default_version = '1.0' +module_pathname = '$libdir/test_dshash' +relocatable = true -- 2.47.3 ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: dshash_find_or_insert vs. OOM @ 2026-03-25 17:55 Robert Haas <[email protected]> parent: Sami Imseih <[email protected]> 0 siblings, 2 replies; 15+ messages in thread From: Robert Haas @ 2026-03-25 17:55 UTC (permalink / raw) To: Sami Imseih <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]> On Thu, Mar 19, 2026 at 8:26 PM Sami Imseih <[email protected]> wrote: > I just realized v2 removed the test for dshash_find_or_insert_extended > with the NO_OOM flag. Corrected in v3. You need to get rid of the bare blocks. If you happen to be using an AI to help write your code, I definitely suggest telling it to remember not to ever do that. This verifies that dshash_dump() doesn't crash, but not that it produces useful output. Whether that's worth the infrastructure, I don't know. But more generally, I wonder what people think about how much value this kind of thing has. With just the core regression test suite, we get coverage of 76.5% of the lines in dshash.c. Adding the isolation test suite and the test_dsm_registry test suite brings coverage up to 81.9% (334 of 408). The things that are not covered are: OOM cases, dshash_destroy(), dshash_dump(), concurrency case in resizing. delete_key_from_bucket() loop iterating more than once, delete_item_from_bucket() returning false. With the v3 patch, running just the test_dshash suite, coverage rises to 93.4% of lines (381 of 408). The omissions are: some OOM cases, dshash_delete_bucket() successfully deletes something, concurrency case in resizing, most of delete_key_from_bucket(), delete_item_from_bucket() iterating more than once or returning false. Combining your patch with the test suite previously mentioned, we get up to 97.1% coverage by lines (396 of 408). So the patch definitely has some value: it adds coverage of 60+ lines of code. On the other hand, it still feels to me like we'd be far more likely to notice new dshash bugs based on its existing usages than because of this. If I were modifying dshash, I wouldn't run this test suite first; I'd run the regression tests first. And the patch does have a cost: it's 334 lines of code that will have to be maintained going forward. I don't know if that's worth it. I feel like we're a lot more likely to break dshash behavior in some complicated concurrency scenario than we are to break it in a test like this. And, if somebody modifies dshash_destory() or dshash_dump(), they just need to test the code at least once before committing to get essentially the same benefit we'd get from this, which you would hope people would do anyway. None of this is to say that this patch is a horrible idea, but I'm also not entirely convinced that it is an excellent idea, so I would like to hear some other opinions. Of course, there's also the fact that this patch is quite similar to some other test_* modules that we already have, like test_dsa or test_bitmapset. So maybe those are good and this is also good. But I'm not sure. The good news is, if we do want this, it probably doesn't need much more work to be committable. Thanks, -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: dshash_find_or_insert vs. OOM @ 2026-03-25 21:52 Sami Imseih <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 0 replies; 15+ messages in thread From: Sami Imseih @ 2026-03-25 21:52 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]> > You need to get rid of the bare blocks. done in v4 > This verifies that dshash_dump() doesn't crash, but not that it > produces useful output. Whether that's worth the infrastructure, I > don't know. I don't think dshash_dump() will be deterministic, as keys may hash to different buckets on different platforms, perhaps. Which is why I did not think it was a good idea to compare the output from the logs. Testing that it does not crash seemed worthwhile to increase the coverage. > 408). The omissions are: some OOM cases, dshash_delete_bucket() > successfully deletes something, concurrency case in resizing, most of > delete_key_from_bucket(), delete_item_from_bucket() iterating more Yes, we can probably address some of these things with more complexity. > than once or returning false. Combining your patch with the test suite > previously mentioned, we get up to 97.1% coverage by lines (396 of > 408). This is solid coverage > So the patch definitely has some value: it adds coverage of 60+ lines > of code. On the other hand, it still feels to me like we'd be far more > likely to notice new dshash bugs based on its existing usages than > because of this. If I were modifying dshash, I wouldn't run this test > suite first; I'd run the regression tests first. Sure, but once a bug is fixed, having direct test coverage fo this bug in a dedicated test suite is a good thing. The NO_OOM flag that started this discussion would not fit neatly in other test suites, AFAICT. -- Sami Attachments: [application/octet-stream] v4-0001-Add-test-module-for-dshash.patch (12.9K, 2-v4-0001-Add-test-module-for-dshash.patch) download | inline diff: From 930c22e7efdccc0ab89a39710cef90a014a94d6e Mon Sep 17 00:00:00 2001 From: Sami Imseih <[email protected]> Date: Thu, 19 Mar 2026 15:26:46 +0000 Subject: [PATCH v4 1/1] Add test module for dshash This exercises the core dshash APIs including insert, find, sequential scan, various delete paths, and dshash_dump. It also tests OOM behavior both with and without the DSHASH_INSERT_NO_OOM flag. Discussion:https://postgr.es/m/CAA5RZ0tWjsthTod0ODPEW4noNrSvY%2BgRWTMxiQ89-yY-k92B8Q%40mail.gmail.com --- src/test/modules/Makefile | 1 + src/test/modules/meson.build | 1 + src/test/modules/test_dshash/.gitignore | 4 + src/test/modules/test_dshash/Makefile | 23 ++ .../test_dshash/expected/test_dshash.out | 14 ++ src/test/modules/test_dshash/meson.build | 33 +++ .../modules/test_dshash/sql/test_dshash.sql | 10 + .../modules/test_dshash/test_dshash--1.0.sql | 10 + src/test/modules/test_dshash/test_dshash.c | 228 ++++++++++++++++++ .../modules/test_dshash/test_dshash.control | 4 + 10 files changed, 328 insertions(+) create mode 100644 src/test/modules/test_dshash/.gitignore create mode 100644 src/test/modules/test_dshash/Makefile create mode 100644 src/test/modules/test_dshash/expected/test_dshash.out create mode 100644 src/test/modules/test_dshash/meson.build create mode 100644 src/test/modules/test_dshash/sql/test_dshash.sql create mode 100644 src/test/modules/test_dshash/test_dshash--1.0.sql create mode 100644 src/test/modules/test_dshash/test_dshash.c create mode 100644 src/test/modules/test_dshash/test_dshash.control diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile index 28ce3b35eda..51f2b1b48c5 100644 --- a/src/test/modules/Makefile +++ b/src/test/modules/Makefile @@ -26,6 +26,7 @@ SUBDIRS = \ test_custom_types \ test_ddl_deparse \ test_dsa \ + test_dshash \ test_dsm_registry \ test_escape \ test_extensions \ diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build index 3ac291656c1..dcf813b0823 100644 --- a/src/test/modules/meson.build +++ b/src/test/modules/meson.build @@ -27,6 +27,7 @@ subdir('test_custom_stats') subdir('test_custom_types') subdir('test_ddl_deparse') subdir('test_dsa') +subdir('test_dshash') subdir('test_dsm_registry') subdir('test_escape') subdir('test_extensions') diff --git a/src/test/modules/test_dshash/.gitignore b/src/test/modules/test_dshash/.gitignore new file mode 100644 index 00000000000..5dcb3ff9723 --- /dev/null +++ b/src/test/modules/test_dshash/.gitignore @@ -0,0 +1,4 @@ +# Generated subdirectories +/log/ +/results/ +/tmp_check/ diff --git a/src/test/modules/test_dshash/Makefile b/src/test/modules/test_dshash/Makefile new file mode 100644 index 00000000000..aefba3af5fe --- /dev/null +++ b/src/test/modules/test_dshash/Makefile @@ -0,0 +1,23 @@ +# src/test/modules/test_dshash/Makefile + +MODULE_big = test_dshash +OBJS = \ + $(WIN32RES) \ + test_dshash.o +PGFILEDESC = "test_dshash - test code for dshash" + +EXTENSION = test_dshash +DATA = test_dshash--1.0.sql + +REGRESS = test_dshash + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = src/test/modules/test_dshash +top_builddir = ../../../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif diff --git a/src/test/modules/test_dshash/expected/test_dshash.out b/src/test/modules/test_dshash/expected/test_dshash.out new file mode 100644 index 00000000000..e093d6fa458 --- /dev/null +++ b/src/test/modules/test_dshash/expected/test_dshash.out @@ -0,0 +1,14 @@ +CREATE EXTENSION test_dshash; +-- Exercise core dshash operations. +SELECT test_dshash_basic(); + test_dshash_basic +------------------- + +(1 row) + +-- Exercise OOM handling: NO_OOM returns NULL, regular insert raises ERROR. +-- Use terse verbosity to ignore the DETAIL message. +\set VERBOSITY terse +SELECT test_dshash_find_or_insert_oom_error(); +ERROR: out of memory +\set VERBOSITY default diff --git a/src/test/modules/test_dshash/meson.build b/src/test/modules/test_dshash/meson.build new file mode 100644 index 00000000000..4c0ce938379 --- /dev/null +++ b/src/test/modules/test_dshash/meson.build @@ -0,0 +1,33 @@ +# Copyright (c) 2024-2026, PostgreSQL Global Development Group + +test_dshash_sources = files( + 'test_dshash.c', +) + +if host_system == 'windows' + test_dshash_sources += rc_lib_gen.process(win32ver_rc, extra_args: [ + '--NAME', 'test_dshash', + '--FILEDESC', 'test_dshash - test code for dshash',]) +endif + +test_dshash = shared_module('test_dshash', + test_dshash_sources, + kwargs: pg_test_mod_args, +) +test_install_libs += test_dshash + +test_install_data += files( + 'test_dshash.control', + 'test_dshash--1.0.sql', +) + +tests += { + 'name': 'test_dshash', + 'sd': meson.current_source_dir(), + 'bd': meson.current_build_dir(), + 'regress': { + 'sql': [ + 'test_dshash', + ], + }, +} diff --git a/src/test/modules/test_dshash/sql/test_dshash.sql b/src/test/modules/test_dshash/sql/test_dshash.sql new file mode 100644 index 00000000000..40ee3f1c39a --- /dev/null +++ b/src/test/modules/test_dshash/sql/test_dshash.sql @@ -0,0 +1,10 @@ +CREATE EXTENSION test_dshash; + +-- Exercise core dshash operations. +SELECT test_dshash_basic(); + +-- Exercise OOM handling: NO_OOM returns NULL, regular insert raises ERROR. +-- Use terse verbosity to ignore the DETAIL message. +\set VERBOSITY terse +SELECT test_dshash_find_or_insert_oom_error(); +\set VERBOSITY default diff --git a/src/test/modules/test_dshash/test_dshash--1.0.sql b/src/test/modules/test_dshash/test_dshash--1.0.sql new file mode 100644 index 00000000000..85a6289850b --- /dev/null +++ b/src/test/modules/test_dshash/test_dshash--1.0.sql @@ -0,0 +1,10 @@ +/* src/test/modules/test_dshash/test_dshash--1.0.sql */ + +-- complain if script is sourced in psql, rather than via CREATE EXTENSION +\echo Use "CREATE EXTENSION test_dshash" to load this file. \quit + +CREATE FUNCTION test_dshash_basic() RETURNS VOID + AS 'MODULE_PATHNAME' LANGUAGE C; + +CREATE FUNCTION test_dshash_find_or_insert_oom_error() RETURNS VOID + AS 'MODULE_PATHNAME' LANGUAGE C; diff --git a/src/test/modules/test_dshash/test_dshash.c b/src/test/modules/test_dshash/test_dshash.c new file mode 100644 index 00000000000..e4ff5216ee7 --- /dev/null +++ b/src/test/modules/test_dshash/test_dshash.c @@ -0,0 +1,228 @@ +/*-------------------------------------------------------------------------- + * + * test_dshash.c + * Test dynamic shared hash tables (dshash). + * + * Copyright (c) 2024-2026, PostgreSQL Global Development Group + * + * IDENTIFICATION + * src/test/modules/test_dshash/test_dshash.c + * + * ------------------------------------------------------------------------- + */ +#include "postgres.h" + +#include "fmgr.h" +#include "lib/dshash.h" +#include "storage/dsm_registry.h" +#include "storage/lwlock.h" +#include "utils/dsa.h" + +PG_MODULE_MAGIC; + +/* Size limit for OOM tests */ +#define TEST_DSHASH_SIZE_LIMIT (128 * 1024) + +/* More than enough to exhaust TEST_DSHASH_SIZE_LIMIT */ +#define TEST_DSHASH_MAX_OOM_ITERATIONS 10000 + +typedef struct TestDshashEntry +{ + int key; + int value; +} TestDshashEntry; + +/* To verify payload integrity */ +#define KEY_TO_VALUE(k) ((k) ^ 0x12345678) + +static const dshash_parameters test_params = { + sizeof(int), /* key_size */ + sizeof(TestDshashEntry), /* entry_size */ + dshash_memcmp, + dshash_memhash, + dshash_memcpy, + LWTRANCHE_FIRST_USER_DEFINED /* tranche_id, overwritten at runtime */ +}; + +static void +init_tranche(void *ptr, void *arg) +{ + int *tranche_id = (int *) ptr; + + *tranche_id = LWLockNewTrancheId("test_dshash"); +} + +/* + * test_dshash_basic + * + * Test insert, find, sequential scan, delete_current, delete_key, + * delete of a nonexistent key, and dshash_dump, and re-insertions + * after deletes. + */ +PG_FUNCTION_INFO_V1(test_dshash_basic); +Datum +test_dshash_basic(PG_FUNCTION_ARGS) +{ + int *tranche_id; + bool found; + dsa_area *area; + dshash_table *ht; + dshash_parameters params = test_params; + dshash_seq_status status; + TestDshashEntry *entry; + int count = 10; + int delete_key; + int scanned; + int nonexistent_key; + + tranche_id = GetNamedDSMSegment("test_dshash", sizeof(int), + init_tranche, &found, NULL); + params.tranche_id = *tranche_id; + + area = dsa_create(*tranche_id); + ht = dshash_create(area, ¶ms, NULL); + + /* Insert entries with a payload. */ + for (int i = 0; i < count; i++) + { + entry = dshash_find_or_insert(ht, &i, &found); + if (found) + elog(ERROR, "unexpected duplicate key %d", i); + entry->value = KEY_TO_VALUE(i); + dshash_release_lock(ht, entry); + } + + /* Verify all entries via find, checking both key and value. */ + for (int i = 0; i < count; i++) + { + entry = dshash_find(ht, &i, false); + if (entry == NULL || entry->key != i || entry->value != KEY_TO_VALUE(i)) + elog(ERROR, "key %d not found or corrupted", i); + dshash_release_lock(ht, entry); + } + + /* Dump the hash table. */ + dshash_dump(ht); + + /* Try to delete a key that does not exist. */ + nonexistent_key = count + 1; + found = dshash_delete_key(ht, &nonexistent_key); + if (found) + elog(ERROR, "delete of nonexistent key %d reported found", nonexistent_key); + + /* Verify entry count via sequential scan. */ + scanned = 0; + dshash_seq_init(&status, ht, false); + while ((entry = dshash_seq_next(&status)) != NULL) + scanned++; + dshash_seq_term(&status); + + if (scanned != count) + elog(ERROR, "seq scan returned %d entries, expected %d", scanned, count); + + /* Delete one entry via dshash_delete_entry. */ + delete_key = 0; + entry = dshash_find(ht, &delete_key, true); + if (entry == NULL) + elog(ERROR, "key %d not found for delete_entry", delete_key); + dshash_delete_entry(ht, entry); + + /* Verify it's gone. */ + entry = dshash_find(ht, &delete_key, false); + if (entry != NULL) + { + dshash_release_lock(ht, entry); + elog(ERROR, "key %d still present after delete_entry", delete_key); + } + + /* Delete remaining entries via delete_current. */ + dshash_seq_init(&status, ht, true); + while ((entry = dshash_seq_next(&status)) != NULL) + dshash_delete_current(&status); + dshash_seq_term(&status); + + /* Verify table is empty. */ + scanned = 0; + dshash_seq_init(&status, ht, false); + while ((entry = dshash_seq_next(&status)) != NULL) + scanned++; + dshash_seq_term(&status); + + if (scanned != 0) + elog(ERROR, "expected empty table, got %d entries", scanned); + + /* Re-insert to verify the table is reusable after being emptied. */ + for (int i = 0; i < count; i++) + { + entry = dshash_find_or_insert(ht, &i, &found); + if (found) + elog(ERROR, "unexpected duplicate key %d", i); + entry->value = KEY_TO_VALUE(i); + dshash_release_lock(ht, entry); + } + + dshash_destroy(ht); + dsa_detach(area); + + PG_RETURN_VOID(); +} + +/* + * test_dshash_find_or_insert_oom_error + * + * First, fill the hash table using DSHASH_INSERT_NO_OOM until OOM is hit + * and handled gracefully. Then, insert without the flag to verify that OOM + * raises ERROR. This also exercises resize() along the way. + */ +PG_FUNCTION_INFO_V1(test_dshash_find_or_insert_oom_error); +Datum +test_dshash_find_or_insert_oom_error(PG_FUNCTION_ARGS) +{ + int *tranche_id; + bool found; + dsa_area *area; + dshash_table *ht; + dshash_parameters params = test_params; + int key = 0; + + tranche_id = GetNamedDSMSegment("test_dshash", sizeof(int), + init_tranche, &found, NULL); + params.tranche_id = *tranche_id; + + area = dsa_create(*tranche_id); + dsa_set_size_limit(area, TEST_DSHASH_SIZE_LIMIT); + ht = dshash_create(area, ¶ms, NULL); + + /* Insert until OOM — with NO_OOM flag, returns NULL instead of ERROR. */ + for (key = 0; key < TEST_DSHASH_MAX_OOM_ITERATIONS; key++) + { + TestDshashEntry *entry; + + entry = dshash_find_or_insert_extended(ht, &key, &found, + DSHASH_INSERT_NO_OOM); + if (entry == NULL) + break; + dshash_release_lock(ht, entry); + } + + if (key >= TEST_DSHASH_MAX_OOM_ITERATIONS) + elog(ERROR, "expected out-of-memory, but completed all %d iterations", + TEST_DSHASH_MAX_OOM_ITERATIONS); + + /* Insert without NO_OOM flag — this should raise ERROR. */ + for (key = 0; key < TEST_DSHASH_MAX_OOM_ITERATIONS; key++) + { + TestDshashEntry *entry; + + entry = dshash_find_or_insert(ht, &key, &found); + if (entry == NULL) + elog(ERROR, "dshash_find_or_insert returned NULL unexpectedly"); + dshash_release_lock(ht, entry); + } + + /* Should not reach here — OOM error is expected above. */ + elog(ERROR, "expected out-of-memory, but completed all %d iterations", + TEST_DSHASH_MAX_OOM_ITERATIONS); + + PG_RETURN_VOID(); +} diff --git a/src/test/modules/test_dshash/test_dshash.control b/src/test/modules/test_dshash/test_dshash.control new file mode 100644 index 00000000000..7ab0666d227 --- /dev/null +++ b/src/test/modules/test_dshash/test_dshash.control @@ -0,0 +1,4 @@ +comment = 'Test code for dshash' +default_version = '1.0' +module_pathname = '$libdir/test_dshash' +relocatable = true -- 2.47.3 ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: dshash_find_or_insert vs. OOM @ 2026-03-26 20:26 Andres Freund <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 1 reply; 15+ messages in thread From: Andres Freund @ 2026-03-26 20:26 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Sami Imseih <[email protected]>; PostgreSQL Hackers <[email protected]>; Thomas Munro <[email protected]> Hi, On 2026-03-25 13:55:02 -0400, Robert Haas wrote: > So the patch definitely has some value: it adds coverage of 60+ lines > of code. On the other hand, it still feels to me like we'd be far more > likely to notice new dshash bugs based on its existing usages than > because of this. If I were modifying dshash, I wouldn't run this test > suite first; I'd run the regression tests first. And the patch does > have a cost: it's 334 lines of code that will have to be maintained > going forward. I don't know if that's worth it. I feel like we're a > lot more likely to break dshash behavior in some complicated > concurrency scenario than we are to break it in a test like this. And, > if somebody modifies dshash_destory() or dshash_dump(), they just need > to test the code at least once before committing to get essentially > the same benefit we'd get from this, which you would hope people would > do anyway. None of this is to say that this patch is a horrible idea, > but I'm also not entirely convinced that it is an excellent idea, so I > would like to hear some other opinions. > > Of course, there's also the fact that this patch is quite similar to > some other test_* modules that we already have, like test_dsa or > test_bitmapset. So maybe those are good and this is also good. But I'm > not sure. The good news is, if we do want this, it probably doesn't > need much more work to be committable. I think tests like this do have value and I'd definitely run them first while hacking on code related to dshash, rather than relying on the regression tests or such. E.g. having test_aio was invaluable to being able to get AIO into a stable state. When hacking on something with complicated edge cases I'd just add a test for it, making development faster as well as ensuring the complicated case continues to work into the future. However, creating its own test module for small parts of the codebase doesn't quite make sense to me. A pretty decent chunk of the test is just boilerplate to add a new module, and every new test module requires its own cluster, which adds a fair bit of runtime overhead, particularly on windows. I think test_dsa, test_dsm_registry, test_dshash should just be one combined test module, they're testing quite closely related code. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: dshash_find_or_insert vs. OOM @ 2026-03-26 23:32 Michael Paquier <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 1 reply; 15+ messages in thread From: Michael Paquier @ 2026-03-26 23:32 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; Sami Imseih <[email protected]>; PostgreSQL Hackers <[email protected]>; Thomas Munro <[email protected]> On Thu, Mar 26, 2026 at 04:26:33PM -0400, Andres Freund wrote: > I think tests like this do have value and I'd definitely run them first while > hacking on code related to dshash, rather than relying on the regression tests > or such. E.g. having test_aio was invaluable to being able to get AIO into a > stable state. When hacking on something with complicated edge cases I'd just > add a test for it, making development faster as well as ensuring the > complicated case continues to work into the future. These test modules have a lot of value because they are cheap to run and are very usually able to reproduce edge cases that no other place of the code tree would be able to reach in a predictible way. Cheap, fast and reliable is good. On top of that they can serve as code template. Bonus points. > However, creating its own test module for small parts of the codebase doesn't > quite make sense to me. A pretty decent chunk of the test is just boilerplate > to add a new module, and every new test module requires its own cluster, which > adds a fair bit of runtime overhead, particularly on windows. I think > test_dsa, test_dsm_registry, test_dshash should just be one combined test > module, they're testing quite closely related code. Yeah, perhaps grouping all the DSA things into a single module would be OK, with a parallel schedule that would speed up things. It depends on the complexity and the size of the module to me. Saying that, I think that the shape of the proposed test_dshash is wrong: it proposes one SQL function that does a bunch of dshash-related operations in a single function call, in a random manner. We have a shared memory state that can survive across SQL calls, making it a set of thinner SQL function that wrap directly dshash calls able to manipulate the table would feel much more natural to me. And it would be easier to design edge cases in the SQL scripts themselves. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, 2-signature.asc) download ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: dshash_find_or_insert vs. OOM @ 2026-04-07 17:00 Sami Imseih <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 15+ messages in thread From: Sami Imseih @ 2026-04-07 17:00 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Robert Haas <[email protected]>; PostgreSQL Hackers <[email protected]>; Thomas Munro <[email protected]> > On Thu, Mar 26, 2026 at 04:26:33PM -0400, Andres Freund wrote: > > I think tests like this do have value and I'd definitely run them first while > > hacking on code related to dshash, rather than relying on the regression tests > > or such. E.g. having test_aio was invaluable to being able to get AIO into a > > stable state. When hacking on something with complicated edge cases I'd just > > add a test for it, making development faster as well as ensuring the > > complicated case continues to work into the future. > > These test modules have a lot of value because they are cheap to run > and are very usually able to reproduce edge cases that no other place > of the code tree would be able to reach in a predictible way. Cheap, > fast and reliable is good. On top of that they can serve as code > template. Bonus points. > > > However, creating its own test module for small parts of the codebase doesn't > > quite make sense to me. A pretty decent chunk of the test is just boilerplate > > to add a new module, and every new test module requires its own cluster, which > > adds a fair bit of runtime overhead, particularly on windows. I think > > test_dsa, test_dsm_registry, test_dshash should just be one combined test > > module, they're testing quite closely related code. > > Yeah, perhaps grouping all the DSA things into a single module would > be OK, with a parallel schedule that would speed up things. It > depends on the complexity and the size of the module to me. > > Saying that, I think that the shape of the proposed test_dshash is > wrong: it proposes one SQL function that does a bunch of > dshash-related operations in a single function call, in a random > manner. We have a shared memory state that can survive across SQL > calls, making it a set of thinner SQL function that wrap directly > dshash calls able to manipulate the table would feel much more natural > to me. And it would be easier to design edge cases in the SQL > scripts themselves. My apologies for the late response here. I spent some time looking at this yesterday and came to the conclusion that we can add dshash tests to test_dsm_registry, which already allocates a dshash via GetNamedDSHash(). However, I also realized that the API has a gap: callers cannot set a size limit on the dshash. I need this for the test, but more importantly it's a limitation of the API itself. So I plan to target v20 for the tests, as it's likely too late for v19. To start, I've submitted a patch for allowing callers to set a size limit on a GetNamedDSHash()-allocated dshash [1]. [1] https://commitfest.postgresql.org/patch/6655/ -- Sami ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: dshash_find_or_insert vs. OOM @ 2026-04-08 01:04 Michael Paquier <[email protected]> parent: Sami Imseih <[email protected]> 0 siblings, 0 replies; 15+ messages in thread From: Michael Paquier @ 2026-04-08 01:04 UTC (permalink / raw) To: Sami Imseih <[email protected]>; +Cc: Andres Freund <[email protected]>; Robert Haas <[email protected]>; PostgreSQL Hackers <[email protected]>; Thomas Munro <[email protected]> On Tue, Apr 07, 2026 at 12:00:14PM -0500, Sami Imseih wrote: > My apologies for the late response here. I spent some time looking at > this yesterday and came to the conclusion that we can add dshash tests > to test_dsm_registry, which already allocates a dshash via > GetNamedDSHash(). However, I also realized that the API has a gap: callers > cannot set a size limit on the dshash. I need this for the test, but > more importantly it's a limitation of the API itself. As in dsa_set_size_limit() cannot be set for a dshash currently. Indeed that could be independently useful for some use cases. > So I plan to > target v20 for the tests, as it's likely too late for v19. To start, I've > submitted a patch for allowing callers to set a size limit on a > GetNamedDSHash()-allocated dshash [1]. > > [1] https://commitfest.postgresql.org/patch/6655/ That's too late for v19, at least here. Thanks for pursuing this work. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, 2-signature.asc) download ^ permalink raw reply [nested|flat] 15+ messages in thread
end of thread, other threads:[~2026-04-08 01:04 UTC | newest] Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-03-17 23:34 dshash_find_or_insert vs. OOM Robert Haas <[email protected]> 2026-03-18 15:20 ` Sami Imseih <[email protected]> 2026-03-18 18:42 ` Robert Haas <[email protected]> 2026-03-19 19:15 ` Sami Imseih <[email protected]> 2026-03-20 00:26 ` Sami Imseih <[email protected]> 2026-03-25 17:55 ` Robert Haas <[email protected]> 2026-03-25 21:52 ` Sami Imseih <[email protected]> 2026-03-26 20:26 ` Andres Freund <[email protected]> 2026-03-26 23:32 ` Michael Paquier <[email protected]> 2026-04-07 17:00 ` Sami Imseih <[email protected]> 2026-04-08 01:04 ` Michael Paquier <[email protected]> 2026-03-18 17:46 ` Robert Haas <[email protected]> 2026-03-18 18:26 ` Sami Imseih <[email protected]> 2026-03-19 00:08 ` Chao Li <[email protected]> 2026-03-19 16:26 ` Robert Haas <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox