go-spatial / tegola

Tegola is a Mapbox Vector Tile server written in Go
http://tegola.io/
MIT License
1.3k stars 197 forks source link

providers/postgis: remove ST_AsBinary(geom) requirement for sql statements #180

Open ARolek opened 7 years ago

ARolek commented 7 years ago

The ST_AsBinary(geom) requirement in SQL statements is a wart that I would like to get rid of. The underlying postgres driver (pgx) supports binary transfer protocol. We should be able to fetch request using the binary protocol rather than the text protocol to remove this wart.

When this task is worked on, make sure to address the outstanding TODO here.

tomass commented 6 years ago

Would it be enough to:

  1. Add a new configuration parameter: geometry_type=point|linestring|polygon|multipoint|multilinestring|multipolygon
  2. In tegola/postgis.go function layerGeomType check if geometry_type is specified, if so - use geometry type from there, otherwise - do the "old" logic - replace _stasbinary to _stgeometrytype and execute the query.
ARolek commented 6 years ago

@tomass

  1. I don't think this will address the topic issue, but this is absolutely something I want to add. This will help when we can't sniff the geom type appropriately. A new issue needs to be opened around this config param addition.
  2. Ideally we can drop the ST_AsBinary requirement, I just need to look a bit more into it. I think we need to return everything in binary which should also give us a performance improvement.

Are you interested in helping with some research?

tomass commented 6 years ago

@ARolek yes, I'm interested. @paumas has already done some research on this:

  1. Removing (disabling) layerGeomType function gives warning in different places where geometry type is used (like displaying list of layers provided and generating all.json info), but vector tiles are rendered just fine.
  2. WKB returned by _STAsBinary already contains geometry type.
ARolek commented 6 years ago

@tomass

  1. Correct, the tiles don't need this, it's just used for meta data and generating a style for the built in viewer.
  2. Correct, the main goal for dropping the ST_AsBinary requirement is for usability.

If we were to remove auto sniffing the geometry type we could purge a bunch of complex code. It might be best overall to have the user define the geom type in the config file instead of sniffing for it, the downside being more work required by the end user.

tomass commented 6 years ago

Code for sniffing is already there. So maybe both methods could be supported? Somebody could then specify geometry type only for advanced usage (like returning WKB pre-calculated in materialized view or similar). Leaving the sniffing code would also remove the necessity to update the config after upgrading Tegola. But it is up to you to decide.

ARolek commented 6 years ago

@tomass my initial plan was to support both. The logic would essentially be:

This would need to be implemented for both PostGIS and GeoPackage data providers.