Implement msPostGISLayerGetExtent (#3585)

[szekerest]

No description provided.

[tbonfort]

#3585 gives some insight as to why this was not implemented, c.f @pramsey:

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.

Unless I am mistaken I don't see any code in this PR to tackle this issue, and would expect that this feature be disabled by default unless voted forward by the PSC.

[msmitherdc]

This is the reason we set the extents via metadata for Oracle and PostGIS for all of our datasets. Its just too expensive to calculate on the fly.

[szekerest]

@tbonfort Not sure what you mean. If the user sets the extent of the layer manually (probably in the mapfile) that overrides the postgis extent calculation, see msLayerGetExtent in maplayer.c. But in most cases no one would like to set the extent manually if it can be returned by postgis as well. I don't see any comments about the performance issues with ST_Extent it may probably use statistics to make it fast enough.

[tbonfort]

@szekerest : Paul explicitely chose to disable that functionality as it may cause a denial of service (st_extent is not optimized and does not use statistics). Your patch goes against his intention and therefore should imho at least be voted on by the PSC before being applied. If the PSC were to approve that this functionality be on by default, then the migration guide should also prominently expose the fact that mapfiles need to be updated in order to avoid this denial of service.

[szekerest]

@tbonfort I don't think that such driver specific issues require an RFC to be issued and I also don't see where 'Paul explicitely chose to disable that functionality as it may cause a denial of service'. Do we have any comments in the mapserver documentation about that this functionality will never be implemented? On the contrary I see Paul has started to implement that, but he had problems with the subqueries (which anyway has been addressed)

It must also be mentioned that this functionality has been added to master and not to any of the stable branches. In this regard everyone should not expect that mapserver will behave exactly the same as the previous version. Services should either specify the extent manually or let the drivers to calculate that using the driver specific approach. This behaviour is already documented in the mapfile documentation, so returning invalid extents by a service should be considered as a mistake:

EXTENT [minx] [miny] [maxx] [maxy]
The spatial extent of the data. In most cases you will not need to specify this, but it can be used to avoid the speed cost of having MapServer compute the extents of the data. An application can also possibly use this value to override the extents of the map.

Anyway I'll open up a discussion about this topic, but I won't agree to force PostgisLayerGetExtent never be implemented is a reasonable route (in contrast to almost all of the other drivers) to avoid performance issues.

[tbonfort]

@szekerest I'm not opposed to this as there clearly is a need for this feature. Although not written in the documentation, to me

I have long thought that the "least worse" solution is just the current one: have people set their extents manually with wms_extent

is quite clear to express that the feature was intentionally left out, and therefore would need to be brought up for discussion.

[szekerest]

@tbonfort This feature was implemented upon user request (and it has been confirmed to work in this way) so there's no doubt it is needed. The citation you mentioned doesn't mean we should never implement msPostgisLayerGetExtent just that the "least worse" solution is to set the extent manually, which shall continue to be an option here as well.

[pramsey]

My word is certainly not god, and other PSC members (@dmorrisette) have asked in the past that I implement the naive extent calculation. I think if @szekerest is willing to accept the implications of the change, and deal with support questions on it, then the patch is OK. My opinion, however, remains the same: when it fails, it'll fail ugly and non-obvious and unexpected. The current behaviour fails fast and more obviously, which is preferable.

[woodbri]

Can someone provide a little background on why and where layer extents are used?

I have never used them and in fact never noticed that there was an extents at the layer level. I have always set the extents at the map level, so I guess by default the layers inherit that.

I presume that the layer extents are checked to see if the whole layer can be rejected if the image extents does not overlap with the layer extents. Is this correct?

Assuming that it is correct, are we changing this behavior? ie: the layer inheriting from the map extents? (assuming that it did). I would not want to take a performance penalty or not having this and would not want to have to change all my mapfiles to add the extents to all the layers.

[pramsey]

In a GIS-style map, you might have 'zoom to layer' functionality. Apps that only consume a few layers might use just the combined extents to drive zoom-to-extent behaviour.

[woodbri]

Ahh, that makes sense, so how is this queried from the application? Is there a CGI function? or OWS request that trigger this? I guess where my thought is going is that if it is a specific trigger mechanism then it is more controlled behavior than if it is some hidden trigger or something that happens all the time.

I also like Even's suggestion of EXTENT AUTO with the absence of EXTENT at the layer level should default to the MAP EXTENT and if that is not defined then throw an error.

[szekerest]

Seems we keep 2 different pathes of the discussion with similar comments, I'd suggest to replicate all the comments to the mailing list too.

[szekerest]

@pedros007 I wonder how your layer definition is looking like in this case

[pedros007]

My data statement for the layer is maybe a bit verbose? My schema has a base_tiles table with a geometry. I have a projects table composed of many tiles. The tiles table is a join table to base_tiles. The WFS request that causes Postgres to report ERROR: syntax error at or near "0" at character 29 STATEMENT: SELECT ST_Extent(geom) FROM 0 is:

http://localhost/mapserv?service=WFS&VERSION=1.1.0&REQUEST=getfeature&typename=tiles&srsname=EPSG:4326&OUTPUTFORMAT=geojson&project_id==1&partition_quadkey=='021320320'"

The error goes away if I add "ows_extent" "-180 -90 180 90" to my METADATA section.

The quadkeys referenced in the query are Bing Maps tiling scheme. Partitions are level 9, tiles are level 12.

    NAME tiles
    TYPE POLYGON
    METADATA
      "ows_title"             "tiles"
      "ows_description"       "Tile Extents"
      "gml_types"             "auto"
      "gml_include_items"     "all"
      "geojson_include_items" "all"
    END
    STATUS OFF
        INCLUDE "postgis.inc.map"
        DATA "geom FROM (
          SELECT
            base_tiles.geom,
            base_tiles.id as base_tile_id,
            base_tiles.quadkey,
            substring(base_tiles.quadkey from 0 for 10) as partition_quadkey,
            base_tiles.x,
            base_tiles.y,
            base_tiles.level,
            tiles.project_id,
            tiles.id as tile_id,
            tiles.partition_id as partition_id,
            tile_stacks.status
          FROM base_tiles
            INNER JOIN tiles
            ON base_tiles.id = tiles.base_tile_id
            INNER JOIN tile_stacks
            ON tile_stacks.tile_id = tiles.id
          WHERE ST_Intersects(base_tiles.geom, !BOX!) and project_id %project_id% and substring(base_tiles.quadkey from 0 for 10) %partition_quadkey%
          ) AS subquery USING UNIQUE base_tile_id USING SRID=4326"
        VALIDATION
          # %project_id% must be one or more integers
          'project_id' '^=[0-9]+$'
          'default_project_id' 'IS NOT NULL'
          'partition_quadkey' "^='[0-3]{9}'$"
          'default_partition_quadkey' "like '%'"
        END
END

Edit: Also, I should mention that with mapserver-6.4, I used FILTER on my query:

...
 WHERE ST_Intersects(base_tiles.geom, !BOX!)
) AS subquery USING UNIQUE base_tile_id USING SRID=4326"
FILTER "project_id %project_id% and partition_quadkey %partition_quadkey%"
...

Mapserver-7 seems to have deprecated the FILTER syntax for SQL queries in preference for adding to the WHERE clause. So I updated my query like so:

...
 WHERE ST_Intersects(base_tiles.geom, !BOX!) and project_id %project_id% and partition_quadkey %partition_quadkey%
) AS subquery USING UNIQUE base_tile_id USING SRID=4326"
...

[szekerest]

It seems that the table name is not correctly identified with this complex expression (the innermost "from 0" is considered to define the table name). I'll see how that could be work arounded.

[pedros007]

Thanks, @szekerest . I opened #5365 to track this.