Skip to content

fix(places): resolve places on PostgreSQL without PostGIS - #36

Open
torvalstrom wants to merge 1 commit into
thrillfall:mainfrom
torvalstrom:fix/postgres-place-lookup-without-postgis
Open

fix(places): resolve places on PostgreSQL without PostGIS#36
torvalstrom wants to merge 1 commit into
thrillfall:mainfrom
torvalstrom:fix/postgres-place-lookup-without-postgis

Conversation

@torvalstrom

@torvalstrom torvalstrom commented Aug 15, 2026

Copy link
Copy Markdown

The problem

SimplePlaceResolver::queryPoint() uses PostGIS functions on the PostgreSQL path:

$where = "geometry && ST_SetSRID(ST_MakePoint($lat, $lon), 4326) AND ST_Contains(geometry, ST_SetSRID(ST_MakePoint($lat, $lon), 4326))";

PostGIS is not part of a default Nextcloud database, and Memories does not ask for it — it stores memories_planet_geometry.geometry as PostgreSQL's built-in polygon type and queries it with built-in operators. So on a stock PostgreSQL install this path throws for every single point.

The catch swallows it and falls back to the per-file place map, so users mostly don't see wrong results. What it leaves behind:

  • a doomed query issued once per photo per clustering run;
  • 183,700 identical error lines in one day on the install this was found on — 99% of that server's total error volume across all namespaces — each line embedding the full SQL statement;
  • the app's own spatial lookup is dead, so results exist only for points backed by a file Memories has already indexed. queryPoint() cannot answer for an arbitrary coordinate, which is what its signature offers.

The fix

Use PostgreSQL's built-in geometric operators. No extension needed, and they are the indexable ones:

$point = sprintf('%.8F,%.8F', $lat, $lon);
$where = "POLYGON('{$point}') <@ g.geometry";

The GiST index Memories creates on that column uses the poly_ops operator class, which covers polygon-vs-polygon <@ but not point-vs-polygon — hence the degenerate single-vertex polygon built from the point. This is the same construction Memories itself uses in lib/Service/Places.php, so the semantics match the app that wrote the data.

sprintf('%.8F') rather than interpolation, so a float rendered under a locale with a decimal comma cannot produce POLYGON('55,67,12,56').

Verification

Nextcloud 34, PostgreSQL 17, 660k-row planet table, no PostGIS. Driven through the real service via Nextcloud's DI container, and called without a fileId so the per-file fallback cannot mask the result — this isolates the spatial path. Every one of these returned 0 places before:

point after places
København 9.5 ms Europe/Copenhagen, Danmark, Region Hovedstaden, Københavns Kommune
Aarhus 32.2 ms Europe/Copenhagen, Danmark, Region Midtjylland, Aarhus Kommune
Aalborg 14.0 ms Europe/Copenhagen, Danmark, Region Nordjylland, Aalborg Kommune
Odense 3.0 ms Europe/Copenhagen, Danmark, Region Syddanmark, Odense Kommune
Skagen 5.7 ms Europe/Copenhagen, Danmark, Region Nordjylland, Frederikshavn Kommune
30N 40W (open ocean) 53.9 ms Etc/GMT+3 only

The open-ocean control returns only a timezone, so the predicate is genuinely spatial rather than matching everything. Skagen correctly resolves to Frederikshavn Kommune.

EXPLAIN ANALYZE on the same table:

  • non-indexable point form: Parallel Seq Scan, 220,232 rows removed per worker, 437 ms
  • this change: Bitmap Index Scan on planet_osm_polygon_geometry_idx, 10 candidate rows, 1.7 ms

Also included

Both smaller, and the same theme — a failure that recurs per photo must not log per photo:

  • probe MySQL/MariaDB spatial support instead of assuming it, so a stripped build falls back cleanly instead of erroring per point;
  • log the resolver's DB errors once per request rather than once per point.

Happy to split those out if you'd prefer this PR to stay strictly to the PostgreSQL fix.

🤖 Generated with Claude Code

queryPoint() calls ST_MakePoint/ST_SetSRID/ST_Contains on the PostgreSQL path.
PostGIS is not part of a default Nextcloud database, and Memories does not ask
for it: it stores memories_planet_geometry.geometry as PostgreSQL's built-in
`polygon` type and queries it with built-in operators. So on a stock install
this path throws for every single point.

The failure is swallowed by the catch, which falls back to the per-file place
map Memories precomputed. That hides it from users, but it leaves three real
problems:

- a doomed query issued once per photo per clustering run;
- 183,700 identical error lines in one day on the install this was found on,
  each embedding the full SQL statement - 99% of that server's total error
  volume across all namespaces;
- the app's own spatial lookup is dead, so results exist only for points backed
  by a file Memories has already indexed. queryPoint() cannot answer for an
  arbitrary coordinate, which is what its signature offers.

Use the built-in geometric operators instead. They need no extension, and they
are the indexable ones: the GiST index Memories creates on that column uses the
`poly_ops` operator class, which covers polygon-vs-polygon `<@` but not
point-vs-polygon - hence the degenerate single-vertex polygon built from the
point. This is the same construction Memories itself uses in
lib/Service/Places.php, so the semantics match the app that wrote the data.

Verified on Nextcloud 34 / PostgreSQL 17, 660k-row planet table, no PostGIS.
Called without a fileId so the fallback cannot mask the result, which isolates
the spatial path - every one of these returned 0 places before:

  Kobenhavn   9.5 ms  Europe/Copenhagen, Danmark, Region Hovedstaden, Kobenhavns Kommune
  Aarhus     32.2 ms  Europe/Copenhagen, Danmark, Region Midtjylland, Aarhus Kommune
  Aalborg    14.0 ms  Europe/Copenhagen, Danmark, Region Nordjylland, Aalborg Kommune
  Odense      3.0 ms  Europe/Copenhagen, Danmark, Region Syddanmark, Odense Kommune
  Skagen      5.7 ms  Europe/Copenhagen, Danmark, Region Nordjylland, Frederikshavn Kommune
  Atlantic   53.9 ms  Etc/GMT+3 only

The open-ocean control returns just a timezone, so the predicate is genuinely
spatial and not matching everything. EXPLAIN ANALYZE: Bitmap Index Scan, 10
candidate rows, 1.7 ms - against a 437 ms parallel sequential scan for the
non-indexable point form.

Also in this change, both smaller and same theme - a failure that recurs per
photo must not log per photo:

- probe MySQL/MariaDB spatial support instead of assuming it, so a stripped
  build falls back cleanly rather than erroring per point;
- log the resolver's DB errors once per request instead of once per point.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MX4KbhMZnJbCEWixnCETP5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant