Upgrade from 3.2.1 fails due to deprecating two flavors of st_dwithin

[strk]

Going from 3.2.1 to 3.4.0 the upgrade script attempts to deprecate two versions of st_dwithin, resulting in the second deprecation failing because the name of the target renamed function is already taken.

# regress/run_test.pl \
  --after-upgrade-script regress/hooks/hook-after-upgrade.sql \
  --after-upgrade-script raster/test/regress/hooks/hook-after-upgrade-raster.sql \
  --before-upgrade-script raster/test/regress/hooks/hook-before-upgrade-raster.sql \
  --before-upgrade-script regress/hooks/hook-before-upgrade.sql \
  --raster -v --extension \
  --upgrade-path 3.2.1--3.3.3 \
  raster/test/regress/rt_count

WARNING:  Deprecated function st_dwithin_deprecated_by_postgis_300(text,text,double precision) left behind: cannot drop function st_dwithin_deprecated_by_postgis_300(text,text,double precision) because other objects depend on it
DETAIL:  view upgrade_view_test_dwithin depends on function st_dwithin_deprecated_by_postgis_300(text,text,double precision)
HINT:  Replace the view changing all occurrences of st_dwithin_deprecated_by_postgis_300(text,text,double precision) in its definition with st_dwithin and upgrade again
ERROR:  Attempting to rename replaced function st_dwithin(text, text, float8) got function st_dwithin_deprecated_by_postgis_300(text, text, double precision) already exists in schema "public" (42723)

The same error occurs upgrading to 3.4.0

[strk]

The target function name is indeed the same no matter different argument names:

# grep 'RENAME TO st_dwithin_deprecated_by' postgis/postgis_upgrade.sql
ALTER FUNCTION st_dwithin( text, text, float8 ) RENAME TO st_dwithin_deprecated_by_postgis_300;
ALTER FUNCTION st_dwithin( geography, geography, float8 ) RENAME TO st_dwithin_deprecated_by_postgis_300;

Judging by the name of the target function this bug might be affecting 3.0.0 as well, it should be verified. I don't know why CI isn't catching these.

[strk]

The second signature seem to have been added with [ec98bd63e8f255cbdbf6baaed850e6c8b8d57af1/git] referencing ticket #5035

[strk]

The two Replaces comments are here:

postgis/geography.sql.in:770:— Replaces ST_DWithin(geography, geography, float8) deprecated in 3.0.0 postgis/postgis.sql.in:4639:— Replaces ST_DWithin(text, text, float8) deprecated in 3.0.0

It seems to me that the fix would be to include the argument types in the name of the deprecated functions, or something like that

[strk]

Oddly Dronie has no trouble upgrading from 3.2.2 to 3.5.0dev: ​https://dronie.osgeo.org/postgis/postgis/4824/1/3

But I can't tell the same on my local system:

Upgrade path: 3.2.2 --> 3.5.0dev
Database postgis_reg already exists, dropping.
Creating database 'postgis_reg' 
Preparing db 'postgis_reg' using: CREATE EXTENSION postgis VERSION '3.2.2' SCHEMA public
Preparing db 'postgis_reg' using: CREATE EXTENSION postgis_raster VERSION '3.2.2' SCHEMA public
Executor slow factor: 6.6738 (66.738 ms to fetch full version)
Upgrading from postgis 3.2.2
Running before-upgrade-script raster/test/regress/hooks/hook-before-upgrade-raster.sql
Running before-upgrade-script regress/hooks/hook-before-upgrade.sql
Upgrading PostGIS in 'postgis_reg' using: ALTER EXTENSION postgis UPDATE TO '3.5.0dev'
 failed (Error encountered updating EXTENSION POSTGIS: /tmp/pgis_reg/regress_log)
