New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PostGIS layer doesn't return a computed LayerGetExtent #3585

Closed
mapserver-bot opened this Issue Apr 3, 2012 · 12 comments

Comments

Projects
None yet
2 participants
@mapserver-bot

mapserver-bot commented Apr 3, 2012

Reporter: guillaume
Date: 2010/10/25 - 12:11
Trac URL: http://trac.osgeo.org/mapserver/ticket/3585
msPostGISLayerGetExtent function is using hard coded values for a result. A TODO is present :
** TODO: Update to use proper PostGIS functions to pull
** extent quickly and accurately when available.

I'm willing to fix this in order to have precise BBOX computations in GetCapabilities without having to set any wms_extent or EXTENT for the LAYER definition.

An OWS interface approach would be to have msPostGISLayerGetExtent return MS_FALSE and then use and transform Map's extent for layer extent.

But I guess fixing this directly in msPostGISLayerGetExtent would be great. I'd be glad to help, but my small C and mapserver hacking experience require hints.

@mapserver-bot

This comment has been minimized.

Show comment
Hide comment
@mapserver-bot

mapserver-bot Apr 3, 2012

Author: guillaume
Date: 2010/11/04 - 23:53
I succeed in writing a patch, even if it was my first C experience. Tested with QGis which is now able to display a postgis layer which doesn't have an EXTENT explicitly set. Still work is extent or wms_extent is set though.
Please review, I did my best but it may be not perfect.
Regards,

Guillaume

mapserver-bot commented Apr 3, 2012

Author: guillaume
Date: 2010/11/04 - 23:53
I succeed in writing a patch, even if it was my first C experience. Tested with QGis which is now able to display a postgis layer which doesn't have an EXTENT explicitly set. Still work is extent or wms_extent is set though.
Please review, I did my best but it may be not perfect.
Regards,

Guillaume

@mapserver-bot

This comment has been minimized.

Show comment
Hide comment
@mapserver-bot

mapserver-bot Apr 3, 2012

Author: pramsey
Date: 2010/11/05 - 00:07
The patch looks structurally fine, but think about what will happen to MapServer in the event that you run this patch on a layer with 10000000 features in it. It will take 2 minutes to return the WMS capabilities file. And if you get two requests for capabilities simultaneously all bets are off.

The reason there is no extent code is that there is no "easy, fast, correct" solution to the problem. You can get a fast answer with estimated_extent(), but it won't be correct (and occasionally will return null, if there are no statistics available). You can get the correct answer with the approach you have taken, but it will be cripplingly slow in many cases. Since most data extents don't actually change very often, I have long thought that the "least worse" solution is just the current one: have people set their extents manually with wms_extent.

mapserver-bot commented Apr 3, 2012

Author: pramsey
Date: 2010/11/05 - 00:07
The patch looks structurally fine, but think about what will happen to MapServer in the event that you run this patch on a layer with 10000000 features in it. It will take 2 minutes to return the WMS capabilities file. And if you get two requests for capabilities simultaneously all bets are off.

The reason there is no extent code is that there is no "easy, fast, correct" solution to the problem. You can get a fast answer with estimated_extent(), but it won't be correct (and occasionally will return null, if there are no statistics available). You can get the correct answer with the approach you have taken, but it will be cripplingly slow in many cases. Since most data extents don't actually change very often, I have long thought that the "least worse" solution is just the current one: have people set their extents manually with wms_extent.

@mapserver-bot

This comment has been minimized.

Show comment
Hide comment
@mapserver-bot

mapserver-bot Apr 3, 2012

Author: pramsey
Date: 2010/11/05 - 00:08
Congratulations on your first C patch, BTW. Fun, isn't it?

mapserver-bot commented Apr 3, 2012

Author: pramsey
Date: 2010/11/05 - 00:08
Congratulations on your first C patch, BTW. Fun, isn't it?

@mapserver-bot

This comment has been minimized.

Show comment
Hide comment
@mapserver-bot

mapserver-bot Apr 3, 2012

Author: guillaume
Date: 2010/11/05 - 00:19
Very fun indeed, mostly the * and malloc parts... as well as debugging of course !
The problem is most people don't know that they have to set a wms_extent to have a correct bbox for their PostGIS layer. They don't have to do that for shapefiles and even Oracle Layer has got a computation. On the top of that, the GetCapabilities doesn't echo any warning about it. Maybe sending back MS_FAILURE, which causes a warning message in the capabilities would be more useful for people. I globally think that the layer extent should be the map extent if no specific extent is present in the LAYER block. But that would lead to depreciate what has been done on shapefiles and others. I don't know what to think. Maybe a test if layer->type == MS_POSTGIS_LAYER && layer->extent == NULL && layer->metadata->wms_extent == NULL then take the map's extent for this layer's extent ?
Best regards
Guilaume

