opengeospatial / geopackage

An asciidoc version of the GeoPackage specification for easier collaboration
https://www.geopackage.org
Other
264 stars 71 forks source link

RTree _update1 trigger not compatible of UPSERT capability #639

Closed rouault closed 1 year ago

rouault commented 2 years ago

I've found an issue when wanting to use the UPSERT capability on a table with a RTree. For example the following UPSERT that inserts a FID 1 or update it

INSERT INTO foo (fid, geom) VALUES (1, put_here_some_gpkg_geometry_blob) on conflict (fid) DO UPDATE SET geom=excluded.geom;

fails with an error like "UNIQUE constraint failed: foo_rtree.id (19)" when updating an existing row. I've spotted it to be caused by the "_update1" trigger that has a "INSERT OR REPLACE INTO" statement. More details in https://sqlite.org/forum/forumpost/8c8de6ff91 where the SQLite author answered that SQLite3 behave as documented here, although a bit surprising. In https://github.com/OSGeo/gdal/pull/6532, I've put a workardound where I remove the _update1 trigger and replace it with 2 separate triggers to avoid the need of a "INSERT OR REPLACE INTO" statement.

One is a _update1_old_geom_notnull trigger that does an update of the rtree when both the old and new geoms are not null/empty

pszSQL = sqlite3_mprintf(
                   "CREATE TRIGGER \"%w_update1_old_geom_notnull\" AFTER UPDATE OF \"%w\" ON \"%w\" "
                   "WHEN OLD.\"%w\" = NEW.\"%w\" AND "
                   "(NEW.\"%w\" NOTNULL AND NOT ST_IsEmpty(NEW.\"%w\") AND OLD.\"%w\" NOTNULL AND NOT ST_IsEmpty(OLD.\"%w\")) "
                   "BEGIN "
                   "UPDATE \"%w\" SET "
                   "minx = ST_MinX(NEW.\"%w\"), maxx = ST_MaxX(NEW.\"%w\"),"
                   "miny = ST_MinY(NEW.\"%w\"), maxy = ST_MaxY(NEW.\"%w\") "
                   "WHERE id = NEW.\"%w\";"
                   "END",
                   osRTreeName.c_str(), pszC, pszT,
                   pszI, pszI,
                   pszC, pszC, pszC, pszC,
                   osRTreeName.c_str(),
                   pszC, pszC,
                   pszC, pszC,
                   pszI);

and one is a _update1_old_geom_null that does an insert in the rtree when the old geom is null/empty and the new one is not:

    pszSQL = sqlite3_mprintf(
                   "CREATE TRIGGER \"%w_update1_old_geom_null\" AFTER UPDATE OF \"%w\" ON \"%w\" "
                   "WHEN OLD.\"%w\" = NEW.\"%w\" AND "
                   "(NEW.\"%w\" NOTNULL AND NOT ST_IsEmpty(NEW.\"%w\") AND (OLD.\"%w\" ISNULL OR ST_IsEmpty(OLD.\"%w\"))) "
                   "BEGIN "
                   "INSERT INTO \"%w\" VALUES ("
                   "NEW.\"%w\","
                   "ST_MinX(NEW.\"%w\"), ST_MaxX(NEW.\"%w\"),"
                   "ST_MinY(NEW.\"%w\"), ST_MaxY(NEW.\"%w\")"
                   "); "
                   "END",
                   osRTreeName.c_str(), pszC, pszT,
                   pszI, pszI,
                   pszC, pszC, pszC, pszC,
                   osRTreeName.c_str(),
                   pszI,
                   pszC, pszC,
                   pszC, pszC);

Should the spec be updated to replace the _update1 trigger by the 2 above ones ?

jyutzler commented 1 year ago

At the March 28 SWG meeting, we agreed in principle with @rouault 's statements. We also agreed to develop a new extension to indicate the presence of the revised upsert-compatible trigger. Clients that detect the presence of the extension would be free to use upserts. Clients that don't detect the trigger would have two options:

Stay tuned!