#5033 closed defect (fixed)
Breaking change in PostGIS 3.1 with introduction of gridSize
| Reported by: | robe | Owned by: | pramsey |
|---|---|---|---|
| Priority: | medium | Milestone: | PostGIS 3.1.5 |
| Component: | postgis | Version: | 3.1.x |
| Keywords: | Cc: |
Description
I got a complaint recently from someone that they can't upgrade to PostGIS 3.1 from prior versions.
Issue is they have Materialized views that rely on:
ST_Intersection(geometry, geometry)
and our upgrade wants to drop that function and add
ST_Intersection(geometry, geometry, gridSize default -1)
I'm ticketing this just in case we can do something about it in the future.
My solution to them would involve hacking their extension script so I could put back ST_Intersection(geometry, geometry) and make the ST_Intersection(geometry, geometry, gridSize (not a default).
Unfortunately I can't hack their extension script cause they are on Amazon RDS so extension scripts are off limits.
Martin asked why can't we fix it now, and reason is 4 microversions have been released of 3.1 since. Anyone who was bitten by this probably sucked it up dropped their views and recreated them. Fixing this would then break things in a microversion which is a huge no no.
I think the most we can do about this is put a warning in next 3.1.5 release notes about this breaking news that between 3.1/3.0
Sadly of all the things we noted as breaking, we neglected to mention this one
https://git.osgeo.org/gitea/postgis/postgis/src/branch/stable-3.1/NEWS#L87
This affects all functions where we added a gridSize from new things:
ST_Difference - Enhanced: 3.1.0 accept a gridSize parameter - requires GEOS >= 3.9.0 Returns a geometry representing the part of geometry A that does not intersect geometry B. ST_Intersection - Enhanced: 3.1.0 accept a gridSize parameter - requires GEOS >= 3.9.0 Returns a geometry representing the shared portion of geometries A and B. ST_Subdivide - Enhanced: 3.1.0 accept a gridSize parameter, requires GEOS >= 3.9.0 to use this new feature. Computes a rectilinear subdivision of a geometry. ST_SymDifference - Enhanced: 3.1.0 accept a gridSize parameter - requires GEOS >= 3.9.0 Returns a geometry representing the portions of geometries A and B that do not intersect. ST_UnaryUnion - Enhanced: 3.1.0 accept a gridSize parameter - requires GEOS >= 3.9.0 Computes the union of the components of a single geometry. ST_Union - Enhanced: 3.1.0 accept a gridSize parameter - requires GEOS >= 3.9.0 Returns a geometry representing the point-set union of the input geometries.
I suspect most impacted will be ST_Intersection, ST_UnaryUnion, and ST_Union
Change History (19)
comment:1 by , 2 years ago
comment:2 by , 2 years ago
My PR contains a test which shows the failure on upgrade: https://dronie.osgeo.org/postgis/postgis/2276
This is the PR: https://git.osgeo.org/gitea/postgis/postgis/pulls/66
One possible fix would be altering views to use the new function
comment:3 by , 2 years ago
You can reproduce the bug with something like the following:
regress/run_test.pl \ --before-upgrade-script regress/hooks/hook-before-upgrade.sql \ --extension --upgrade-path 3.0.5dev--3.2.0dev \ regress/core/tickets
comment:4 by , 2 years ago
| Summary: | Note Breaking change in PostGIS 3.1 with introduction of gridSize → Breaking change in PostGIS 3.1 with introduction of gridSize |
|---|
comment:5 by , 2 years ago
I have a fix for the 3.0 to 3.2 upgrade, but breaks upgrades from 3.1 due to dropping DEFAULT from arguments being forbidden
comment:6 by , 2 years ago
I found a better fix, all based on comments-above-functions in the SQL file, like:
-- Replaced: ST_intersection(geometry,geometry) in 3.1.0
The new handling is all automatic (no need to mess with after_upgrade, before_upgrade sql scripts) and deals with views. At the moment it will still FAIL if the old signature still cannot be dropped after REPLACING all views using it, but we could decide to act in a different way and allow databases to be in a "still unfinished upgrade" state, reporting such state and hints about how to resolve it from postgis_full_version() .
comment:7 by , 2 years ago
One problem with the current approach is that the create or replace view currently performed is NOT retaining all view option (ie: column names, check options).
If a view has custom columns, for example, the create or replace will fail. In case of local check option, it will be silently changed to cascaded check option.
I'm thinking it could be easier to just NOT attempt to redefine views but rather just warn the user about leaving these "legacy" function signatures around (both on upgrade, via WARNING and from postgis_full_version)
comment:8 by , 2 years ago
Alright I went ahead and removed the attempt to automatically adapt views. In its current state the PR just renames the deprecated function signatures and *ATTEMPTS* to drop them, raising a WARNING (not visible during extension upgrade) to the user. The postgis_full_version() will report the incomplete upgrpade and postgis_extensions_upgrade will give wannings and hints about how to resolve them when those attempt to drop the function and raise warnings with good hints about how to resolve them for complete success).
I think I'm satisfied enough to call this closed. Attempts to automatically adapt the code can be left as a possible future enhancement but doesn't need to be done urgently.
comment:9 by , 2 years ago
There's still a pending issue: renamed functions (not drop) are left pointing to possibly unexisting library:
# regress/run_test.pl --before-upgrade-script regress/hooks/hook-before-upgrade.sql --extension --upgrade-path ${F}--3.2.0dev! regress/core/regress -v --nodrop
Creating database 'postgis_reg'
Preparing db 'postgis_reg' using: CREATE EXTENSION postgis VERSION '2.5.6dev' SCHEMA public
Upgrading from postgis 2.5.6dev
Running before-upgrade-script regress/hooks/hook-before-upgrade.sql
WARNING: postgis_extensions_upgrade() not available or functional in version 2.5.6dev. We'll use manual upgrade.
Upgrading PostGIS in 'postgis_reg' using: ALTER EXTENSION postgis UPDATE TO '3.2.0dev'
Packaging PostGIS Raster in 'postgis_reg' for later drop using: ALTER EXTENSION postgis UPDATE TO '3.2.0dev'
Dropping PostGIS Raster in 'postgis_reg' using: CREATE EXTENSION postgis_raster VERSION '3.2.0dev' FROM unpackaged;
PostgreSQL 12.7 (Ubuntu 12.7-0ubuntu0.20.10.1) on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu 10.2.0-13ubuntu1) 10.2.0, 64-bit
Postgis 3.2.0dev - (3.2.0rc1-11-g211893b3a) - 2021-10-18 22:13:19
scripts 3.2.0dev 3.2.0rc1-11-g211893b3a
GEOS: 3.11.0dev-CAPI-1.16.0
PROJ: 7.1.0
Running tests
regress/core/regress .. failed (diff expected obtained: /tmp/pgis_reg/test_1_diff)
-----------------------------------------------------------------------------
--- regress/core/regress_expected 2021-07-01 12:48:31.374498018 +0200
+++ /tmp/pgis_reg/test_1_out 2021-12-16 19:16:10.970197463 +0100
@@ -231,3 +231,8 @@
314|676
315|0
316|t|LINESTRING(40 8.660254037844387,35 8.660254037844387)
+unexpected probin|st_difference_deprecated_by_postgis_301:$libdir/postgis-2.5
+unexpected probin|st_intersection_deprecated_by_postgis_301:$libdir/postgis-2.5
+unexpected probin|st_subdivide_deprecated_by_postgis_301:$libdir/postgis-2.5
+unexpected probin|st_symdifference_deprecated_by_postgis_301:$libdir/postgis-2.5
+unexpected probin|st_unaryunion_deprecated_by_postgis_301:$libdir/postgis-2.5
-----------------------------------------------------------------------------
This may be a problem in production as the users's views might be just disfunctioning unless the old ones point to the same major version as the new ones (and we didn't change function name in the library).
For the testcase it could be only fixed by adapting those views (either automatically or by the test itself, maybe upgrading against after having dropped the views)
comment:10 by , 2 years ago
I ended up copying the code in both the _upgrade.sql scripts AND the postgis_extensions_upgrade() function. The latter is really ONLY useful to give users some feedback, because there seem to be NO WAY to get any message out from ALTER EXTENSION UPGRADE (extension-less users are receiving all messages)
comment:11 by , 2 years ago
Clean up phase: now the handling is ALL in the upgrade script, I was wrong about being unable to get messages from the ALTER EXTENSION UPGRADE step, I do get them, so there's no duplication and both extension-less and extension-full approaches are covered
comment:13 by , 2 years ago
Merged to main/master branch (3.3.0dev) — to be backported down till 3.1 stable
Forgot to add, strk has a pull request to at least catch this kind of stuff in future in our upgrade script. For very popular functions, we should not drop old signatures (or at least consider cautiously the impact). We should instead add new ones to augment them.
Things that require drop:
Add a new function that introduces default args that would conflict with existing that has no default args, (such as the gridSize case)
Changing names of arguments in functions (we've done this before and it broke some stuff, I forget which function that was).