-----------------------------------------------------------------------------
NOTICE:  schema "public" already exists, skipping
CREATE SCHEMA
CREATE EXTENSION
CREATE EXTENSION
NOTICE:  schema "public" already exists, skipping
psql:raster/test/regress/hooks/hook-before-upgrade-raster.sql:16: NOTICE:  Adding SRID constraint
psql:raster/test/regress/hooks/hook-before-upgrade-raster.sql:16: NOTICE:  Adding scale-X constraint
psql:raster/test/regress/hooks/hook-before-upgrade-raster.sql:16: NOTICE:  Adding scale-Y constraint
psql:raster/test/regress/hooks/hook-before-upgrade-raster.sql:16: NOTICE:  Adding blocksize-X constraint
psql:raster/test/regress/hooks/hook-before-upgrade-raster.sql:16: NOTICE:  Adding blocksize-Y constraint
psql:raster/test/regress/hooks/hook-before-upgrade-raster.sql:16: NOTICE:  Adding alignment constraint
psql:raster/test/regress/hooks/hook-before-upgrade-raster.sql:16: NOTICE:  Adding number of bands constraint
psql:raster/test/regress/hooks/hook-before-upgrade-raster.sql:16: NOTICE:  Adding pixel type constraint
psql:raster/test/regress/hooks/hook-before-upgrade-raster.sql:16: NOTICE:  Adding nodata value constraint
psql:raster/test/regress/hooks/hook-before-upgrade-raster.sql:16: NOTICE:  Adding out-of-database constraint
psql:raster/test/regress/hooks/hook-before-upgrade-raster.sql:16: NOTICE:  Adding maximum extent constraint
 addrasterconstraints 
----------------------
 t
(1 row)

psql:raster/test/regress/hooks/hook-before-upgrade-raster.sql:33: NOTICE:  Adding coverage tile constraint required for regular blocking
psql:raster/test/regress/hooks/hook-before-upgrade-raster.sql:33: NOTICE:  Adding spatially unique constraint required for regular blocking
 addrasterconstraints 
----------------------
 t
(1 row)

NOTICE:  schema "public" already exists, skipping
WARNING:  Deprecated function st_dwithin_deprecated_by_postgis_300(text,text,double precision) left behind: cannot drop function st_dwithin_deprecated_by_postgis_300(text,text,double precision) because other objects depend on it
DETAIL:  view upgrade_view_test_dwithin depends on function st_dwithin_deprecated_by_postgis_300(text,text,double precision)
HINT:  Replace the view changing all occurrences of st_dwithin_deprecated_by_postgis_300(text,text,double precision) in its definition with st_dwithin and upgrade again
ERROR:  Attempting to rename replaced function st_dwithin(text, text, float8) got function st_dwithin_deprecated_by_postgis_300(text, text, double precision) already exists in schema "public" (42723)

[strk]

The difference between CI and my local system is in the hooks being passed.

The working one (run by CI):

  --before-upgrade-script   regress/hooks/hook-before-upgrade.sql \
  --after-upgrade-script    regress/hooks/hook-after-upgrade.sql \

The failing one:

  --before-upgrade-script raster/test/regress/hooks/hook-before-upgrade-raster.sql \
  --before-upgrade-script regress/hooks/hook-before-upgrade.sql \
  --after-upgrade-script regress/hooks/hook-after-upgrade.sql \

Puzzling but for some reason running the raster before-upgrade test hook somehow affects the st_dwithin upgradeability.

[strk]

Forget previous comment, that difference was not the real difference, I'm still digging

[strk]

I get the same failure going from 3.2.1 to 3.2.6dev

The fix might be just including the OID of the deprecated function in the name, should work fine. I'm going to test this in a PR

[strk]

Commit in ​https://git.osgeo.org/gitea/postgis/postgis/pulls/139 fixes it for me. What I'd like though is to see the CI fail before the fix, which is currently not happening.

[strk]

Ok there must have been something bogus with my system because PostGIS 3.2.1 does not seem to have st_dwithin(geography, geography, double precision)

According to the Replaces comment this bug should only be exposed when upgrading from a version earlier than 3.0.0

[strk]

The oldest version from which Dronie tests upgrades is 3.0.6 which explains why CI is green. In turn, the reason why Dronie only tests 3.0.6 is, I believe, that earlier versions do not support PostgreSQL-13 which is the oldest Dronie is testing.

We'd need a test for PostgresSQL-12 to get this bug visible in Dronie.

Regina any reason for not re-introducing PostgreSQL-12 testing there ?

[strk]

PostgreSQL-12 testing was moved from Dronie to Woodie with commit [56d525e779fa814893a2a3cf772c1980a270b0d9/git]

Woodie PostgreSQL-12 testing seem to pass upgrades from 2.5.7: ​https://woodie.osgeo.org/repos/30/pipeline/854/9#L9192

PASS: postgis extension upgrade 2.5.7--3.5.0dev!

[strk]

Running in the trisquel docker container used by Woodie:

postgis_reg=# create extension postgis version '2.5.7';
CREATE EXTENSION
postgis_reg=# select oid::regprocedure from pg_proc where proname = 'st_dwithin';
                            oid                             
------------------------------------------------------------
 st_dwithin(text,text,double precision)
 st_dwithin(geometry,geometry,double precision)
 st_dwithin(geography,geography,double precision)
 st_dwithin(raster,raster,double precision)
 st_dwithin(geography,geography,double precision,boolean)
 st_dwithin(raster,integer,raster,integer,double precision)
(6 rows)
postgis_reg=# create view v1 as select st_dwithin(null::text, null::text, null::float8);
CREATE VIEW
postgis_reg=# create view v2 as select st_dwithin(null::geography, null::geography, null::float8);
CREATE VIEW
postgis_reg=# alter extension postgis update to '3.3.2';
WARNING:  unpackaging raster
WARNING:  PostGIS Raster functionality has been unpackaged
HINT:  type `SELECT postgis_extensions_upgrade();` to finish the upgrade. After upgrading, if you want to drop raster, run: DROP EXTENSION postgis_raster;
WARNING:  Deprecated function st_dwithin_deprecated_by_postgis_300(text,text,double precision) left behind: cannot drop function st_dwithin_deprecated_by_postgis_300(text,text,double precision) because other objects depend on it
DETAIL:  view v1 depends on function st_dwithin_deprecated_by_postgis_300(text,text,double precision)
HINT:  Replace the view changing all occurrences of st_dwithin_deprecated_by_postgis_300(text,text,double precision) in its definition with st_dwithin and upgrade again
WARNING:  Deprecated function st_dwithin_deprecated_by_postgis_300(geography,geography,double precision) left behind: cannot drop function st_dwithin_deprecated_by_postgis_300(geography,geography,double precision) because other objects depend on it
DETAIL:  view v2 depends on function st_dwithin_deprecated_by_postgis_300(geography,geography,double precision)
HINT:  Replace the view changing all occurrences of st_dwithin_deprecated_by_postgis_300(geography,geography,double precision) in its definition with st_dwithin and upgrade again
ALTER EXTENSION

Interestingly there seem to be no problem with having two deprectated function with the same name but different type arguments. So my problem must have been really due to something else, and specifically to a broken system whereas to go from version A to version B in presence of a view we're hitting two upgrades instead of one, which try to deprecate the same function twice, resulting in an attempt to run the SAME alter more than once.

I guess I'll mark this ticket as invalid or maybe it can become a ticket about idempotency of an upgrade step. In other words: should we allow running postgis_extensions_upgrade() twice in the same database where a view is defined using deprecated functions ? Right now doing so will error out the second time, with this hard-to-understand error.

We could maybe just improve the error to hint about the possibility that upgrade issues need be fixed before further upgrading

[strk]

I'll close this as invalid, too noisy to be actable.

[strk]

A more clear report about what's going on is in #5494