> 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
correspondingassert 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)
