Opened 10 years ago

Closed 10 years ago

#4869 closed defect (wontfix)

PostGIS driver fails to detect SRS from -sql query with tables= and no permission to query geometry_columns

Reported by: strk Owned by: warmerdam
Priority: normal Milestone:
Component: default Version: 1.9.2
Severity: normal Keywords:


ogr2ogr -f 'ESRI Shapefile' tmp 'PG:... tables=fake' -sql \
  'select * from gshhs_cent limit 1'
Warning 1: More than one geometry column was found in the result of the SQL request. Only last one will be used
OGRPGResultLayer::GetSpatialRef: nSRSId=-2 hasPostGISGeometry:1 GeomTableName:gshhs_cent
 Got SRID from GetLayerByName: 0

The user I'm using to connect to the database has no permission to read from geometry_column. Fixing that also fixes retrival of the SRID.

But since I do see code to fetch the SRID from the first row returned by the query, I wonder if that code path should be taken when the geometry_columns view query fails, rather than accepting an UNKNOWN srid as a result.

I'm talking about line ~350 of ogr/ogrsf_frmts/pg/ogrpgresultlayer.cpp

Attachments (1)

0001-Print-an-error-when-spatial_ref_sys-querying-fails.patch (888 bytes ) - added by strk 10 years ago.
proposed patch

Download all attachments as: .zip

Change History (13)

comment:1 by Even Rouault, 10 years ago

Hum, still your weird setup. I'm middly convinced that the PG driver should be complicated to support that use case. Is it supposed to be or become a common use case ?

And are you sure that the spatial_ref_sys table has SELECT priviledges for the connected user ? If not, that's a dead end anyway since we must find the WKT definition from the SRID.

comment:2 by strk, 10 years ago

Not, it won't be a common use case.

I'm just saying that the code is already there, so handling the impossibility to run the query shouldn't make things so much more complex. We're always talking about an arbitrary query so I'm even surprised the code is able to come up with a table name to lookup in geometry_column. All I'm saying is that failure to do the geometry_column lookup should let the code flow to next attempt (fetching srid from dataset).

I guess one possibly common use case would be fetching data from views using spatial function in the sql part. Those views would appear as having SRID=0 in geometry_column, while fetching the srid from the query result would give a more sensible answer.

comment:3 by strk, 10 years ago

Ok back to this issue. I tried my "custom query involving no tables" query and indeed it worked. So the only problematic query is only when geometry_columns is unreadable _and_ tables=xx is passed.

With no tables=xx you get an error : ERROR 1: ERROR: permission denied for relation geometry_columns

With tables=xx you only get a .prj file WHEN the user can read geometry_columns.

It still seems inconsistent to me. It should either raise an error (if geometry_columns scanning is required) or find the SRID, don't you think so ?

comment:4 by Even Rouault, 10 years ago

The issue is that OGRPGResultLayer::GetSpatialRef() has no idea that the value returned by poBaseLayer->GetSRID(), which happens to have been set by nSRSId = poDS->GetUndefinedSRID() at line 571 of ogrpgtablelayer.cpp (due to previous failed requests that have tried to fetch the srid of the table from geometry_columns), is due to permission problems on geometry_columns. The undefined SRID value (0 for postgis 2.0) could very well be that the table has been loaded with no SRID at all, in which case querying the first feature is useless.

So the easiest workaround would be to rewrite the test at line 355 of ogrpgresultlayer.cpp like that :

nSRID == poDS->GetUndefinedSRID() )

(It could also be argued that trying to guess the SRID from the SRID of the first geometry returned is an optimistic guess. Some corner and evil use cases could be : "SELECT SetSRID(geometry, random_number) FROM my_table" where random_number changes for each row)

comment:5 by strk, 10 years ago

Indeed I'd rather check the SRID of each and every feature in the set. You have to scan them all anyway, and you don't need to know their srid any earlier than on end, do you ? If there's a mismatch we could produce a WARNING and omit the .prj.

Beside, why does GetSRID() doesn't return UNDETERMINED_SRID directly ?

comment:6 by strk, 10 years ago

Now here's some more debugging:

PG: GetSpatialRef enter, nSRSId is -2 (undetermined is -2)
PG: GetSpatialRef: bHasPostGISGeometry, GeomTableName is (null)
PG: GetSpatialRef: nSRSId is still undetermined
PG: GetSpatialRef: did set nSRSId to -1 and and run SELECT ST_SRID("the_geom") FROM(select st_convexhull(the_geom) as the_geom from ( select 'SRID=4326;POINT(0 0)'::geometry as the_geom) as foo) AS ogrpggetsrid LIMIT 1
PG: GetSpatialRef: q run returned 1 tuples 
PG: GetSpatialRef: nSRSId is 4326 after the q run

Still... the resulting shapefile does NOT include a .prj file. Now, it is interesting that the _same_ debugging output results to the availability of a .prj file IFF the database user has access to the geometry_columns table.

comment:7 by strk, 10 years ago

Now I'm confused, all that nice code to set nSRSId, but then... it isn't used at the end of function, is it ?

The function ends with

return OGRPGLayer::GetSpatialRef()

So where does all the knowledge extracted by that function go ?

comment:8 by strk, 10 years ago

I realized the knowledge goes to nSRSId, which is a class member, so far so good. Then why the .prj is not produced is still an open question. Code generating .prj is taking another path ?

comment:9 by strk, 10 years ago

Alright, found. You were right this was not about geometry_column but about spatial_ref_sys !

PG: OGRPGLayer::GetSpatialRef: FetchSRS for 4326 is (nil)

I think that event should just trigger a WARNING and we'd be all done. Deal ?

comment:10 by strk, 10 years ago

The patch is against 1.9.2 as patched by the other patch attached to #4870 (should not be in conflict). It actually prints an ERROR but doesn't trigger a FAILURE return from ogr2ogr, and i don't think it should.

comment:11 by Even Rouault, 10 years ago

0001-Print-an-error-when-spatial_ref_sys-querying-fails.patch applied in r25148

comment:12 by Even Rouault, 10 years ago

Resolution: wontfix
Status: newclosed

Closing assuming r25148 is an acceptable way of resolving this.

Note: See TracTickets for help on using tickets.