Skip to content
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

Ensure PG 16+ generate same result as PG15 and before #1651

Open
nyurik opened this issue Jan 12, 2025 · 10 comments · Fixed by #1675
Open

Ensure PG 16+ generate same result as PG15 and before #1651

nyurik opened this issue Jan 12, 2025 · 10 comments · Fixed by #1675
Assignees
Labels
ci/cd serving Related to web serving component

Comments

@nyurik
Copy link
Member

nyurik commented Jan 12, 2025

It seems there might be some cases when Martin does not generate the same result in PG 16 as in versions before. More importantly, sometimes martin-cp generates a different number of tiles from it too. We must get a very clear understanding of why it differs, and try to make them the same again.

see also #1642 for a great analysis of this issue by @sharkAndshark

@sharkAndshark
Copy link
Collaborator

sharkAndshark commented Jan 13, 2025

Less features returned by PostGIS whose version below 3.5.?

Looks like This is an upstream issue. I've post an issue there.

To verify and duplicate this:

  1. Init table with this sql
  2. Run the query with zxy == 1 1 1 or 1 1 0 and watch the results.
SELECT
      ST_AsMVT(tile, 'table_source', 4096, 'geom')
    FROM (
      SELECT
        ST_AsMVTGeom(
            ST_Transform(ST_CurveToLine("geom"::geometry), 3857),
            ST_TileEnvelope($1::integer, $2::integer, $3::integer),
            4096, 64, true
        ) AS geom
        , "gid"
      FROM
        "public"."table_source"
      WHERE
        "geom" && ST_Transform(ST_TileEnvelope($1::integer, $2::integer, $3::integer, margin => 0.015625), 4326)
      
    ) AS tile;

I jsut paste the results here in case someone need it:

z x y pg_version(docker image) source st_asmvt is_empty
1 1 1 postgis/postgis:14-3.3 table_source (BLOB) 2 bytes yes
1 1 0 postgis/postgis:14-3.3 table_source (BLOB) 2 bytes yes
1 1 1 postgis/postgis:14-3.5 table_source (BLOB) 607 bytes no
1 1 0 postgis/postgis:14-3.5 table_source (BLOB) 1.18 KB no

@sharkAndshark
Copy link
Collaborator

Different features order with various PostGIS versions

I think we could pay less attention on this as there is no guarantee the orders should be same without an order by.
Just update the test.sh to ignore the order with ogrmerge of gdal. I've tried and it works well.

@sharkAndshark
Copy link
Collaborator

sharkAndshark commented Jan 27, 2025

pmt2_0_0_0.png.txt changed after update to potsgis:14-3.3

But defintely postgis couldn't be the reason. There must be something else.

From this

tests/output/configured/pmt2_0_0_0.png: RIFF (little-endian) data, Web/P image, with alpha, 511+1x511+1

to

tests/output/configured/pmt2_0_0_0.png: RIFF (little-endian) data, Web/P image

@sharkAndshark
Copy link
Collaborator

So now we have 3 problems..

  1. Features order changed
  2. MBTiles content changed
  3. pmt2_0_0_0.png changed

@sharkAndshark
Copy link
Collaborator

sharkAndshark commented Feb 1, 2025

It may take too long to wait the PostGIS upstream issue be reseloved.
I think we could try to fix the features order first.
But at first we should back to postgis 14-3.3 with pr #1642 to get a stable ci.

@CommanderStorm CommanderStorm added the serving Related to web serving component label Feb 1, 2025
@sharkAndshark sharkAndshark self-assigned this Feb 2, 2025
sharkAndshark added a commit that referenced this issue Feb 2, 2025
Try to fix #1651 
- [x] Ignore `*.pbf` as the features ordering has changed
- [x] Use `ogrmerge of gdal` to generate the description text of `*.pbf`
files
- [x] Add "gid" to `function_Mixed_Name.sql` for `jq` sorting 
- [x] Upgrade to PostGIS:15-3.4
- [x] Add verification of gdal-bin in `test.sh` 
- [x] Just bless
- [x] Update doc
@sharkAndshark sharkAndshark reopened this Feb 3, 2025
@sharkAndshark
Copy link
Collaborator

sharkAndshark commented Feb 3, 2025

Version matrix(all tested on docker image only):

pg postgis features in pg tile ordering changed mbtiles content changed
14 3.3.2 no no
15 3.3.4 yes no
16 3.3 no docker image no docker image
17 3.3 no docker image no docker image
14.13.0 3.4.3 no no
15.8 3.4.3 yes no
16 3.4 yes no
17 3.4 yes no
15 3.5 yes yes
16 3.5 yes yes
17 3.5 yes yes

Things to track:

status PR
Back to postgis:14-3.3 first to get a stable CI Fixed #1642
Features changed and upgrade to postgis 15-3.4 Fixed #1675
MBTiles changed and upgrade to postgis 16-3.5 Working on #1679
why pmt2_0_0_0.png changed? help wanted help wanted
Add a warning when PostGIS version below than 3.5.? help wanted help wanted

@sharkAndshark
Copy link
Collaborator

sharkAndshark commented Feb 3, 2025

@CommanderStorm suggested we may need to add a warning when PostGIS version below than 3.5.?.

I personally think we should add the warning with an issue link which has more deep/detail information about this bug. Seems we have no such a link about this by now.

I remember that you said someone talk about something similar on slack, would you mind @me there? My name is Lucas on slack

CC @nyurik

@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Feb 3, 2025

with an issue link which has more deep/detail information about this bug. Seems we have no such a link about this by now.

Linking this very issue -> #1651 (comment) might be a good choice.. what do you think? 🤔

CommanderStorm pushed a commit that referenced this issue Feb 4, 2025
Related to #1651 

- [x] Upgrade our postgis version to 16-3.5
- [x] Update mbtiles fixtures
@sharkAndshark
Copy link
Collaborator

sharkAndshark commented Feb 4, 2025

Good idea. @CommanderStorm And I update some descriptions here to make it more clearly.
(English is not my first language so feel free to edit them if needed)

@CommanderStorm
Copy link
Collaborator

Your English is excellent, no complaints/.. there. Don't worry about that ever ^^

(To be fair: it is also not a native language for me)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/cd serving Related to web serving component
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants