point_ops for GiST

Hi!

patch implements operator class for GiST over points. Supported operations:
point << point
point >> point
point <^ point
point >^ point
point ~= point
point <@ box
box @> point
point <@ polygon
polygon @> point
point <@ circle
circle @> point
--
Teodor Sigaev E-mail: teodor(at)sigaev(dot)ru
WWW: http://www.sigaev.ru/

2009/11/23 Teodor Sigaev <teodor(at)sigaev(dot)ru>:

> point   <@ box
> point   <@ polygon
> point   <@ circle

I've always wondered why we didn't have these

--
greg

Sync with current CVS

--
Teodor Sigaev E-mail: teodor(at)sigaev(dot)ru
WWW: http://www.sigaev.ru/

2009/12/30 Teodor Sigaev <teodor(at)sigaev(dot)ru>:
> Sync with current CVS

I have reviewed this patch and it looks good to me. The only
substantive question I have is why gist_point_consistent() uses a
different coding pattern for the box case than it does for the polygon
and circle cases? It's not obvious to me on the face of it why these
aren't consistent.

Beyond that, I have a variety of minor whitespace and commenting
suggestions, so I am attaching an updated version of the patch as well
as an incremental diff between your version and mine, for your
consideration. The changes are: (1) comment reuse of gist_box
functions for point_ops, (2) format point ops function analogously to
existing sections in same file, (3) uncuddle opening braces, (4)
adjust indentation and spacing in a few places, (5) rename
StrategyNumberOffsetRange to GeoStrategyNumberOffset, and (6) use a
plain block instead of do {} while (0) - the latter construct is
really only needed in certain types of macros.

...Robert

> I have reviewed this patch and it looks good to me. The only
> substantive question I have is why gist_point_consistent() uses a
> different coding pattern for the box case than it does for the polygon
> and circle cases? It's not obvious to me on the face of it why these
> aren't consistent.

gist_circle_consistent/gist_poly_consistent set recheck flag to true because
corresponding index contains only bounding box of indexed values
(circle/polygon). gist_point_consistent could do an exact check. Will add a coments.

> Beyond that, I have a variety of minor whitespace and commenting
> suggestions, so I am attaching an updated version of the patch as well

Agree with your changes. Suppose, there is no objection to commit it?

--
Teodor Sigaev E-mail: teodor(at)sigaev(dot)ru
WWW: http://www.sigaev.ru/

2010/1/11 Teodor Sigaev <teodor(at)sigaev(dot)ru>:
>> I have reviewed this patch and it looks good to me.  The only
>> substantive question I have is why gist_point_consistent() uses a
>> different coding pattern for the box case than it does for the polygon
>> and circle cases?  It's not obvious to me on the face of it why these
>> aren't consistent.
>
> gist_circle_consistent/gist_poly_consistent set recheck flag to true because
> corresponding index contains only bounding box of indexed values
> (circle/polygon). gist_point_consistent could do an exact check. Will add a
> coments.

Make sense. A comment sounds good.

>> Beyond that, I have a variety of minor whitespace and commenting
>> suggestions, so I am attaching an updated version of the patch as well
>
> Agree with your changes. Suppose, there is no objection to commit it?

No, I think it looks good... if no one else chimes in with objections
in the next day or two I would go ahead.

...Robert

Robert Haas wrote:
> 2009/12/30 Teodor Sigaev <teodor(at)sigaev(dot)ru>:
> > Sync with current CVS
>
> I have reviewed this patch and it looks good to me. The only
> substantive question I have is why gist_point_consistent() uses a
> different coding pattern for the box case than it does for the polygon
> and circle cases? It's not obvious to me on the face of it why these
> aren't consistent.

Emre Hasegeli just pointed out to me that this patch introduced
box_contain_pt() and in doing so used straight C comparison (<= etc)
instead of FPlt() and friends. I would think that that's a bug and
needs to be changed -- but certainly not backpatched, because gist
indexes would/might become corrupt.

This is in the context of his inclusion opclass for BRIN
http://www.postgresql.org/message-id/CAE2gYzwBZQ=z02ZiOefNhrXOf+1VegQC_cWPVJ8LwNqGX1--Ww@mail.gmail.com

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera wrote:
> Robert Haas wrote:
> > 2009/12/30 Teodor Sigaev <teodor(at)sigaev(dot)ru>:
> > > Sync with current CVS
> >
> > I have reviewed this patch and it looks good to me. The only
> > substantive question I have is why gist_point_consistent() uses a
> > different coding pattern for the box case than it does for the polygon
> > and circle cases? It's not obvious to me on the face of it why these
> > aren't consistent.
>
> Emre Hasegeli just pointed out to me that this patch introduced
> box_contain_pt() and in doing so used straight C comparison (<= etc)
> instead of FPlt() and friends.

