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

Add more validations to .mbtiles detection #741

Open
2 tasks
nyurik opened this issue Jul 3, 2023 · 6 comments · May be fixed by #1689
Open
2 tasks

Add more validations to .mbtiles detection #741

nyurik opened this issue Jul 3, 2023 · 6 comments · May be fixed by #1689
Labels
good first issue help wanted mbtiles Anything related to Martin's MBTiles support

Comments

@nyurik
Copy link
Member

nyurik commented Jul 3, 2023

As part of mbtiles type detection, we should catch some common issues:

  • detect if tiles (simple) or map (deduplicated) tables have an index by z,x,y columns
  • ???

In case of a problem, use a warn! macro to print a warning, unless we are certain the issue makes it impossible to use a file, in which case it should be appropriate error (add it to the error enum).

A bigger discussion is if mbtiles should stop using logging, and return some Vec<MbtilesWarning> if non-critical issues are detected.

@nyurik nyurik added help wanted good first issue mbtiles Anything related to Martin's MBTiles support labels Jul 3, 2023
@Auspicus
Copy link

@nyurik I noticed this was causing absolutely horrific tail latency for tiles. We were using .mbtiles with no index on map or tiles and this was causing >1s query times on and off. Surprisingly SQLite was still able to deliver some good results on-and-off. But there was a LARGE variation in query times.

I'd be happy to take this on!

@nyurik
Copy link
Member Author

nyurik commented Feb 14, 2025

Thanks @Auspicus , that would be awesome!

@Auspicus
Copy link

Auspicus commented Feb 14, 2025

It looks like this feature more or less exists inside the mbtiles validate tool already. But the martin server does not warn the user when their MBTiles do not contain indices which could lead to sub-par performance.

I'm thinking the following could be useful:

  • warnings when this is the case: easily ignored and not a breaking change but will be useful for alerting users of potential performance issues: "Using .mbtiles without an index on the underlying data can result in slower queries in most cases. It's encouraged to index either the map or tiles table."
  • a way to not serve MBTiles that are lacking indices via config (ie. config.mbtiles.ignore_tiles_without_index) which defaults to false in case the user is dynamically uploading tiles

This could slow down the startup slightly and might require another config flag to turn off in the case the user wants to skip this check. Is it OK to make concessions on the startup time in the interest of ensuring a good experience with the server?

So to summarize:

  • config config.mbtiles.skip_mbtiles_index_check (default: false) - avoids running this check on startup
  • config config.mbtiles.ignore_mbtiles_without_index (default: false) - ignores .mbtiles without an index
  • warning on startup when .mbtiles file detected with no index (unless it's skipped via previous config)

Thoughts? @nyurik

@nyurik
Copy link
Member Author

nyurik commented Feb 14, 2025

Thx, I agree that something like this is needed. I am not a big fan of tons of bools, so perhaps something like this?

validate =
  - skip -- do not validate
  - fast -- quickly check the file (default)
  - thorough -- do a slow check of everything

on_invalid =
  - warn -- print warning message, and abort if the error is critical  (default)
  - ignore_source -- skip this source
  - abort -- do not start Martin on any warnings

TODO

  • Perhaps find a better name for on_invalid because the file might be valid but inefficient (no index)

Config structure

We currently have this structure:

mbtiles:
  paths:
    # scan this whole dir, matching all *.mbtiles files
    - /dir-path
    # specific mbtiles file will be published as mbtiles2 source
    - /path/to/mbtiles.mbtiles
  sources:
    # named source matching source name to a single file
    mb-src1: /path/to/mbtiles1.mbtiles

So we may need to be able to add both validate and on_invalid keys to 3 places - to the "root" (overrides the default ones), and to each place a path string is used at the moment:

mbtiles:
  # overrides built in defaults
  validate: skip
  on_invalid: abort
  paths:
    # scan this whole dir, matching all *.mbtiles files
    - path: /dir-path
      # applies to all files in the dir
      validate: skip
      on_invalid: abort
    # specific mbtiles file will be published as mbtiles2 source
    - path: /path/to/mbtiles.mbtiles
      # applies to the given file
      validate: skip
      on_invalid: abort
  sources:
    # named source matching source name to a single file
    mb-src1:
      path: /path/to/mbtiles1.mbtiles
      # applies to the named source
      validate: skip
      on_invalid: abort

CLI arguments

The same values can be used from the CLI: --validate-mbtiles skip --on-invalid-mbtiles abort

@Auspicus
Copy link

Auspicus commented Feb 15, 2025

@nyurik I've made a start here, haven't gotten around to adding the config to each individual source / path yet, but the integration tests seem to have caught an issue with the tests/fixtures/webp.mbtiles file? It appears the tile_data column doesn't have the expected BLOB type: https://github.com/maplibre/martin/actions/runs/13342149377/job/37268254989#step:10:217

If you get a chance to review the PR so far, I'd love to know if I'm going in the right direction.

Cheers!

@CommanderStorm
Copy link
Collaborator

If you get a chance to review the PR so far, I'd love to know if I'm going in the right direction.

Not who you asked but chiming in anyway.
Seems pretty solid. 👍🏻

What would be really helpful for this is to add an integration test here.
Adding integration tests prevents features from breaking/being broken in the future.

Slightly offtopic (read not a reuqirement, but closely related work):
Apparently, the shown error message could use a slight pass (Invalid data format for MBTile file tests/fixtures/mbtiles/webp.mbtiles does not really tell our users what is wrong..).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue help wanted mbtiles Anything related to Martin's MBTiles support
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants