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)
