feat(plugin-postgresql): render PostGIS geometry/geography as EWKT (#1458)

[shubhank-saxena]

Summary

• Fixes Render PostGIS geometry/geography columns as WKT (PostgreSQL plugin) #1458: PostgreSQL geometry and geography columns now render as EWKT (SRID=4326;POINT(-73 40.7237)) in the data grid instead of raw EWKB hex. We probe pg_type for the dynamic PostGIS OIDs once at connect time, detect spatial columns in the result metadata, and convert the already-fetched EWKB-hex values to EWKT.
• The original user statement is never re-executed. After the first result is parsed, spatial values are converted with a separate, side-effect-free query that operates on the fetched values, not the user's SQL: SELECT ST_AsEWKT(t::geometry) FROM unnest($1::text[]) WITH ORDINALITY AS x(t, ord) ORDER BY ord. The hex values are passed as one bound text[] parameter, order preserved via WITH ORDINALITY; NULL stays NULL and POINT EMPTY round-trips. If the conversion query fails it logs and keeps the raw hex (no error surfaced). Because nothing re-runs the user statement, side effects can't double-apply and the parameterized path converts identically.
• Spatial columns' libpq-reported type is text after conversion, so we override columnTypeNames back to the original geometry / geography. The data grid still shows the correct type, and a future edit-back PR has the signal it needs to wrap UPDATE ... SET geom = ST_GeomFromEWKT($1) with SRID injection. No PluginKit ABI change.

Scope and structure

All changes live inside Plugins/PostgreSQLDriverPlugin/. No TableProPluginKit change, no currentPluginKitVersion bump:

• PostGISSpatialRewrite.swift holds the pure helpers (probe query, the two conversion queries, and the array-literal builder) as enum PostGISSpatialRewrite.
• PostgreSQLPluginDriver.connect() sets a core.onPostConnect hook that refreshes catalogPresence and postgisOids. LibPQDriverCore.connect() invokes the hook, and since reconnect() routes through connect(), transient reconnects re-probe and the OID map is never left stale.
• LibPQPluginConnection.fetchResults(from:) parses the result, then if the cached OID map flags any spatial columns it converts those columns' fetched hex values via a bound text[] parameter. PQclear for the original result is in defer in executeQuerySync so cancellation no longer leaks the result handle.
• Both executeQuerySync and executeParameterizedQuerySync go through the same conversion, so parameterized queries (SELECT geom FROM places WHERE id = $1) render EWKT too.

Out of scope (follow-up issues)

• geometry[] / geography[] array support, raster, box2d, box3d.
• Edit-back via ST_GeomFromEWKT with SRID injection.
• Map preview cell for geometry values.
• Streaming path (streamQuery single-row mode); only the buffered fetchResults path is wired.

Reconnect

LibPQDriverCore gained an onPostConnect hook invoked at the end of connect(). reconnect() routes through connect(), so a transient connection loss re-runs the PostGIS OID probe (and catalog-presence probe) and the cache is never left stale. This closes the gap an earlier revision documented as pre-existing.

Tests

Pure Swift Testing suite at TableProTests/Plugins/PostGISSpatialRewriteTests.swift. The helper source is mirrored into TableProTests/PluginTestSources/PostGISSpatialRewrite.swift as a symlink, matching the other plugin mirrors. Coverage:

• conversionQuery(forTypeName:): geometry / geography map to the right query, unknown type names return nil, and the queries apply ST_AsEWKT over a single bound text[] parameter (never the user statement).
• arrayLiteral(from:): single / multiple / empty inputs, order preservation, nil becomes an unquoted NULL element, all-NULL arrays, and embedded " / \ escaping.

The earlier safe-to-wrap tokenizer (and its tests) is removed: with no re-execution there's nothing to gate, so the predicate stack that guarded the second PQexec is gone.

Verified end-to-end

Tested locally against PostgreSQL 18 + PostGIS (Kartoza image, geometry and geography types). Schema used:

CREATE EXTENSION postgis;
CREATE TABLE places (id int, name text, point geometry(Point, 4326));
INSERT INTO places VALUES
  (1, 'a', ST_SetSRID(ST_MakePoint(-73, 40.7237), 4326)),
  (2, 'b', NULL),
  (3, 'empty', ST_GeomFromText('POINT EMPTY', 4326));

CREATE TABLE shapes (id int, label text, area geometry(Polygon, 4326));
INSERT INTO shapes VALUES
  (10, 'square', ST_GeomFromText('POLYGON((0 0,1 0,1 1,0 1,0 0))', 4326)),
  (11, 'nogeo',  NULL);

CREATE TABLE checkpoints (id int, location geography(Point, 4326));
INSERT INTO checkpoints VALUES (1, 'SRID=4326;POINT(2.349 48.864)');

places (geometry(Point, 4326)): point renders as EWKT, NULL stays NULL, POINT EMPTY round-trips:

shapes (geometry(Polygon, 4326)): polygons render as EWKT:

checkpoints (geography(Point, 4326)): confirms the geography type is converted too, not just geometry:

The parameterized path (item 2 from the review) was re-verified live: SELECT * FROM places WHERE id = $1 now renders EWKT, where it previously returned raw hex.

Still worth a maintainer's eye on a non-PostGIS database: opening any PostgreSQL connection without PostGIS installed should be a no-op (the probe returns empty, and the conversion path stays dormant). I didn't have a Postgres-without-PostGIS container handy to demonstrate, but the code path is the empty-map guard in fetchResults and the probe's catch block in probePostgisOids().

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: db02badf8a

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[shubhank-saxena]

I have read the CLA Document and I hereby sign the CLA.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 63524583c5

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[datlechin]

Code review: PostGIS EWKT rendering

Two automated reviews ran against this branch: a repo-wide review and a Codex diff-scoped review. Findings are merged and deduplicated below. The feature is well-scoped and well-tested; the careful SQL-aware tokenizer (handles ''/"" escapes, nested comments, dollar-quote tags, bare $1 params) and the PQclear-in-defer leak fix are good. The issues below are correctness gaps in the rewrite path plus one maintainability regression.

Must fix before merge

1. The rewrite runs the query a second time, so side effects apply twice. LibPQPluginConnection.swift:739. When a spatial result is deemed safe, executeSpatialRewrite issues a second PQexec after the first has already completed. Any side-effecting expression the blacklist does not recognize runs twice. Example: SELECT pg_advisory_unlock(1), geom FROM places passes the blacklist, and the returned value flips from true to false because the lock was released by the discarded first execution. The same second PQexec is also un-cancellable: pressing Cancel cannot interrupt it, which doubles worst-case latency on heavy spatial queries. Both problems go away with one fix: rewrite the columns in place against the first result instead of issuing a second PQexec.

2. Parameterized queries never get EWKT. LibPQPluginConnection.swift:459. A nil is passed here, disabling the spatial rewrite for all PQexecParams results, even though executeUserQuery routes parameterized queries through this path. With PostGIS installed, SELECT geom FROM places WHERE id = $1 still yields raw EWKB while the same query without parameters yields EWKT. Thread the rewrite context through the PQexecParams path.

3. The OID map is lost on reconnect. LibPQDriverCore.swift:89-90. reconnect() forwards the existing map to the current connection but never re-runs probePostgisOids(), so a fresh connection after a transient loss starts with an empty map and every PostGIS column silently falls back to raw EWKB until the driver is fully reconnected. Re-probe inside reconnect() and re-forward the map. (The PR self-describes this as a low-priority pre-existing gap, but the silent fallback makes it a correctness issue.)

4. Test mirror is committed as a copy, not a symlink. TableProTests/PluginTestSources/PostGISSpatialRewrite.swift:1. Every other mirrored plugin source in that folder is a symlink (mode 120000); this one is a regular file (100644) with byte-identical content. Both copies compile under synchronized folder groups and will silently drift on the next edit. Replace it with a symlink matching the existing pattern. The symlink name also differs from the source (PostgreSQLPluginDriver+Spatial.swift), so either confirm the test group tolerates that or rename the source (it is a standalone enum, so the +Spatial filename is misleading anyway).

Follow-ups (fine to defer)

5. ST_AsEWKT rounds coordinates to 15 digits. PostgreSQLPluginDriver+Spatial.swift:48. Display-only, so harmless here, but flag it on the edit-back follow-up so that path reads exact stored values, not the rendered EWKT, or it will corrupt coordinates on round-trip.

6. commandTag comes from the wrapper result. LibPQPluginConnection.swift:825-831. Identical to the original for a plain SELECT; could differ subtly for WITH ... SELECT. Carry the original tag through if you want exactness.

What is good

OID probing at connect, preserving the original geometry/geography type name while the wrapped column OID is text (the right signal for grid display and a future edit-back path), the thorough tokenizer and its 445-line test suite, and the PQclear defer refactor that fixes a pre-existing result-handle leak on the cancellation throw path.

[shubhank-saxena]

Thanks for the thorough review. All four must-fix items are addressed in acd04e7a / db626ede / 4f1ee845.

1 + 2 — double execution and parameterized queries (acd04e7a)

Took the structural fix you suggested: the original statement is never run a second time. After the first result is fetched, the spatial columns already hold their EWKB hex, so I convert those values with a separate, side-effect-free query that operates on the fetched values rather than the user's SQL:

SELECT ST_AsEWKT(t::geometry) FROM unnest($1::text[]) WITH ORDINALITY AS x(t, ord) ORDER BY ord

The hex values are passed as one bound text[] parameter (PQexecParams), order preserved via WITH ORDINALITY, NULLs and POINT EMPTY round-trip. Because nothing re-runs the user statement:

• No double side effects. The entire isSafeToWrap / side-effect denylist machinery (including the pg_notify/advisory-lock list from the last round) is gone — it only existed to guard the re-execution, so pg_advisory_unlock(1) and friends are no longer a concern at all.
• The conversion is a cheap value transform, so it doesn't re-pay the cost of a heavy spatial query. It's a normal query on the connection, not the un-cancellable second run of the original.
• The parameterized path now converts identically. fetchResults no longer takes an originalQuery, and both executeQuerySync and executeParameterizedQuerySync go through the same conversion, so SELECT geom FROM places WHERE id = $1 renders EWKT.

If the conversion query fails for any reason it logs and keeps the raw hex (no error surfaced).

3 — OID map lost on reconnect (db626ede)

Added an onPostConnect hook to LibPQDriverCore that connect() invokes at the end. Since reconnect() routes through connect(), transient reconnects now re-run the probes. The driver sets it to refresh both catalogPresence and postgisOids, so the catalog-presence gap you noted is closed by the same mechanism.

4 — test mirror + filename (4f1ee845)

TableProTests/PluginTestSources/PostGISSpatialRewrite.swift is now a symlink (mode 120000) matching the other mirrors, and the source is renamed to PostGISSpatialRewrite.swift so the symlink target and the enum agree. Tests now cover the value-conversion helpers (conversionQuery, arrayLiteral) instead of the removed tokenizer.

Follow-ups 5 + 6

Noted. Edit-back must read the stored geometry, not the rendered EWKT, to avoid the 15-digit rounding — I'll call that out on the edit-back issue. commandTag is no longer an issue since there's no second result; the tag is carried straight from the original query.

Re-verified end-to-end against PostgreSQL 18 + PostGIS: places (geometry point, NULL, POINT EMPTY), shapes (polygon), and checkpoints (geography) all render EWKT, and the parameterized path now converts too.

[shubhank-saxena]

Rebased onto the latest main to clear a CHANGELOG conflict (the sidebar-tree and password-source entries collided with the PostGIS line). Only the changelog needed resolving.

One thing to note: the rebase rewrote the commit hashes, so the SHAs I referenced in the review replies above point at the old pre-rebase commits. The code is all still here, just under new hashes. Nothing was dropped.