Opened 7 months ago

Closed 7 months ago

Last modified 6 months ago

#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 robe, 7 months ago

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).

comment:2 by strk, 7 months 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 strk, 7 months 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 robe, 7 months ago

Summary: Note Breaking change in PostGIS 3.1 with introduction of gridSizeBreaking change in PostGIS 3.1 with introduction of gridSize

comment:5 by strk, 7 months 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 strk, 7 months 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 strk, 7 months 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 strk, 7 months 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 strk, 7 months 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 strk, 7 months 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 strk, 7 months 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:12 by Sandro Santilli <strk@…>, 7 months ago

In a99a00a/git:

Allow upgrades in presence of views using deprecated functions

Support adding a 'Replaces' comment above functions to improve upgrade

When a function has a line like the following in in the comments
above its signature:

Replaces ST_Intersection(geometry, geometry) deprecated in 3.1.0

The upgrade script will try to drop the deprecated functions and in case
of failure leave them behind renamed with a _deprecated_by_postgis_<version>
suffix, warns the user about such occurrence and gives her hints on
how to fix.

The postgis_full_version() will report presence of such deprecated
functions as an "incomplete upgrade".

References #5033 in master/main branch (3.3.0dev)

Includes regression tests

comment:13 by strk, 7 months ago

Merged to main/master branch (3.3.0dev) — to be backported down till 3.1 stable

comment:14 by Sandro Santilli <strk@…>, 7 months ago

In 82d84900/git:

Allow upgrades in presence of views using deprecated functions

Support adding a 'Replaces' comment above functions to improve upgrade

When a function has a line like the following in in the comments
above its signature:

Replaces ST_Intersection(geometry, geometry) deprecated in 3.1.0

The upgrade script will try to drop the deprecated functions and in case
of failure leave them behind renamed with a _deprecated_by_postgis_<version>
suffix, warns the user about such occurrence and gives her hints on
how to fix.

The postgis_full_version() will report presence of such deprecated
functions as an "incomplete upgrade".

References #5033 in 3.2 branch (3.2.1dev)

Includes regression tests

comment:15 by Sandro Santilli <strk@…>, 7 months ago

Resolution: fixed
Status: newclosed

In 3d94744/git:

Allow upgrades in presence of views using deprecated functions

Support adding a 'Replaces' comment above functions to improve upgrade

When a function has a line like the following in in the comments
above its signature:

Replaces ST_Intersection(geometry, geometry) deprecated in 3.1.0

The upgrade script will try to drop the deprecated functions and in case
of failure leave them behind renamed with a _deprecated_by_postgis_<version>
suffix, warns the user about such occurrence and gives her hints on
how to fix.

The postgis_full_version() will report presence of such deprecated
functions as an "incomplete upgrade".

Closes #5033 in 3.1 branch (3.1.5dev)

comment:16 by Sandro Santilli <strk@…>, 6 months ago

In 43f92d9/git:

Do not rewrite rules on upgrade (too dangerous)

References #5033 in master/main branch (3.3.0dev)
Drop deprecated function after updgrade in CI

comment:17 by Sandro Santilli <strk@…>, 6 months ago

In 7020472/git:

Do not rewrite rules on upgrade (too dangerous)

References #5033 in master/main branch (3.3.0dev)
Drop deprecated function after updgrade in CI

comment:18 by Sandro Santilli <strk@…>, 6 months ago

In 3cd3b5e/git:

Do not rewrite rules on upgrade (too dangerous)

References #5033 in stable-3.1 branch (3.1.5dev)
Drop deprecated function after updgrade in CI

comment:19 by Sandro Santilli <strk@…>, 6 months ago

In 832131a/git:

Rewrite deprecated function in SQL, to avoid referencing old libraries

References #5033 in master/main branch (3.3.0dev)

NOTE: still drop the function from CI after upgrade because otherwise

they will depend on PostGIS types (for arguments) and thus prevent
dropping postgis extension

Note: See TracTickets for help on using tickets.