Re: dshash_find_or_insert vs. OOM

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