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: Thu, 19 Mar 2026 14:15:45 -0500
Message-ID: <CAA5RZ0tWjsthTod0ODPEW4noNrSvY+gRWTMxiQ89-yY-k92B8Q@mail.gmail.com> (raw)
In-Reply-To: <CA+TgmoZ3cd+wh1vz8gB_iD9zxZX558xUERaPm7G-M8wa7p9GNg@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>
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
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: <CAA5RZ0tWjsthTod0ODPEW4noNrSvY+gRWTMxiQ89-yY-k92B8Q@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