agora inbox for pgsql-bugs@postgresql.org
help / color / mirror / Atom feedSIGSEGV in dynahash
6+ messages / 5 participants
[nested] [flat]
* SIGSEGV in dynahash
@ 2026-08-15 13:06 Konstantin Knizhnik <knizhnik@garret.ru>
0 siblings, 2 replies; 6+ messages in thread
From: Konstantin Knizhnik @ 2026-08-15 13:06 UTC (permalink / raw)
To: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On PG19,|ShmemInitHash|always builds afixed-sizeshared hash with abump
allocator(|ShmemHashAlloc|) whose|alloc_arg|is astack-localregion used
only during|hash_create|. After init, that pointer is dead.
In|hash_search|, for every|HASH_ENTER|/|HASH_ENTER_NULL|, dynahash does
thisbeforelookup:
dynahash.cLines927-937
if(action ==HASH_ENTER ||action ==HASH_ENTER_NULL)
{
if(hctl->freeList[0].nentries>(int64)hctl->max_bucket&&
!IS_PARTITIONED(hctl)&&!hashp->frozen&&
!has_seq_scans(hashp))
(void)expand_table(hashp);
}
It may cause SIGSEGV in case of using HASH_ENTER_NULL:
hash_search(HASH_ENTER_NULL)
→ expand_table → seg_alloc → SIGSEGV in libc (MemSet/alloc)
Pre-PG19, shared hashes used|ShmemAllocNoError|from the global pool, so
a failed grow tended to return|NULL|/ error instead of faulting on a
dead bump allocator.
It was introduced by commit 9fe9ecd516b — Allocate all parts of shmem
hash table from a single contiguous area
Patch preventing extension of fixed dynahash is attached.
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index dc7ae64a5a9..727adfccf2d 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -927,11 +927,13 @@ hash_search_with_hash_value(HTAB *hashp,
if (action == HASH_ENTER || action == HASH_ENTER_NULL)
{
/*
- * Can't split if running in partitioned mode, nor if frozen, nor if
+ * Can't split if running in partitioned mode, nor if frozen,
+ * nor if fixed (in shared memory), nor if
* table is the subject of any active hash_seq_search scans.
*/
if (hctl->freeList[0].nentries > (int64) hctl->max_bucket &&
!IS_PARTITIONED(hctl) && !hashp->frozen &&
+ !hctl->isfixed &&
!has_seq_scans(hashp))
(void) expand_table(hashp);
}
Attachments:
[text/plain] 0001-prevent-fixed-dynahash-extension-20260815.patch (781B, ../../d59221f2-b3d2-41ad-8bf0-d581b42e4cba@garret.ru/3-0001-prevent-fixed-dynahash-extension-20260815.patch)
download | inline diff:
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index dc7ae64a5a9..727adfccf2d 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -927,11 +927,13 @@ hash_search_with_hash_value(HTAB *hashp,
if (action == HASH_ENTER || action == HASH_ENTER_NULL)
{
/*
- * Can't split if running in partitioned mode, nor if frozen, nor if
+ * Can't split if running in partitioned mode, nor if frozen,
+ * nor if fixed (in shared memory), nor if
* table is the subject of any active hash_seq_search scans.
*/
if (hctl->freeList[0].nentries > (int64) hctl->max_bucket &&
!IS_PARTITIONED(hctl) && !hashp->frozen &&
+ !hctl->isfixed &&
!has_seq_scans(hashp))
(void) expand_table(hashp);
}
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: SIGSEGV in dynahash
@ 2026-08-15 16:12 Tom Lane <tgl@sss.pgh.pa.us>
parent: Konstantin Knizhnik <knizhnik@garret.ru>
1 sibling, 2 replies; 6+ messages in thread
From: Tom Lane @ 2026-08-15 16:12 UTC (permalink / raw)
To: Konstantin Knizhnik <knizhnik@garret.ru>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Konstantin Knizhnik <knizhnik@garret.ru> writes:
> On PG19,|ShmemInitHash|always builds afixed-sizeshared hash with abump
> allocator(|ShmemHashAlloc|) whose|alloc_arg|is astack-localregion used
> only during|hash_create|. After init, that pointer is dead.
> ...
> Pre-PG19, shared hashes used|ShmemAllocNoError|from the global pool, so
> a failed grow tended to return|NULL|/ error instead of faulting on a
> dead bump allocator.
> It was introduced by commit 9fe9ecd516b — Allocate all parts of shmem
> hash table from a single contiguous area
Yeah. I'm not too pleased with 9fe9ecd516b for a different reason.
In pursuit of what seems to be a merely cosmetic goal (ie make shared
hashes be reported differently in pg_shmem_allocations), it's made a
fundamental and IMO possibly destabilizing change in the behavior of
shared-memory hash tables. To wit, it is no longer possible to expand
a shared hash table beyond its startup-time allocation. For some of
them that doesn't matter, but for others it definitely does; the lock
table in particular is sized only heuristically. For the last couple
of decades, there was slop in the max_locks_per_transaction limit
because the lock table could grow into the 100kB slop space we leave
in shared memory; but now there is no slop. I suspect we will get
complaints from people whose workloads used to work without trouble
and now don't. There might be extension code that depends on shared
hashtables not having a hard limit, too.
I wonder whether we shouldn't just revert this.
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: SIGSEGV in dynahash
@ 2026-08-15 23:32 Michael Paquier <michael@paquier.xyz>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 6+ messages in thread
From: Michael Paquier @ 2026-08-15 23:32 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Konstantin Knizhnik <knizhnik@garret.ru>; Heikki Linnakangas <hlinnaka@iki.fi>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On Sat, Aug 15, 2026 at 12:12:29PM -0400, Tom Lane wrote:
> Yeah. I'm not too pleased with 9fe9ecd516b for a different reason.
>
> In pursuit of what seems to be a merely cosmetic goal (ie make shared
> hashes be reported differently in pg_shmem_allocations), it's made a
> fundamental and IMO possibly destabilizing change in the behavior of
> shared-memory hash tables. To wit, it is no longer possible to expand
> a shared hash table beyond its startup-time allocation.
There is a difference between shmem_hash.c and dynahash.c. dynahash.c
is able to support a growing size, but it's not the case of
shmem_hash.c.
Note also some comments in dynahash.c:
* table grows much beyond the initial size. (Currently, shared memory hash
* tables are only created by ShmemRequestHash()/ShmemInitHash() though, which
* doesn't support growing at all.)
shmem_hash_create() also claims the non-growth argument as true as we
would need to grow the underlying shmem region linked with a hash
table defined by shmem_hash.c.
Or am I missing something?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../aoD3ACTRYXaF7t1-@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: SIGSEGV in dynahash
@ 2026-08-20 13:19 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 6+ messages in thread
From: Heikki Linnakangas @ 2026-08-20 13:19 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Konstantin Knizhnik <knizhnik@garret.ru>; +Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On 15/08/2026 19:12, Tom Lane wrote:
> Yeah. I'm not too pleased with 9fe9ecd516b for a different reason.
>
> In pursuit of what seems to be a merely cosmetic goal (ie make shared
> hashes be reported differently in pg_shmem_allocations), it's made a
> fundamental and IMO possibly destabilizing change in the behavior of
> shared-memory hash tables. To wit, it is no longer possible to expand
> a shared hash table beyond its startup-time allocation. For some of
> them that doesn't matter, but for others it definitely does; the lock
> table in particular is sized only heuristically. For the last couple
> of decades, there was slop in the max_locks_per_transaction limit
> because the lock table could grow into the 100kB slop space we leave
> in shared memory; but now there is no slop. I suspect we will get
> complaints from people whose workloads used to work without trouble
> and now don't. There might be extension code that depends on shared
> hashtables not having a hard limit, too.
>
> I wonder whether we shouldn't just revert this.
This was discussed at the time. My opinion, and I thought there was a
consensus on it, is that the unpredictable behavior of the "slop" was
not a good idea. The slop got assigned to whatever hash table happened
to use it first, and from there on it was reserved for that hash table
until server restart. That's pretty unpredictable, and hard to reason
about for tuning purposes.
It was all about the lock manager, none of the other hash tables used
the slop. The lock table consists of two hash tables, the "LOCK hash"
and the "PROCLOCK hash". The slop would get assigned to one of those
depending on the kind of queries you run after server startup. If you
ran a query that happened to need a lot of "LOCK hash" space, and then
ran a workload that needed more "PROCLOCK hash" space, the latter would
fail. But if you ran the queries in different order, then the slop was
assigned to the "PROLOCK hash" and it would work, but running the other
kind of query would fail instead. Weird.
I increased the default for max_locks_per_transaction (commit
79534f9065) to compensate for this, so that most applications that were
using the default settings and were relying on the slop will continue to
work.
- Heikki
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: SIGSEGV in dynahash
@ 2026-08-25 04:30 Rahila Syed <rahilasyed90@gmail.com>
parent: Konstantin Knizhnik <knizhnik@garret.ru>
1 sibling, 1 reply; 6+ messages in thread
From: Rahila Syed @ 2026-08-25 04:30 UTC (permalink / raw)
To: Konstantin Knizhnik <knizhnik@garret.ru>; +Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Hi,
> On PG19, ShmemInitHash always builds a fixed-size shared hash with a bump allocator (ShmemHashAlloc) whose alloc_arg is a stack-local region used only during hash_create. After init, that pointer is dead.
>
> In hash_search, for every HASH_ENTER / HASH_ENTER_NULL, dynahash does this before lookup:
>
> dynahash.cLines 927-937
> if (action == HASH_ENTER || action == HASH_ENTER_NULL)
> {
> if (hctl->freeList[0].nentries > (int64) hctl->max_bucket &&
> !IS_PARTITIONED(hctl) && !hashp->frozen &&
> !has_seq_scans(hashp))
> (void) expand_table(hashp);
> }
>
>
> It may cause SIGSEGV in case of using HASH_ENTER_NULL:
>
>
> hash_search(HASH_ENTER_NULL)
> → expand_table → seg_alloc → SIGSEGV in libc (MemSet/alloc)
>
The same thing cannot be triggered by
hash_search_with_hash_value()->get_hash_entry()->element_alloc()
because element_alloc()'s
if (hctl->isfixed) return false; guard trips before ever touching the allocator.
Similarly for dir_realloc() which has a check if
(hashp->hctl->max_dsize != NO_MAX_DSIZE) return false.
Since shared hash tables set a fixed max_dsize (not NO_MAX_DSIZE),
dir_realloc() returns false immediately instead of growing the
directory.
This makes me think we should add a check for hctl->isfixed in
seg_alloc() instead of before
expand_table() like it is done in the proposed patch.
It would also be valuable to add this SIGSEGV as a regression test. It
can't be triggered through any of the existing shared hash tables,
though — LOCK and PROCLOCK are the only fixed-size ones in core, and
both are partitioned, which makes expand_table() unreachable for them
(!IS_PARTITIONED(hctl) at dynahash.c:934/:1497). A dedicated
non-partitioned, fixed-size test hash table would be needed.
Thank you,
Rahila Syed
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: SIGSEGV in dynahash
@ 2026-08-25 11:14 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Rahila Syed <rahilasyed90@gmail.com>
0 siblings, 0 replies; 6+ messages in thread
From: Heikki Linnakangas @ 2026-08-25 11:14 UTC (permalink / raw)
To: Rahila Syed <rahilasyed90@gmail.com>; Konstantin Knizhnik <knizhnik@garret.ru>; +Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On 25/08/2026 07:30, Rahila Syed wrote:
>> On PG19, ShmemInitHash always builds a fixed-size shared hash with a bump allocator (ShmemHashAlloc) whose alloc_arg is a stack-local region used only during hash_create. After init, that pointer is dead.
>>
>> In hash_search, for every HASH_ENTER / HASH_ENTER_NULL, dynahash does this before lookup:
>>
>> dynahash.cLines 927-937
>> if (action == HASH_ENTER || action == HASH_ENTER_NULL)
>> {
>> if (hctl->freeList[0].nentries > (int64) hctl->max_bucket &&
>> !IS_PARTITIONED(hctl) && !hashp->frozen &&
>> !has_seq_scans(hashp))
>> (void) expand_table(hashp);
>> }
>>
>>
>> It may cause SIGSEGV in case of using HASH_ENTER_NULL:
>>
>>
>> hash_search(HASH_ENTER_NULL)
>> → expand_table → seg_alloc → SIGSEGV in libc (MemSet/alloc)
>
> The same thing cannot be triggered by
> hash_search_with_hash_value()->get_hash_entry()->element_alloc()
> because element_alloc()'s
> if (hctl->isfixed) return false; guard trips before ever touching the allocator.
>
> Similarly for dir_realloc() which has a check if
> (hashp->hctl->max_dsize != NO_MAX_DSIZE) return false.
> Since shared hash tables set a fixed max_dsize (not NO_MAX_DSIZE),
> dir_realloc() returns false immediately instead of growing the
> directory.
>
> This makes me think we should add a check for hctl->isfixed in
> seg_alloc() instead of before
> expand_table() like it is done in the proposed patch.
Hmm, yeah, I see what you mean, although I don't feel too bad about just
assuming that they're not called from elsewhere. I added an
Assert(!hashp->hctl->isfixed) in both seg_alloc() and dir_realloc(), to
document that you shouldn't get there for fixed-size hash tables.
Committed the fix with those extra Asserts. Thanks!
> It would also be valuable to add this SIGSEGV as a regression test. It
> can't be triggered through any of the existing shared hash tables,
> though — LOCK and PROCLOCK are the only fixed-size ones in core, and
> both are partitioned, which makes expand_table() unreachable for them
> (!IS_PARTITIONED(hctl) at dynahash.c:934/:1497). A dedicated
> non-partitioned, fixed-size test hash table would be needed.
All shmem hash tables are fixed-size. But you need one that has just the
right number of elements, is not partitioned, and you then need to fill
it up to trigger the bug.
I came up with the attached, but now that we've fixed the bug it doesn't
feel very interesting to test for exactly that, so I didn't feel it's
worth committing. Maybe as part of a larger unit test of various hash
table operations, but even then the requirement that the hash table is
just the right size to exercise this case feels fragile.
- Heikki
Attachments:
[text/x-patch] test-fixed-hash-expand-bug.patch (4.2K, ../../e3ae5aa3-a051-4e07-9fac-c0a24c361e22@iki.fi/2-test-fixed-hash-expand-bug.patch)
download | inline diff:
diff --git a/src/test/modules/test_shmem/t/001_late_shmem_alloc.pl b/src/test/modules/test_shmem/t/001_late_shmem_alloc.pl
index 5cf07d071ec..62be5cd0edd 100644
--- a/src/test/modules/test_shmem/t/001_late_shmem_alloc.pl
+++ b/src/test/modules/test_shmem/t/001_late_shmem_alloc.pl
@@ -25,6 +25,11 @@ my $attach_count2 =
$node->safe_psql("postgres", "SELECT get_test_shmem_attach_count();");
cmp_ok($attach_count2, '>', $attach_count1,
"attach callback is called in each backend");
+
+# Test a shared memory hash table too.
+my $result = $node->safe_psql("postgres", "SELECT test_shmem_hash_overflow();");
+ok($result eq "", "shmem hash works");
+
$node->stop;
###
@@ -56,5 +61,9 @@ else
);
}
+# Test a shared memory hash table too.
+$result = $node->safe_psql("postgres", "SELECT test_shmem_hash_overflow();");
+ok($result eq "", "shmem hash works");
+
$node->stop;
done_testing();
diff --git a/src/test/modules/test_shmem/test_shmem--1.0.sql b/src/test/modules/test_shmem/test_shmem--1.0.sql
index 2d01fd9256c..7b91f8c6f42 100644
--- a/src/test/modules/test_shmem/test_shmem--1.0.sql
+++ b/src/test/modules/test_shmem/test_shmem--1.0.sql
@@ -7,3 +7,7 @@
CREATE FUNCTION get_test_shmem_attach_count()
RETURNS pg_catalog.int4 STRICT
AS 'MODULE_PATHNAME' LANGUAGE C;
+
+CREATE FUNCTION test_shmem_hash_overflow()
+RETURNS void STRICT
+AS 'MODULE_PATHNAME' LANGUAGE C;
diff --git a/src/test/modules/test_shmem/test_shmem.c b/src/test/modules/test_shmem/test_shmem.c
index 9bd4012b435..da9fddc5ee5 100644
--- a/src/test/modules/test_shmem/test_shmem.c
+++ b/src/test/modules/test_shmem/test_shmem.c
@@ -20,10 +20,12 @@
#include "fmgr.h"
#include "miscadmin.h"
#include "storage/shmem.h"
+#include "utils/hsearch.h"
PG_MODULE_MAGIC;
+/* For testing ShmemRequestStruct */
typedef struct TestShmemData
{
int value;
@@ -35,6 +37,21 @@ static TestShmemData *TestShmem;
static bool attached_or_initialized = false;
+/* For testing ShmemRequestHash */
+
+/*
+ * XXX: This is chosen to be equal to HASH_SEGSIZE (256) so that we exercise
+ * the directory expansion bug.
+ */
+#define TEST_HASH_NELEMS 256
+
+typedef struct TestHashEntry
+{
+ int32 key; /* hash key, must be first */
+} TestHashEntry;
+
+static HTAB *TestShmemHash;
+
static void test_shmem_request(void *arg);
static void test_shmem_init(void *arg);
static void test_shmem_attach(void *arg);
@@ -54,6 +71,13 @@ test_shmem_request(void *arg)
ShmemRequestStruct(.name = "test_shmem area",
.size = sizeof(TestShmemData),
.ptr = (void **) &TestShmem);
+
+ ShmemRequestHash(.name = "test_shmem overflow hash",
+ .nelems = TEST_HASH_NELEMS,
+ .hash_info.keysize = sizeof(int32),
+ .hash_info.entrysize = sizeof(TestHashEntry),
+ .hash_flags = HASH_ELEM | HASH_BLOBS,
+ .ptr = &TestShmemHash);
}
static void
@@ -99,3 +123,48 @@ get_test_shmem_attach_count(PG_FUNCTION_ARGS)
elog(ERROR, "shmem area not yet initialized");
PG_RETURN_INT32(TestShmem->attach_count);
}
+
+/*
+ * Test a shared memory hash table.
+ *
+ * Check that the correct number of elements can be inserted with
+ * HASH_ENTER_NULL. The hash table is expected to be empty before the call.
+ */
+PG_FUNCTION_INFO_V1(test_shmem_hash_overflow);
+Datum
+test_shmem_hash_overflow(PG_FUNCTION_ARGS)
+{
+ int count = 0;
+ int32 key;
+ bool found;
+ TestHashEntry *entry;
+
+ /* Assume the hash to be empty before the call */
+ if (hash_get_num_entries(TestShmemHash) != 0)
+ elog(ERROR, "hash table is not empty");
+
+ /* Fill up the hash table */
+ for (key = 0; key < TEST_HASH_NELEMS; key++)
+ {
+ entry = hash_search(TestShmemHash, &key, HASH_ENTER_NULL, &found);
+ if (found)
+ elog(ERROR, "hash entry with key %d already exists", key);
+ if (entry == NULL)
+ elog(ERROR, "hash table of size %d is full after only %d insertions",
+ TEST_HASH_NELEMS, count);
+ count++;
+ }
+
+ /*
+ * Try to insert one more entry. It should now fail because the hash
+ * table is full.
+ */
+ entry = hash_search(TestShmemHash, &key, HASH_ENTER_NULL, &found);
+ if (found)
+ elog(ERROR, "hash entry with key %d already exists", key);
+ if (entry != NULL)
+ elog(ERROR, "hash table of size %d had space for an extra element",
+ TEST_HASH_NELEMS);
+
+ PG_RETURN_VOID();
+}
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2026-08-25 11:14 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-08-15 13:06 SIGSEGV in dynahash Konstantin Knizhnik <knizhnik@garret.ru>
2026-08-15 16:12 ` Tom Lane <tgl@sss.pgh.pa.us>
2026-08-15 23:32 ` Michael Paquier <michael@paquier.xyz>
2026-08-20 13:19 ` Heikki Linnakangas <hlinnaka@iki.fi>
2026-08-25 04:30 ` Rahila Syed <rahilasyed90@gmail.com>
2026-08-25 11:14 ` Heikki Linnakangas <hlinnaka@iki.fi>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox