On 09/04/2024 21:04, Jeff Davis wrote:
> On Fri, 2024-04-05 at 16:58 +0900, Masahiko Sawada wrote:
>>> I have some further comments and I believe changes are required for
>>> v17.
>
> I also noticed that the simplehash is declared in binaryheap.h with
> "SH_SCOPE extern", which seems wrong. Attached is a rough patch to
> bring the declarations into binaryheap.c.
>
> Note that the attached patch uses "SH_SCOPE static", which makes sense
> to me in this case, but causes a bunch of warnings in gcc. I will post
> separately about silencing that warning, but for now you can either
> use:
>
> SH_SCOPE static inline
>
> which is probably fine, but will encourage the compiler to inline more
> code, when not all callers even use the hash table. Alternatively, you
> can do:
>
> SH_SCOPE static pg_attribute_unused()
>
> which looks a bit wrong to me, but seems to silence the warnings, and
> lets the compiler decide whether or not to inline.
>
> Also probably needs comment updates, etc.
Sorry I'm late to the party, I didn't pay attention to this thread
earlier. But I wonder why this doesn't use the existing pairing heap
implementation? I would assume that to be at least as good as the binary
heap + hash table. And it's cheap to to insert to (O(1)), so we could
probably remove the MAX_HEAP_TXN_COUNT_THRESHOLD, and always keep the
heap up-to-date.
--
Heikki Linnakangas
Neon (https://neon.tech)