(This is commit 4cbe473938779ec414d90c2063c4398e68a70838)

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> Emre Hasegeli just pointed out to me that this patch introduced
> box_contain_pt() and in doing so used straight C comparison (<= etc)
> instead of FPlt() and friends. I would think that that's a bug and
> needs to be changed -- but certainly not backpatched, because gist
> indexes would/might become corrupt.

The problem with this is BRIN inclusion opclass uses some operators to
implement others. It was using box @> point operator to implement
point ~= point operator by indexing points in boxes. The former
doesn't use the macros, but later does. The opclass could return
wrong result when the point right near the index boundaries.

Currently, there are not BRIN opclasses for geometric types except box
because of this reason. I would like to work on supporting them for
the next release. I think the best way is to change the operators
which are not using the macros to be consistent with the others. Here
is the list:

* polygon << polygon
* polygon &< polygon
* polygon &> polygon
* polygon >> polygon
* polygon <<| polygon
* polygon &<| polygon
* polygon |&> polygon
* polygon |>> polygon
* box @> point
* point <@ box
* lseg <@ box
* circle @> point
* point <@ circle

I can send a patch, if it is acceptable.

Hi Emre

Pls. don't misunderstand my questions: They are directed to get an
even more useful spatial data handling of PostgreSQL. I'm working with
PostGIS since years and are interested in any work regarding spatial
types...

Can anyone report use cases or applications of these built-in geometric types?

Would'nt it be even more useful to concentrate to PostGIS
geometry/geography types and extend BRIN to these types?

:Stefan

2015-06-13 23:04 GMT+02:00 Emre Hasegeli <emre(at)hasegeli(dot)com>:
>> Emre Hasegeli just pointed out to me that this patch introduced
>> box_contain_pt() and in doing so used straight C comparison (<= etc)
>> instead of FPlt() and friends. I would think that that's a bug and
>> needs to be changed -- but certainly not backpatched, because gist
>> indexes would/might become corrupt.
>
> The problem with this is BRIN inclusion opclass uses some operators to
> implement others. It was using box @> point operator to implement
> point ~= point operator by indexing points in boxes. The former
> doesn't use the macros, but later does. The opclass could return
> wrong result when the point right near the index boundaries.
>
> Currently, there are not BRIN opclasses for geometric types except box
> because of this reason. I would like to work on supporting them for
> the next release. I think the best way is to change the operators
> which are not using the macros to be consistent with the others. Here
> is the list:
>
> * polygon << polygon
> * polygon &< polygon
> * polygon &> polygon
> * polygon >> polygon
> * polygon <<| polygon
> * polygon &<| polygon
> * polygon |&> polygon
> * polygon |>> polygon
> * box @> point
> * point <@ box
> * lseg <@ box
> * circle @> point
> * point <@ circle
>
> I can send a patch, if it is acceptable.
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

Hi, Alvaro!

On Tue, May 12, 2015 at 9:13 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> Robert Haas wrote:
> > 2009/12/30 Teodor Sigaev <teodor(at)sigaev(dot)ru>:
> > > Sync with current CVS
> >
> > I have reviewed this patch and it looks good to me. The only
> > substantive question I have is why gist_point_consistent() uses a
> > different coding pattern for the box case than it does for the polygon
> > and circle cases? It's not obvious to me on the face of it why these
> > aren't consistent.
>
> Emre Hasegeli just pointed out to me that this patch introduced
> box_contain_pt() and in doing so used straight C comparison (<= etc)
> instead of FPlt() and friends. I would think that that's a bug and
> needs to be changed -- but certainly not backpatched, because gist
> indexes would/might become corrupt.
>

This was already fixed for GiST.
See following discussion
http://www.postgresql.org/message-id/CAPpHfdvGticGniaj88VCHzHboXJobUhjLm6c09q_Op_u9EoBFg@mail.gmail.com
and
commit
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=3c29b196b0ce46662cb9bb7a1f91079fbacbcabb
"Consistent" method of GiST influences only search and can't lead to
corrupt indexes. However, "same" method can lead to corrupt indexes.
However, this is not the reason to not backpatch the changes and preserve
buggy behaviour; this is the reason to recommend reindexing to users. And
it was already backpatched.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi, Stefan!

On Sun, Oct 11, 2015 at 10:00 PM, Stefan Keller <sfkeller(at)gmail(dot)com> wrote:

> Pls. don't misunderstand my questions: They are directed to get an
> even more useful spatial data handling of PostgreSQL. I'm working with
> PostGIS since years and are interested in any work regarding spatial
> types...
>
> Can anyone report use cases or applications of these built-in geometric
> types?
>
> Would'nt it be even more useful to concentrate to PostGIS
> geometry/geography types and extend BRIN to these types?
>

Note, that PostGIS is a different project which is maintained by separate
team. PostGIS have its own priorities, development plan etc.
PostgreSQL have to be self-consistent. In particular, it should have
reference implementation of operator classes which extensions can use as
the pattern. This is why it's important to maintain built-in geometric
types.