mapserver-bot commented Apr 3, 2012

Author: guillaume
Date: 2010/11/05 - 00:19
Very fun indeed, mostly the * and malloc parts... as well as debugging of course !
The problem is most people don't know that they have to set a wms_extent to have a correct bbox for their PostGIS layer. They don't have to do that for shapefiles and even Oracle Layer has got a computation. On the top of that, the GetCapabilities doesn't echo any warning about it. Maybe sending back MS_FAILURE, which causes a warning message in the capabilities would be more useful for people. I globally think that the layer extent should be the map extent if no specific extent is present in the LAYER block. But that would lead to depreciate what has been done on shapefiles and others. I don't know what to think. Maybe a test if layer->type == MS_POSTGIS_LAYER && layer->extent == NULL && layer->metadata->wms_extent == NULL then take the map's extent for this layer's extent ?
Best regards
Guilaume

@mapserver-bot

This comment has been minimized.

Show comment
Hide comment
@mapserver-bot

mapserver-bot Apr 3, 2012

Author: pramsey
Date: 2010/11/05 - 00:32
I think a warning in the XML comments would be a good idea. I think having the extent calculation by default would lead to lots of "why is mapserver slow" questions, but having an XML warning would probably lead people setting up their WMS for the first time to recognize the manual extent issue and address it (that has certainly been my workflow in doing Mapserver WMS -- try a config and read the capabilities and remove the warnings).

I'm less certain of any automatic filling in of extents... though I suppose your approach is no worse than the current Inf/-Inf extent. Except, what happens in the care where all layers are postgis layers? I can't recall, is MAP.EXTENT a required mapfile field?

mapserver-bot commented Apr 3, 2012

Author: pramsey
Date: 2010/11/05 - 00:32
I think a warning in the XML comments would be a good idea. I think having the extent calculation by default would lead to lots of "why is mapserver slow" questions, but having an XML warning would probably lead people setting up their WMS for the first time to recognize the manual extent issue and address it (that has certainly been my workflow in doing Mapserver WMS -- try a config and read the capabilities and remove the warnings).

I'm less certain of any automatic filling in of extents... though I suppose your approach is no worse than the current Inf/-Inf extent. Except, what happens in the care where all layers are postgis layers? I can't recall, is MAP.EXTENT a required mapfile field?

@mapserver-bot

This comment has been minimized.

Show comment
Hide comment
@mapserver-bot

mapserver-bot Apr 3, 2012

Author: guillaume
Date: 2010/11/05 - 00:40
MAP.EXTENT is not mandatory, but as the doc says :
EXTENT [minx] [miny] [maxx] [maxy]
The spatial extent of the map to be created. In most cases you will need to specify this, although MapServer can sometimes (expensively) calculate one if it is not specified.
We maybe need another test here, and if no extent is specified nowhere, send the user on the moon.
Another idea : can't we add a test on the number of records, and if it is reasonnable (<100000 ? <1000000 ?) compute the precise extent, if not send back MS_FAILURE ?

mapserver-bot commented Apr 3, 2012

Author: guillaume
Date: 2010/11/05 - 00:40
MAP.EXTENT is not mandatory, but as the doc says :
EXTENT [minx] [miny] [maxx] [maxy]
The spatial extent of the map to be created. In most cases you will need to specify this, although MapServer can sometimes (expensively) calculate one if it is not specified.
We maybe need another test here, and if no extent is specified nowhere, send the user on the moon.
Another idea : can't we add a test on the number of records, and if it is reasonnable (<100000 ? <1000000 ?) compute the precise extent, if not send back MS_FAILURE ?

@mapserver-bot

This comment has been minimized.

Show comment
Hide comment
@mapserver-bot

mapserver-bot Apr 3, 2012

Author: pramsey
Date: 2010/11/05 - 00:50
Number of records is another thing that is expensive to calculate exactly (select count(*) from table forces a seqscan), though statistics on it are probably more available than on spatial extent (but any appeal to statistics has to handle the case where they have not been gathered yet).

Fall back to Inf/-Inf if there's nothing else available, I think.

