public inbox for [email protected]
help / color / mirror / Atom feedFrom: Sami Imseih <[email protected]>
To: Robert Haas <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Thomas Munro <[email protected]>
Subject: Re: dshash_find_or_insert vs. OOM
Date: Wed, 25 Mar 2026 16:52:54 -0500
Message-ID: <CAA5RZ0uUPCnTgKs0KPmt8ieR-K-wtVEOeT5wPjKeT=KFAfnBQw@mail.gmail.com> (raw)
In-Reply-To: <CA+TgmoYmzRbNjx8sbm_0Hk-VtLBtJPe=juOUSFkm7ge4Xy7hYw@mail.gmail.com>
References: <CA+TgmoaJwUukUZGu7_yL74oMTQQz2=zqucMhF9+9xBmSC5us1w@mail.gmail.com>
<CAA5RZ0vNVdBnm_TSCR5K6fT7yc7-jQuneNUYFxFgS7hQnn62sA@mail.gmail.com>
<CA+TgmoZ3cd+wh1vz8gB_iD9zxZX558xUERaPm7G-M8wa7p9GNg@mail.gmail.com>
<CAA5RZ0tWjsthTod0ODPEW4noNrSvY+gRWTMxiQ89-yY-k92B8Q@mail.gmail.com>
<CAA5RZ0uhz2uEtUV1KqoCHKQY4U3pgB6N1n-oR-gy8TPUoenQSA@mail.gmail.com>
<CA+TgmoYmzRbNjx8sbm_0Hk-VtLBtJPe=juOUSFkm7ge4Xy7hYw@mail.gmail.com>
> 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
view thread (15+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: dshash_find_or_insert vs. OOM
In-Reply-To: <CAA5RZ0uUPCnTgKs0KPmt8ieR-K-wtVEOeT5wPjKeT=KFAfnBQw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox