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, 18 Mar 2026 10:20:56 -0500
Message-ID: <CAA5RZ0vNVdBnm_TSCR5K6fT7yc7-jQuneNUYFxFgS7hQnn62sA@mail.gmail.com> (raw)
In-Reply-To: <CA+TgmoaJwUukUZGu7_yL74oMTQQz2=zqucMhF9+9xBmSC5us1w@mail.gmail.com>
References: <CA+TgmoaJwUukUZGu7_yL74oMTQQz2=zqucMhF9+9xBmSC5us1w@mail.gmail.com>
> 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
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: <CAA5RZ0vNVdBnm_TSCR5K6fT7yc7-jQuneNUYFxFgS7hQnn62sA@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