mapserver-bot commented Apr 3, 2012

Author: pramsey
Date: 2010/11/05 - 00:50
Number of records is another thing that is expensive to calculate exactly (select count(*) from table forces a seqscan), though statistics on it are probably more available than on spatial extent (but any appeal to statistics has to handle the case where they have not been gathered yet).

Fall back to Inf/-Inf if there's nothing else available, I think.

@mapserver-bot

This comment has been minimized.

Show comment
Hide comment
@mapserver-bot

mapserver-bot Apr 3, 2012

Author: guillaume
Date: 2010/11/05 - 08:31
Inf/-Inf is leading to misunterstanding. As the values are reprojected in the layer's projection, you see values in getCapabilities which seem computed (different values for min and max etc). When QGIS doesn't show your layer because the view's bbox doesn't intersect the layer's miscomputed bbox, you can get crazy pretty quickly.
I would suggest to send back MS_FAILURE which has the huge advantage of displaying a warning in the getCapabilities and leads to better behaviour for the client and better understanding for the user.

mapserver-bot commented Apr 3, 2012

Author: guillaume
Date: 2010/11/05 - 08:31
Inf/-Inf is leading to misunterstanding. As the values are reprojected in the layer's projection, you see values in getCapabilities which seem computed (different values for min and max etc). When QGIS doesn't show your layer because the view's bbox doesn't intersect the layer's miscomputed bbox, you can get crazy pretty quickly.
I would suggest to send back MS_FAILURE which has the huge advantage of displaying a warning in the getCapabilities and leads to better behaviour for the client and better understanding for the user.

@mapserver-bot

This comment has been minimized.

Show comment
Hide comment
@mapserver-bot

mapserver-bot Apr 3, 2012

Author: pramsey
Date: 2011/03/17 - 15:10
I started implementing and then realized there is a whole other case which messes things up, the "the_geom from (select foo from bar) as subq" case, where the data source isn't a table at all, but a subquery. You can use estimated_extent() to get the extent of tables quickly, but it does nothing for subqueries.

mapserver-bot commented Apr 3, 2012

Author: pramsey
Date: 2011/03/17 - 15:10
I started implementing and then realized there is a whole other case which messes things up, the "the_geom from (select foo from bar) as subq" case, where the data source isn't a table at all, but a subquery. You can use estimated_extent() to get the extent of tables quickly, but it does nothing for subqueries.

@mapserver-bot

This comment has been minimized.

Show comment
Hide comment
@mapserver-bot

mapserver-bot Apr 3, 2012

Author: dmorissette
Date: 2011/09/09 - 03:31
Bump... this is still an issue in MapServer 6.x and trunk:
http://lists.osgeo.org/pipermail/mapserver-users/2011-September/070003.html

mapserver-bot commented Apr 3, 2012

Author: dmorissette
Date: 2011/09/09 - 03:31
Bump... this is still an issue in MapServer 6.x and trunk:
http://lists.osgeo.org/pipermail/mapserver-users/2011-September/070003.html

@mapserver-bot

This comment has been minimized.

Show comment
Hide comment
@mapserver-bot

mapserver-bot Apr 5, 2012

attachment http://trac.osgeo.org/mapserver/attachment/ticket/3585/msPostGISLayerGetExtent.patch :

   Patch to get correct computation of extent for a postgis layer

mapserver-bot commented Apr 5, 2012

attachment http://trac.osgeo.org/mapserver/attachment/ticket/3585/msPostGISLayerGetExtent.patch :

   Patch to get correct computation of extent for a postgis layer
@mapserver-bot

This comment has been minimized.

Show comment
Hide comment
@mapserver-bot

mapserver-bot Feb 23, 2016

This is an automated comment

This issue has been closed due to lack of activity. This doesn't mean the issue is invalid, it simply got no attention within the last year. Please reopen with missing/relevant information if still valid.

Typically, issues fall in this state for one of the following reasons:

  • Hard, impossible or not enough information to reproduce
  • Missing test case
  • Lack of a champion with interest and/or funding to address the issue

mapserver-bot commented Feb 23, 2016

This is an automated comment

This issue has been closed due to lack of activity. This doesn't mean the issue is invalid, it simply got no attention within the last year. Please reopen with missing/relevant information if still valid.

Typically, issues fall in this state for one of the following reasons:

  • Hard, impossible or not enough information to reproduce
  • Missing test case
  • Lack of a champion with interest and/or funding to address the issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment