Can't upgrade from PostGIS 3.0.0alpha3 to 3.0.0alpha4 or 3.0.0alpha5dev (ST_AsGeoJSON)

[robe]

CREATE EXTENSION postgis VERSION "3.0.0alpha3";
ALTER EXTENSION postgis UPDATE;

Get error — velix mentioned this in IRC -

ERROR:  cannot change name of input parameter "pretty_print"
HINT:  Use DROP FUNCTION st_asgeojson(record,text,integer,boolean) first.

it apppears someone decided to rename the last param and did not add the necessary drop logic;

To work around the issue I had to do:

ALTER EXTENSION postgis DROP FUNCTION st_asgeojson(record,text,integer,boolean);

DROP FUNCTION st_asgeojson(record,text,integer,boolean);

Then

ALTER EXTENSION postgis UPDATE;

works. strk I thought our upgrade tests are supposed to catch these no?

[strk]

Debbie and Dronie do test upgrades. I see both are red, but didn't check the actual reported errors there. Humans can run utils/check_all_upgrades.sh 3.0.0alpha4 to check…

[pramsey]

In 17725:

Try and harmonize function parameter names for ST_AsGEoJSON(record)
References #4483

[komzpa]

Lowering to medium because r17725.

[pramsey]

Since this is a problem in the tag, is it even something we can "fix" as described? I am hoping the commit at r17725 fixes this on a "going forward" basis, is there anything else to do?

[strk]

I'm still suffering from this. Can you explain the "tag" thing, I didn't understand it. How about renaming the parameter back ? I find "pretty_print" more intuitive than "pretty_bool"

[pramsey]

"tag", "release", we bundled out a release with this change already in place, so if A was old and B was new, and A>B is broken, we can't really fix it, right? B is broken and it's released. Also, changing back for C would imply B>C would also be broken. But at least then C>D, D>E, etc would all work.

We can change back, the only thing that "pretty_bool" is good for is matching up to the json emitting functions in PostgreSQL which use "pretty_bool" as the name for their pretty printing variable. I noticed while adding the casts and decided to try to match them up and then we ended up here (because why PostgreSQL cares so much about variable names that you cannot change them in signatures? seems overly tight to me)

I still don't know what to do to fix this. I don't even know what test you're running. If you're upgrading from the broken release to trunk that won't work, right?

[strk]

Ok I see what you mean by "tag". Yes, changing back would fix A>C but break B>C and keep A>B broken. If A is just an alpha tag we can say those are not really supported and move on. Alternatively, we'll need to drop the function in the postgis_before_upgrade.sql file, so it is created anew, but that would break in case there's any view using the function.

How I'm running the test is written in the first comment, but to simplify, it is:

regress/run_test.pl -v --upgrade --upgrade-path 3.0.0alpha3--3.0.0alpha5dev