In short: once we implement it for built-in geometric types, you can ask
PostGIS team to do the same for their geometry/geography.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

> This was already fixed for GiST.
> See following discussion
> http://www.postgresql.org/message-id/CAPpHfdvGticGniaj88VCHzHboXJobUhjLm6c09q_Op_u9EoBFg@mail.gmail.com
> and commit
> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=3c29b196b0ce46662cb9bb7a1f91079fbacbcabb
> "Consistent" method of GiST influences only search and can't lead to corrupt
> indexes. However, "same" method can lead to corrupt indexes.
> However, this is not the reason to not backpatch the changes and preserve
> buggy behaviour; this is the reason to recommend reindexing to users. And it
> was already backpatched.

Fixing it on the opclass is not an option for BRIN. We designed BRIN
opclasses extensible using extra SQL level support functions and
operators. It is possible to support point datatype using box vs
point operators. Doing so would lead to wrong results, because point
operators use FP macros, but box_contain_pt() doesn't.

GiST opclass could be more clean and extensible, if we wouldn't have
those macros.

On Mon, Oct 12, 2015 at 12:47 PM, Emre Hasegeli <emre(at)hasegeli(dot)com> wrote:

> > This was already fixed for GiST.
> > See following discussion
> >
> http://www.postgresql.org/message-id/CAPpHfdvGticGniaj88VCHzHboXJobUhjLm6c09q_Op_u9EoBFg@mail.gmail.com
> > and commit
> >
> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=3c29b196b0ce46662cb9bb7a1f91079fbacbcabb
> > "Consistent" method of GiST influences only search and can't lead to
> corrupt
> > indexes. However, "same" method can lead to corrupt indexes.
> > However, this is not the reason to not backpatch the changes and preserve
> > buggy behaviour; this is the reason to recommend reindexing to users.
> And it
> > was already backpatched.
>
> Fixing it on the opclass is not an option for BRIN. We designed BRIN
> opclasses extensible using extra SQL level support functions and
> operators. It is possible to support point datatype using box vs
> point operators. Doing so would lead to wrong results, because point
> operators use FP macros, but box_contain_pt() doesn't.
>

You still can workaround this problem in opclass. For instance, you can
assign different strategy number for this case. And call another support
function instead of overlap operator in brin_inclusion_consistent. For
sure, this would be a kluge.

> GiST opclass could be more clean and extensible, if we wouldn't have
> those macros.
>

In my opinion it would be cool remove FP macros. I see absolutely no sense
in them. But since it break compatibility it would be quite hard though.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

>> Pls. don't misunderstand my questions: They are directed to get an
>> even more useful spatial data handling of PostgreSQL. I'm working with
>> PostGIS since years and are interested in any work regarding spatial
>> types...
>>
>> Can anyone report use cases or applications of these built-in geometric
>> types?
>>
>> Would'nt it be even more useful to concentrate to PostGIS
>> geometry/geography types and extend BRIN to these types?
>
>
> Note, that PostGIS is a different project which is maintained by separate
> team. PostGIS have its own priorities, development plan etc.
> PostgreSQL have to be self-consistent. In particular, it should have
> reference implementation of operator classes which extensions can use as the
> pattern. This is why it's important to maintain built-in geometric types.
>
> In short: once we implement it for built-in geometric types, you can ask
> PostGIS team to do the same for their geometry/geography.

The problem is that geometric types don't even serve well to this
purpose in their current state. We have to make the operators
consistent to better demonstrate index support of cross type
operators.

Hi Alexander

Thanks for your succinct reply.
Actually I considered contributing myself for the first time to
PostgreSQL and/or PostGIS.
So, concluding from your explanations there's no big use case behind
build-in geometric types except serving as reference implementation?
I'm still torn over this splitting resources to implement types like
geometry twice.

:Stefan

2015-10-12 11:24 GMT+02:00 Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>:
> Hi, Stefan!
>
> On Sun, Oct 11, 2015 at 10:00 PM, Stefan Keller <sfkeller(at)gmail(dot)com> wrote:
>>
>> Pls. don't misunderstand my questions: They are directed to get an
>> even more useful spatial data handling of PostgreSQL. I'm working with
>> PostGIS since years and are interested in any work regarding spatial
>> types...
>>
>> Can anyone report use cases or applications of these built-in geometric
>> types?
>>
>> Would'nt it be even more useful to concentrate to PostGIS
>> geometry/geography types and extend BRIN to these types?
>
>
> Note, that PostGIS is a different project which is maintained by separate
> team. PostGIS have its own priorities, development plan etc.
> PostgreSQL have to be self-consistent. In particular, it should have
> reference implementation of operator classes which extensions can use as the
> pattern. This is why it's important to maintain built-in geometric types.
>
> In short: once we implement it for built-in geometric types, you can ask
> PostGIS team to do the same for their geometry/geography.
>
> ------
> Alexander Korotkov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>