ST_AsMVT: add feature id support

[stepankuzmin]

According to Vector Tile Specification v2.1 (​https://github.com/mapbox/vector-tile-spec/tree/master/2.1#42-features):

A feature MAY contain an id field. If a feature has an id field, the value of the id SHOULD be unique among the features of the parent layer.

But there is no way to set a feature id using ST_AsMVT.

[Algunenano]

This is a feature request. Moving it to 3.0.

PR by stepankuzmin with discussion in ​https://github.com/postgis/postgis/pull/274

[komzpa]

In 16808:

ST_AsMVT: Support for Feature ID.

Patch by Stepan Kuzmin.

Closes #4128
Closes ​https://github.com/postgis/postgis/pull/274

[Algunenano]

I have several objections to this change:

• It doesn't include documentation.
• The id column is added twice, once in the id field and one as a property. This duplicates information making tiles bigger for no reason.
• If there are multiple columns with the same name there will be extra work done.
• IMO it should throw if the type is invalid, not just silently ignore it. It's not documented which types are supported.
• The check is added in a hot path and it's done for each property for each feature. Instead it should be done, as with the geometry column, once at the start. I expect this to have an important impact on performance, even when not using this feature.

I'm reopening it to have a look into metrics and document this new feature properly.

[komzpa]

@Algunenano documentation for the new argument was added in this change. Did I miss something else that should have been documented?

[Algunenano]

@Algunenano documentation for the new argument was added in this change. Did I miss something else that should have been documented?

Yeah, sorry I missed that. I was checking Github PR and not the changeset. The only thing missing there is to define which are the valid types for it.

[komzpa]

Something is off with this change, it works locally and on Travis, but not on Drone:

[20:37] <strk> mvt tests crash backend
[20:37] <strk> https://drone.osgeo.org/postgis/postgis/1325/3
[20:38] <strk> since r16808

[strk]

If you have docker installed you can investigate the environment of Dronie with:

docker run -ti --rm \
 docker.kbt.io/postgis/build-test:trisquel2 \
 /bin/bash

I did, to check protoc version:

root@c4bdbe9eb096:/# protoc-c --version
protobuf-c 1.3.0
libprotoc 3.5.1

[strk]

If you have drone installed (version 0.5) you can reproduce the Dronie's builds locally with changing into PostGIS source tree and running:

  drone exec

[robe]

debbie is getting an mvt crash as well:

​https://debbie.postgis.net/job/PostGIS_Regress/10058/consoleFull

 mvt .. failed (diff expected obtained: /var/lib/jenkins/workspace/postgis/tmp/3_0_pg10w64/test_125_diff)
-----------------------------------------------------------------------------
--- mvt_expected	2018-09-16 16:55:14.518502234 +0000
+++ /var/lib/jenkins/workspace/postgis/tmp/3_0_pg10w64/test_125_out	2018-09-16 17:10:02.610250474 +0000
@@ -47,53 +47,7 @@
 PG43 - ON |MULTIPOLYGON(((5 5,0 0,10 0,5 5)),((0 10,5 5,10 10,0 10)))
 PG43 - OFF|MULTIPOLYGON(((5 5,-1 -1,11 -1,5 5)),((5 5,11 11,-1 11,5 5)))
 TG1|GiEKBHRlc3QSDBICAAAYASIECTLePxoCYzEiAigBKIAgeAI=
-TG2|GiMKBHRlc3QSDhICAAAYASIGETLePwIBGgJjMSICKAEogCB4Ag==
-TG3|GiYKBHRlc3QSERICAAAYAiIJCQCAQArQD88PGgJjMSICKAEogCB4Ag==
-TG4|GiYKBHRlc3QSERICAAAYAiIJCQCAQArQD88PGgJjMSICKAEogCB4Ag==
-TG5|GjAKBHRlc3QSGxICAAAYAiITCQL+PwrQD88PCc0Pzg8K0A/PDxoCYzEiAigBKIAgeAI=
-TG6|GjIKBHRlc3QSHRICAAAYAyIVCUbsPxoxEwonPAkPCTEeEhQUCh0PGgJjMSICKAEogCB4Ag==
-TG7|Gj0KBHRlc3QSKBICAAAYAyIgCVCwPxIKFDEdDwkAFCIyHh0eJwkAJw8JKBQSEwkAFA8aAmMxIgIo
-ASiAIHgC
-TG8|GiEKBHRlc3QSDBICAAAYASIECTLePxoCYzEiAigBKIAgeAI=
-TG9|GiMKBHRlc3QSDhICAAAYASIGETLeP2VGGgJjMSICKAEogCB4Ag==

[Algunenano]

The id value is being assigned directly from the datum, which is will cause issues such as those crashes. I also see that it doesn't do any bound checking (so negative values ca be assigned) which could cause issues too.

I'll try to have it fixed and fully documented (negative or NULL values, invalid types, column name not found…) tomorrow.

[Algunenano]

My proposed changes:

• Obviously fix the bug casting the Datum to the appropriate type.
• Match the geometry column behaviour: Do the validation once, including type, and fail if not found.
• Do not include the id column as a property too. If you want the information duplicated you can add the column twice. Document this.
• Silently ignore (leave them as id not set) NULLs and negative values (not supported by MVT). If you want a default value you can set it up before passing the information to the aggregation function. Document this.
• The column is chosen as the first column with the correct name and valid types (smallint, integer, bigint). Properties inside JSON values are not supported. Document this.
• Add tests around all these behaviours.

PR coming.

[Algunenano]

WIP in ​https://github.com/postgis/postgis/pull/303

[algunenano]

In 16825:

St_AsMVT: Fix id support, and extra tests and documentation

Closes #4128
Closes ​https://github.com/postgis/postgis/pull/303