> On Mar 18, 2026, at 07:34, Robert Haas <[email protected]> wrote:
>
> 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.
> <v1-0001-dshash-Make-it-possible-to-suppress-out-of-memory.patch>
I think adding a DSHASH_INSERT_NO_OOM flag is the right choice. As you
mentioned, we already have other _NO_OOM flags, so this feels consistent with
existing patterns.
I don’t see any correctness issue with the patch. I just have a couple of minor
nits.
1
```
@@ -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;
+ }
```
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.
2
```
/* 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);
+ }
```
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.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
