Thread: 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@sigaev.ru
WWW: http://www.sigaev.ru/
Attachment
2009/11/23 Teodor Sigaev <teodor@sigaev.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@sigaev.ru
WWW: http://www.sigaev.ru/
Attachment
2009/12/30 Teodor Sigaev <teodor@sigaev.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
Attachment
> 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@sigaev.ru WWW: http://www.sigaev.ru/
2010/1/11 Teodor Sigaev <teodor@sigaev.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@sigaev.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@sigaev.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@hasegeli.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@postgresql.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@2ndquadrant.com> wrote:
Robert Haas wrote:
> 2009/12/30 Teodor Sigaev <teodor@sigaev.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@gmail.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
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@hasegeli.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
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.korotkov@postgrespro.ru>: > Hi, Stefan! > > On Sun, Oct 11, 2015 at 10:00 PM, Stefan Keller <sfkeller@gmail.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 >