Re: [PATCH] binary heap implementation

On 2012-11-21 15:09:12 -0500, Robert Haas wrote:
> On Wed, Nov 21, 2012 at 1:30 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
> > The following comments still talk about "key and value", thus need
> > an update:
>
> Oops.
>
> > This comment needs updated (s/comparator/compare/, and also add
> > has_heap_property and arg):
>
> Fixed.
>
> > I would suggest to add prefixes to struct members, so bh_size, bh_space
> > and so on.  This makes it easier to grep for stuff later.
>
> OK.
>
> > Do we really need the struct definition be public?  Couldn't it just
> > live in binaryheap.c?
>
> Well, that would preclude defining any macros, like
> binaryheap_empty().  Since efficiency is among the reasons for
> inventing this in the first place, that doesn't seem prudent to me.
>
> Another new version attached.

One very minor nitpick I unfortunately just found now, not sure when
that changed:
binaryheap_replace_first() hardcodes the indices for the left/right node
of the root node. I would rather have it use (left|right)_offset(0).

> P.S. to Abhijit: Please, please tell me the secret to getting five
> people to review your patches.  I'm lucky when I can get one!

Heh. This was rather surprising, yes. Its a rather easy patch to review
in a way, you don't need much pg internals knowledge for it...

Greetings,

Andres Freund

--Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services