Horton

Re: SP-GiST for ranges based on 2d-mapping and quad-tree

Alexander Korotkov <aekorotkov@gmail.com>

View thread
From: Alexander Korotkov <aekorotkov@gmail.com>
To: Jeff Davis <pgsql@j-davis.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, pgsql-hackers <pgsql-hackers@postgresql.org>, Heikki Linnakangas <hlinnakangas@vmware.com>
Date: 2012-12-13T21:31:02Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Teach SP-GiST to do index-only scans.

Attachments

Hi!

On Sun, Nov 4, 2012 at 11:41 PM, Jeff Davis <pgsql@j-davis.com> wrote:

> On Fri, 2012-11-02 at 12:47 +0400, Alexander Korotkov wrote:
>
> > Right version of patch is attached.
> >
> * In bounds_adjacent, there's no reason to flip the labels back.
>

Fixed.


> * Comment should indicate more explicitly that bounds_adjacent is
> sensitive to the argument order.
>

Fixed.


> * In bounds_adjacent, it appears that "bound1" corresponds to "B" in the
> comment above, and "bound2" corresponds to "A" in the comment above. I
> would have guessed from reading the comment that bound1 corresponded to
> A. We should just use consistent names between the comment and the code
> (e.g. boundA and boundB).
>

Fixed.


> * I could be getting confused, but I think that line 645 of
> rangetypes_spgist.c should be inverted (!bounds_adjacent(...)).
>

Good catch! Actually, code was in direct contradiction with comment. Fixed.


> * I think needPrevious should have an explanatory comment somewhere. It
> looks like you are using it to store some state as you descend the tree,
> but it doesn't look like it's used to reconstruct the value (because we
> already have the value anyway). Since it's being used for a purpose
> other than what's intended, that should be explained.
>

Yes, it's a some kind of hack now. Comment is added.

------
With best regards,
Alexander Korotkov.