or something along those lines (note that current run_test.pl also supports ":auto" for the target version of an upgrade

[strk]

BTW, if we made sure all bots are green _before_ tagging any release, this would not happen (right, Regina?) — I guess HOWTO_RELEASE should be updated to include this checking step

[strk]

Note: I've added the item in HOWTO_RELEASE with r17755. Please NEVER release with red bots

[Algunenano]

r17725 did not fix the issue for me since the arguments provided didn't match the ones stored.

I've changed in r17797, but now I'm wondering if there might have been different signatures of ST_AsGeoJSON over time…

[pramsey]

Is this still a thing?

[robe]

It might be a different thing now.

I installed postgis-3.0.0alpha3 in my PostgreSQL 12rc1 server.

Then I installed postgis-3.0.0alpha5dev

Proceeded to upgrade my 3.0.0alpha3 to 3.0.0alpha5dev

ALTER EXTENSION postgis UPDATE TO "3.0.0alpha5dev";

and to my horror I got this message:

ERROR: could not find function "lwgeom_sortsupport" in file "C:/ming64gcc81/projects/postgresql/rel/pg12w64gcc81/lib/postgis-3.dll"

So it's hard to tell if the original problem is fixed as we seem to have a new problem.

I verified that I can do

CREATE EXTENSION postgis VERSION  "3.0.0alpha5dev";

So I'm guess the lwgeom_sortsupport thingy for whatever reason is not being included in the upgrade script

[robe]

False alarm. There most have been an old dll in memory or something.

After I restarted the PostgreSQL service and tried again it work. I was able to upgrade.

[strk]

I'm reopening as this is still an issue for me, see #4521

[Algunenano]

Please provide the exact arguments of the function as indicated in ​https://github.com/postgis/postgis/blob/548f6d1f953df7b56e69abbbdff72e33dcfdcff2/postgis/postgis_before_upgrade.sql#L26

[pramsey]

Yeah, I've used the exact arguments and not seen this behaviour. It works. The inability to move between defunct development versions seems not worth worrying about unless it can be demonstrated this actually affects moving between real releases.

[robe]

I'm downgrading this to medium. I think the blocker part of this has been dealt with.

[strk]

Raul, this is what I find in the ANY—3.0.0alpha5dev.sql file:

-- FUNCTION ST_AsGeoJson changed argument names
-- (pretty_print => pretty_bool) in 3.0alpha4
SELECT _postgis_drop_function_if_needed
        (
        '@extschema@',
        'ST_AsGeoJson',
        'r record, geom_column text, maxdecimaldigits int4, pretty_print bool'
        );

Here's the log of a "manual" session to try that portion of upgrade:

CREATE FUNCTION _postgis_drop_function_if_needed ....

strk=# begin;
BEGIN
strk=# SELECT _postgis_drop_function_if_needed
strk-#         (
strk(#         '@extschema@',
strk(#         'ST_AsGeoJson',
strk(#         'r record, geom_column text, maxdecimaldigits int4, pretty_print bool'
strk(#         );
 _postgis_drop_function_if_needed
----------------------------------

(1 row)

strk=# CREATE OR REPLACE FUNCTION ST_AsGeoJson(r record, geom_column text DEFAULT '', maxdecimaldigits int4 DEFAULT 9, pretty_bool bool DEFAULT false)
strk-#         RETURNS text
strk-#         AS '$libdir/postgis-3','ST_AsGeoJsonRow'
strk-#         LANGUAGE 'c' STABLE STRICT PARALLEL SAFE
strk-#         COST 1;
ERROR:  cannot change name of input parameter "pretty_print"
HINT:  Use DROP FUNCTION st_asgeojson(record,text,integer,boolean) first.

[pramsey]

The @extschema@ part was removed when I fixed this issue, I don't know why you're seeing it. Try the RC?

[robe]

If its still not fixed to strk's satisfaction lets push this one to 3.0.1

[robe]

Okay I just pg_upgraded my psuedo production system on Ubuntu 18.04 running PostgreSQL 12 beta (something I think 3), PostGIS 3.0alpha4 to PostgreSQL 12.0 released, PostGIS 3.0.0rc1 and I ran into two stumbling blocks. One is apt.postgresql.org bug which velix (on IRC) already warned about and another I think is our issue, but since it's a alpha4 (not really supported) probably not worth fixing.

Issue one when apt.postgresql.org backs up the old cluster files and binaries, it does not backup lib and for some reason their pg_upgradecluster goes to find it and bails when it can't find postgis-3.so in the temp folder, why it even needs it temp install is a mystery to me as I always thought it loaded in new.

These notes are mostly for posterity in case others run into the issue

Steps I did to upgrade: First our union functions evidentally changed under the hood between alpha4 and rc1, so I ran into issue something like pgis_geometry_union_transfn not found. So to prevent this in my second attempt I did this

su postgres
psql -U postgres -p 5432 -d gisdb
ALTER EXTENSION postgis DROP AGGREGATE ST_Union(geometry);
DROP AGGREGATE ST_Union(geometry);
ALTER EXTENSION postgis DROP AGGREGATE ST_Union(geometry);
DROP AGGREGATE ST_Union(geometry);
ALTER EXTENSION postgis DROP FUNCTION pgis_geometry_union_transfn(internal,geometry);
ALTER EXTENSION postgis DROP FUNCTION pgis_geometry_union_finalfn(internal);
DROP FUNCTION pgis_geometry_union_transfn(internal,geometry);
DROP FUNCTION pgis_geometry_union_finalfn(internal);
\q
exit

#next run the upgrade as root

apt update && apt upgrade

#for some reason upgrade does not copy lib files from old cluster but needs it
cp /usr/lib/postgresql/12/lib/* /var/tmp/postgresql-12-201907221/lib/
su postgres #I did this cause last time I attempted this as root it failed
pg_renamecluster 12 main main.old
pg_upgradecluster 12 main.old --rename main -m upgrade --old-bindir=/var/tmp/postgresql-12-201907221/bin

#make sure it says success before moving on
exit #back in as root
service postgresql start

su postgres
psql -U postgres -p 5432 -d gisdb

#in psql terminal
SELECT postgis_extensions_upgrade(); 
analyze (verbose);
\q

#back in shell
pg_dropcluster 12 main.old

exit
#back in shell as root

rm -rf /var/tmp/postgresql-12-201907221

[myon]

pg_upgradecluster 12 main.old --rename main -m upgrade --old-bindir=/var/tmp/postgresql-12-201907221/bin

The old bin directory is needed so it can actually read the old data directory which was using a different catalog version.

When implementing this I thought extension modules were not necessary for dumping the old catalog structure, but that's wrong. We should copy the whole bin+lib directories instead.

[robe]

I think Raul fixed this on the PostGIS side and Myon did something on the apt side.

I don't have alpha5dev lying around and don't think I had issue upgrading my 3.0.0 to 3.0.1. I'm going to close this for now and reopen after I retest upgrading 3.0.0 to 3.0.1dev if it is still an issue.

[strk]

How was this fixed ? I'm still having it:

strk=# select postgis_full_version();
POSTGIS="3.1.0dev r3.1.0alpha1-3-gfc5392de7" [EXTENSION] PGSQL="96" GEOS="3.9.0dev-CAPI-1.14.0" SFCGAL="1.3.6" PROJ="7.0.0" GDAL="GDAL 3.0.4, released 2020/01/28" LIBXML="2.9.4" LIBJSON="0.12.1" LIBPROTOBUF="1.2.1" WAGYU="0.4.3 (Internal)" (core procs from "3.0.0alpha3dev r17535" need upgrade) TOPOLOGY (topology procs from "3.0.0alpha5dev r17729" need upgrade) [UNPACKAGED!] RASTER (raster procs from "3.0.0alpha3dev r17535" need upgrade) (sfcgal procs from "3.0.0alpha3dev r17535" need upgrade)
strk=# select postgis_extensions_upgrade();
ERROR:  cannot change name of input parameter "pretty_print"
HINT:  Use DROP FUNCTION st_asgeojson(record,text,integer,boolean) first.
CONTEXT:  SQL statement "ALTER EXTENSION postgis UPDATE TO "3.1.0dev";"
PL/pgSQL function postgis_extensions_upgrade() line 59 at EXECUTE

[strk]

I'm stuck there. Tried calling the _postgis_drop_function_if_needed function but the output isn't clear (did it drop that thing?):

strk=# SELECT _postgis_drop_function_if_needed
strk-#   (
strk(#   'ST_AsGeoJson',
strk(#   'r record, geom_column text, maxdecimaldigits integer, pretty_print boolean'
strk(#   );
DEBUG:  rehashing catalog cache id 42 for pg_proc; 257 tups, 128 buckets
DEBUG:  rehashing catalog cache id 42 for pg_proc; 513 tups, 256 buckets
DEBUG:  rehashing catalog cache id 42 for pg_proc; 1025 tups, 512 buckets
DEBUG:  rehashing catalog cache id 0 for pg_aggregate; 33 tups, 16 buckets
DEBUG:  rehashing catalog cache id 42 for pg_proc; 2049 tups, 1024 buckets
 _postgis_drop_function_if_needed 
----------------------------------
 
(1 row)

[strk]

It doesn't like like the cleanup function did drop my offending function:

strk=# select proname, proargnames from pg_proc where proname = 'st_asgeojson';
   proname    |                  proargnames                  
--------------+-----------------------------------------------
 st_asgeojson | 
 st_asgeojson | {geom,maxdecimaldigits,options}
 st_asgeojson | {geog,maxdecimaldigits,options}
 st_asgeojson | {r,geom_column,maxdecimaldigits,pretty_print}
(4 rows)

[strk]

This query returns nothing:

SELECT  p.oid as oid,
        n.nspname as schema,
        n.oid as schema_oid,
        p.proname as name,
        pg_catalog.pg_get_function_arguments(p.oid) as arguments,
        pg_catalog.pg_get_function_identity_arguments(p.oid) as identity_arguments
      FROM pg_catalog.pg_proc p
      LEFT JOIN pg_catalog.pg_namespace n ON n.oid = p.pronamespace
      WHERE
        n.oid = (
          SELECT n.oid
          FROM pg_proc p
          JOIN pg_namespace n ON p.pronamespace = n.oid
          WHERE proname = 'postgis_full_version'
          ) AND
        LOWER(p.proname) = 'st_asgeojson' AND
        LOWER(pg_catalog.pg_get_function_arguments(p.oid)) ~ LOWER('r record, geom_column text, maxdecimaldigits integer, pretty_print boolean') AND
        pg_catalog.pg_function_is_visible(p.oid);

The problem is that pg_get_function_arguments includes also the default values in my case:

arguments                          | r record, geom_column text DEFAULT ''::text, maxdecimaldigits integer DEFAULT 15, pretty_print boolean DEFAULT false
pg_get_function_identity_arguments | r record, geom_column text, maxdecimaldigits integer, pretty_print boolean

But the _drop_if_needed is using the pg_catalog.pg_get_function_arguments function instead of the pg_catalog.pg_get_function_identity_arguments to find a match!

[strk]

This patch fixes it for me:

Raul: do you see anything wrong with it ?

diff --git a/postgis/postgis_before_upgrade.sql b/postgis/postgis_before_upgrade.sql
index 0a5f26696..1aacf9168 100644
--- a/postgis/postgis_before_upgrade.sql
+++ b/postgis/postgis_before_upgrade.sql
@@ -57,7 +57,7 @@ BEGIN
                                        WHERE proname = 'postgis_full_version'
                                        ) AND
                                LOWER(p.proname) = LOWER(function_name) AND
-                               LOWER(pg_catalog.pg_get_function_arguments(p.oid)) ~ LOWER(function_arguments) AND
+                               LOWER(pg_catalog.pg_get_function_identity_arguments(p.oid)) ~ LOWER(function_arguments) AND
                                pg_catalog.pg_function_is_visible(p.oid)
                        ORDER BY 1, 2, 4
        LOOP

[strk]

I filed PR ​https://git.osgeo.org/gitea/postgis/postgis/pulls/44 with the proposed change. I'd feel better if we added the offending source version in our upgrade test, but that requires rebuilding the docker image, which I'm not in a comfortable position to do at the moment.

[Algunenano]

Raul: do you see anything wrong with it ?

Yes, we use pg_get_function_arguments because we do want the default values in the signature, so we drop the function when the defaults change and not always.

I see that the signature to drop was changed in ​https://trac.osgeo.org/postgis/changeset/17879, probably because there was more than one issue with the changes and multiple drops where needed and we have switched back and forth.

The solution is to add a new line readding what was removed in r17879

[strk]

There are many versions of that function that I see in git log:

Removed by commit [aafcb2e8ef797be6537f160dc321396a6fb5c8c9/git]:

ST_AsGeoJson(geom geometry, maxdecimaldigits int4 DEFAULT 15, options int4 DEFAULT 0)

Removed by commit [2034809342c06295428fef80616cfa02d46126eb/git]:

r record, geom_column text DEFAULT , maxdecimaldigits int4 DEFAULT 15, pretty_print bool DEFAULT false

Removed by commit [ce70e49067618cdfb7224ce5de2a69d56e6130ae/git]:

_ST_AsGeoJson(int4, geometry, int4, int4) ST_AsGeoJson(gj_version int4, geog geography, maxdecimaldigits int4 DEFAULT 15, options int4 DEFAULT 0)

Both are currently removed in postgis_after_upgrade.sql

If the above is correct then the commit in r17879 should just be reverted as there's never been a default-less version taking a record as first argument ? Also we should probably add a drop for the very first one or it will be kept around.

Also I noticed we have a couple of the removed functions in legacy.sql, does it make sense to have things both in postgis_after/before_upgrade and in legacy.sql ?

[strk]

I confirm that reverting r17879 goes beyond the ST_AsGeoJSON problem (while hitting another, for which another ticket will be filed) - ok to just revert ? I updated my PR with the revert

[Algunenano]

If the above is correct then the commit in r17879 should just be reverted as there's never been a default-less version taking a record as first argument ?

Yes, I think so too.

If the above is correct then the commit in r17879 should just be reverted as there's never been a default-less version taking a record as first argument ?

Also yes, we should have a drop in place for all signatures that have disappeared.

Also I noticed we have a couple of the removed functions in legacy.sql, does it make sense to have things both in postgis_after/before_upgrade and in legacy.sql ?

It seems like the original intention of legacy.sql was to facilitate the upgrade to Postgis 2, and I'm not sure if we keep it around for anything else. If the only use for the file is for 1.x → 2.x upgrade, I think it's time to drop it and ask people to update to intermediate releases (1.x → 2.x/3.0, 2.x → 3.1).

[Sandro Santilli <strk@…>]

In 1bbc4cc/git:

Revert "Try and be a little broader in dropping temporary old signature"

This reverts commit 548f6d1f953df7b56e69abbbdff72e33dcfdcff2.

Fixes #4483 and #4521 as far as I can tell

[Sandro Santilli <strk@…>]

In 98e3e25/git:

Revert "Try and be a little broader in dropping temporary old signature"

This reverts commit 548f6d1f953df7b56e69abbbdff72e33dcfdcff2.

Fixes #4483 and #4521 as far as I can tell, in the stable-3.0 branch