Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#520 closed defect (fixed)

No check for different srid on operators when index is used

Reported by: nicklas Owned by: pramsey
Priority: medium Milestone: PostGIS 2.0.0
Component: postgis Version: trunk
Keywords: history Cc:

Description

If tbl1 and tbl2 has different srid this:

select * from tbl1, tbl2 where a.the_geom && b.the_geom;

will give error if no indexes is used, but with indexes it just runs.

Maybe it is not possible to do the check when using indexes or it is just too costly. If so, I suggest the check is removed from the corresponding function too. Different behaviour with and without index I think is a little annoying.

And in all other cases than these operators the different srids will be catched by the recheck with a "real" function, so as I understand it, it is just a matter for the operators.

What I consider a defect here is not the lack of check but the different behaviour with and without index.

/Nicklas

Change History (13)

comment:1 Changed 9 years ago by nicklas

hmm, little strange.

I think it looks like the code is there in the operator functions in lwgeom_gist.c to catch the srid mismatch but it doesn't work, at least not for me.

Am I missing something here?

/Nicklas

comment:2 Changed 9 years ago by mcayland

Right. This is likely because your tables tbl1 and tbl2 have no overlapping regions. When returning an index scan from an operator, PostgreSQL simply reads the bounding boxes from the index and returns them for use within the query - so in other words they don't go through the main operator function. We don't store the SRID in the bounding box since it is redundant information, i.e. for most use cases the SRID for a whole table should be the same and so it ends up just taking space.

IMO having the actual operator functions in lwgeom_gist.c is misleading because they are not just used from within the index code which is probably where your confusion comes from.

Originally we had the RECHECK operator which would trap this case, but since it has been demonstrated to contribute a significant overhead to several types of query it has now been removed.

HTH,

Mark.

comment:3 Changed 9 years ago by nicklas

Mark, you are right, that s what confused me. I just read through it and presumed it was some sort of initialization for the index thing.

So, then I was right from the beginning that there is no check for index scans.

Then, how about the suggestion to remove it also from the actual operator function.

What I mean is If it is acceptable for index scans why not when the index is not used. The different srids will be found anyway by st_dwithin or whatever is using the operator. The case when this might make life easier is when comparing geometries with bounding boxes and then you must know what you are doing anyway because of the index scan behaviour.

/Nicklas

comment:4 Changed 9 years ago by robe

I agree with Nicklas on this one. We should remove it from the function now that we have taken out the RECHECK. Having different behavior for index vs non-index when we get no benefit seems pointless.

comment:5 Changed 9 years ago by mcayland

I must admit to be concerned at the time, but like you say now that the indexable operators are built into higher level functions it doesn't seem so much of a problem. So I'm not overly opposed to removing it to help keep things consistent.

comment:6 Changed 9 years ago by nicklas

Milestone: PostGIS 1.5.2PostGIS 2.0.0
Version: 1.5.Xtrunk

I move this to trunk since it actually means a little change in behavior. I will remove the srid check from the operators there if no objections.

/Nicklas

comment:7 Changed 8 years ago by pramsey

Am I mistaken, or has the inconsistency disappeared in 2.0?

select 'SRID=4326;POINT(0 0)'::geometry && 'SRID=4325;POINT(0 0)'::geometry;

comment:8 Changed 8 years ago by robe

You are mistaken. What Nicklas is saying is that if you have an index -- its ignore. Here is a standalone example that I ran on 2.0 and didn't throw an error:

CREATE TABLE tb1(gid serial primary key, geom geometry);
CREATE INDEX idx_tb1_geom_gist ON tb1 USING gist(geom);

INSERT INTO tb1(geom)
SELECT ST_SetSRID(ST_Point(i,j),srid)
FROM generate_series(-10,10,2) As i
    CROSS JOIN generate_series(-10,10,2) As j
    CROSS JOIN generate_series(4325,4326) As srid;


SELECT a.gid As a_gid, b.gid As b_gid, ST_SRID(a.geom) As a_srid, ST_SRID(b.geom) As b_srid
 FROM tb1 As a, tb1 As b 
WHERE a.geom && b.geom

Result is: a_gid b_gid a_srid b_srid 1 1 4325 4325 1 122 4325 4326

If I do this though:

SELECT a.gid As a_gid, b.gid As b_gid, ST_SRID(a.geom) As a_srid, ST_SRID(b.geom) As b_srid
 FROM tb1 As a, tb1 As b 
WHERE a.geom && b.geom 
   AND ST_SRID(a.geom) = 4325 AND ST_SRID(b.geom) = 4326;

I do get an error of:

ERROR:  Operation on two geometries with different SRIDs

comment:9 Changed 8 years ago by robe

FWIW, I really don't think this is a big issue and would just assume forget about it if its hard to fix. Usually when people use &&, they know what they are doing and I suspect trying to fix this will just slow things down.

NOTE: This more common query does give error:

SELECT a.gid As a_gid, b.gid As a_gid, ST_SRID(a.geom) As a_srid, 
  ST_SRID(b.geom) As b_srid
 FROM tb1 As a, tb1 As b 
WHERE ST_Intersects(a.geom,b.geom);

ERROR:  Operation on mixed SRID geometries

comment:10 Changed 8 years ago by robe

oops sorry forgot to read Nicklas comments -- yah the issue is not that it doesn't check but that && behaves differently when you are using an index vs. not. It's still a minor issue though.

comment:11 Changed 8 years ago by nicklas

Keywords: history added
Resolution: fixed
Status: newclosed

Hallo

I think Paul is right. The srid check is not there in the gserialized versions of the box comparing functions in file gserialized_gist_2d.c

If I test the query in Pauls example above it passes in trunk version but gets caught in 1.5.

I think this is the right behavior.

Regina, when I tried your example it also passed in trunk both with and without index.

So I close this one.

I also put a history tag for slightly new behavior.

Thanks Nicklas

comment:12 Changed 8 years ago by strk

There used to be a RECHECK in our indices, exactly for this. If anyone has a reference to a mailing list thread in which disabling the RECHECK was discussed please put it here.

Note: See TracTickets for help on using tickets.