Can't upgrade with a view based on ST_AsMVTGeom

[Algunenano]

cartodb_dev_user_3e4a6fc6-4137-4c59-bc63-066f80efb90e_db=# alter extension postgis update TO "3.0.0devnext";
ERROR:  cannot drop function st_asmvtgeom(geometry,box2d,integer,integer,boolean) because other objects depend on it
DETAIL:  view mvt depends on function st_asmvtgeom(geometry,box2d,integer,integer,boolean)
view "Building" depends on function st_asmvtgeom(geometry,box2d,integer,integer,boolean)
view aaaaaaaaaaa depends on function st_asmvtgeom(geometry,box2d,integer,integer,boolean)
view aaaaa depends on function st_asmvtgeom(geometry,box2d,integer,integer,boolean)
HINT:  Use DROP ... CASCADE to drop the dependent objects too.

postgis_upgrade.sql does this:

DROP FUNCTION IF EXISTS ST_AsMVTGeom(geom geometry, bounds box2d, extent int4, buffer int4, clip_geom bool);

[...]

CREATE OR REPLACE FUNCTION ST_AsMVTGeom(geom geometry, bounds box2d, extent int4 default 4096, buffer int4 default 256, clip_geom bool default true)
	RETURNS geometry
	AS '$libdir/postgis-3','ST_AsMVTGeom'
	LANGUAGE 'c' IMMUTABLE  PARALLEL SAFE;

Since the 2 signatures are the same, it's trying to drop the current ST_AsMVTGeom before doing the CREATE OR REPLACE, so the upgrade fails.

I'm planning on checking the rest of the functions in postgis_drop_before.sql to confirm they aren't covering already existing functions like it's happening in ST_AsMVTGeom.

I guess 2.4+ is affected by this.

[Algunenano]

This is reproducible with the following elements:

• AddGeometryColumn:

  # CREATE VIEW break_add_geom AS Select AddGeometryColumn(NULL, NULL, NULL, NULL, 0, NULL, 0, true);
  CREATE VIEW
  # alter extension postgis update TO "3.0.0devnext";
  ERROR:  cannot drop function addgeometrycolumn(character varying,character varying,character varying,character varying,integer,character varying,integer,boolean) because other objects depend on it
  DETAIL:  view break_add_geom depends on function addgeometrycolumn(character varying,character varying,character varying,character varying,integer,character varying,integer,boolean)
  HINT:  Use DROP ... CASCADE to drop the dependent objects too.
  

• st_asx3d:

  # CREATE VIEW break_asx3d AS Select st_asx3d(NULL, 0, 0);
  CREATE VIEW
  # alter extension postgis update TO "3.0.0devnext";
  ERROR:  cannot drop function st_asx3d(geometry,integer,integer) because other objects depend on it
  DETAIL:  view break_asx3d depends on function st_asx3d(geometry,integer,integer)
  HINT:  Use DROP ... CASCADE to drop the dependent objects too.
  

• UpdateGeometrySRID:

  # CREATE VIEW break_updategeomtry AS SELECT UpdateGeometrySRID(NULL, NULL, NULL, NULL, 0);
  CREATE VIEW
  # alter extension postgis update TO "3.0.0devnext";
  ERROR:  cannot drop function updategeometrysrid(character varying,character varying,character varying,character varying,integer) because other objects depend on it
  DETAIL:  view break_updategeomtry depends on function updategeometrysrid(character varying,character varying,character varying,character varying,integer)
  HINT:  Use DROP ... CASCADE to drop the dependent objects too
  

• ST_AsLatLonText:

  # CREATE VIEW break_aslatlon AS SELECT ST_AsLatLonText(the_geom, NULL) FROM all_month;
  CREATE VIEW
  # alter extension postgis update TO "3.0.0devnext";
  ERROR:  cannot drop function st_aslatlontext(geometry,text) because other objects depend on it
  DETAIL:  view break_mvt depends on function st_aslatlontext(geometry,text)
  HINT:  Use DROP ... CASCADE to drop the dependent objects too.
  

• ST_AsMVTGeom:

  # CREATE VIEW break_mvt AS SELECT ST_AsMVTGeom(the_geom, NULL) FROM all_month;
  CREATE VIEW
  # alter extension postgis update TO "3.0.0devnext";
  ERROR:  cannot drop function st_asmvtgeom(geometry,box2d,integer,integer,boolean) because other objects depend on it
  DETAIL:  view break_mvt depends on function st_asmvtgeom(geometry,box2d,integer,integer,boolean)
  HINT:  Use DROP ... CASCADE to drop the dependent objects too.
  

• geometry_columns:

  # CREATE VIEW break_geometry_column AS SELECT * FROM geometry_columns;
  CREATE VIEW
  # alter extension postgis update TO "3.0.0devnext";
  ERROR:  cannot drop view geometry_columns because other objects depend on it
  DETAIL:  view break_geometry_column depends on view geometry_columns
  HINT:  Use DROP ... CASCADE to drop the dependent objects too
  

These are dummy examples but they show that these drops can break upgrades. They mostly come from renaming parameters, so we'll need to either only drop these functions if they have the old parameters or make sure that the upgrade is possible when a parameter is renamed.

[strk]

This is a known issue with functions that change signature over time. Whenever a function changes signature in a significant way our upgrade process _drops_ the old function because CREATE OR REPLACE cannot do everything (like changing parameter names or types or introducing optional parameters).

To reduce this problem it is HIGHLY ADVISABLE that we _always_ keep around a function with the old signature, being just a wrapper to the old. I've tried to do this whenever possible, but not all devs have taken this into account, saying that some functions should really just not be relied upon for views and the such. Enforcing such behavior is NOT EASY at all, as it would imply having our testsuite create a view using each and every existing function right before an upgrade (does actually sound like a plan, if you want to start this).

An alternative "solution" to the problem would be to temporarely drop views relying on some functions and re-create them afterwards. This would of course ONLY WORK if the new function has a compatible signature with the old one (ie: not when dropping parameters).

I hope this was a good starting point for you to fix this long-standing robustness issue

[Algunenano]

@strk Thanks a lot for the comments.

I think deleting and recreating views automagically in the extension is excessive (since then other stuff might depend on that view and the cycle goes on); as you say, we should try to keep around the old signature, but when that's impossible or I'd be fine forcing a manual user intervention once.

I've come up with a sql function that I think could handle this.For example, let's say we have a function with this signature:

CREATE OR REPLACE FUNCTION ST_AsMVTGeom(geom geometry, bounds box2d, extent int4, buffer int4, clip_geom bool)

And we added defaults:

CREATE OR REPLACE FUNCTION ST_AsMVTGeom(geom geometry, bounds box2d, extent int4 default 4096, buffer int4 default 256, clip_geom bool default true)

Adding defaults (same for renaming a parameter) means you can't CREATE OR REPLACE as you've already mentioned, and this means that manual intervention is forced; but using the following SQL function we could require the intervention only once and not with every upgrade:

CREATE OR REPLACE FUNCTION postgis_cond_drop_function(
    function_schema text,
    function_name text,
    function_arguments text) RETURNS void AS $$
DECLARE
    frec RECORD;
    sql_drop text;
BEGIN
    FOR frec IN
        SELECT  p.oid as oid,
                n.nspname as schema,
                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.nspname ~* function_schema AND
                p.proname ~* function_name AND
                LOWER(pg_catalog.pg_get_function_arguments(p.oid)) ~ LOWER(function_arguments)
            AND pg_catalog.pg_function_is_visible(p.oid)
            ORDER BY 1, 2, 4
    LOOP
        sql_drop := 'DROP FUNCTION ' || quote_ident(frec.schema) || '.' || quote_ident(frec.name) || ' ( ' || frec.identity_arguments || ' ) ';
        RAISE DEBUG 'Name (%): %', frec.oid, frec.name;
        RAISE DEBUG 'Arguments: %', frec.arguments;
        RAISE DEBUG 'Identity arguments: %', frec.identity_arguments;
        RAISE DEBUG 'SQL query: %', sql_drop;
        BEGIN
            EXECUTE sql_drop;
        EXCEPTION
            WHEN OTHERS THEN
                RAISE EXCEPTION 'Could not drop function %.%. You might need to drop dependant objects. Postgres error: %', function_schema, function_name, SQLERRM;
        END;
    END LOOP;
END;
$$ LANGUAGE plpgsql;

Instead of calling DROP, you'd call:

SELECT postgis_cond_drop_function('public', 'ST_AsMVTGeom', 'geom geometry, bounds box2d, extent integer, buffer integer, clip_geom boolean default true');

The function would only drop ST_AsMVTGeom if the passing arguments (3rd argument) matches exactly the ones from the installed function. It has a caveat though, you need to pass the function arguments as stored by Postgres, not as declared in the CREATE FUNCTION (eg. integer and not int4), since that's what I could use to match them. Since we are doing it ourselves I don't think that's too big of an issue.

What do you think about this approach? Does it makes sense to you?

[strk]

Yes, makes sense to me (not having read details of the function). What I suggest is:

1) Rename the function to be clear about _which_ condition we're checking (or more genererically what's the goal, like "_postgis_drop_function_if_needed")

2) Pass the new signature to the function ? (or how can it tell whether it is needed to drop it or not ? That depends on the new function signature, right ?)

3) Integrate the function in the postgis upgrade function (implies using the function from postgis_drop_before.sql file

4) Integrate tests (could be useful to do something in sub create_upgrade_test_objects of run_test.pl, or further generalize that to find things to do in a pre-upgrade.sql script)

[Algunenano]

1) Rename the function to be clear about _which_ condition we're checking (or more genererically what's the goal, like "_postgis_drop_function_if_needed")

Makes sense.

2) Pass the new signature to the function ? (or how can it tell whether it is needed to drop it or not ? That depends on the new function signature, right ?)

The idea is that you pass the complete signature of the old function and it will only try to drop it if it matches the one installed, so passing the new signature isn't necessary.

3) Integrate the function in the postgis upgrade function (implies using the function from postgis_drop_before.sql file

That's the idea. I'm thinking on creating this auxiliar function and use it to drop the functions in postgis_drop_before.sql.

4 4) Integrate tests (could be useful to do something in sub create_upgrade_test_objects of run_test.pl, or further generalize that to find things to do in a pre-upgrade.sql script)

Automating this for all functions looks really complex, but I can probably hook the view tests I did above.

[strk]

The idea is that you pass the complete signature of the old function and it will only try to drop it if it matches the one installed, so passing the new signature isn't necessary.

You mean if it does NOT match the one installed, right ? The new signature isn't visible at upgrade time, because we drop the old one _before_ the new one is installed. So, to recap: the OLD (current) signature is known, while the NEW (to be installed) isn't.

Automating this for all functions looks really complex, but I can probably hook the view tests I did above.

A step in that direction would be good, even if it is only a manually edited sql file creating those constraining views. We can think about automating the generation of those view in a second step (I guess it would fail for some upgrade paths in that we didn't always restrict changes to compatible ones).

Having a test in advance is also good to _prove_ the usefulness of the code change…

[robe]

this sounds too hard to backport and is an age old problem. I'm not even convinced it's solvable without a change upstream.

[Algunenano]

This also applies to aggregates, for example a view over the ST_Makeline aggregation also breaks:

cannot drop function st_makeline(geometry) because other objects depend on it

This aggregation is being dropped unconditionally since 2.5:

$postgis_proc_upgrade$;
-- Aggregate ST_MakeLine (geometry) -- LastUpdated: 205
DO LANGUAGE 'plpgsql'
$postgis_proc_upgrade$
BEGIN
  IF 205 > version_from_num OR (
      205 = version_from_num AND version_from_isdev
    ) FROM _postgis_upgrade_info
  THEN
    EXECUTE 'DROP AGGREGATE IF EXISTS ST_MakeLine (geometry)';
    EXECUTE $postgis_proc_upgrade_parsed_def$ CREATE AGGREGATE ST_MakeLine (geometry) (
	SFUNC = pgis_geometry_accum_transfn,
	STYPE = internal,
	parallel = safe,
	FINALFUNC = pgis_geometry_makeline_finalfn
	);
 $postgis_proc_upgrade_parsed_def$;
  END IF;
END
$postgis_proc_upgrade$;
```

The exact same issue for ST_Collect:
```
    cartodb_user_d404665e-f81e-49c3-a4f2-86d7b3a96f24_db

-- Aggregate ST_Collect (geometry) -- LastUpdated: 205
DO LANGUAGE 'plpgsql'
$postgis_proc_upgrade$
BEGIN
  IF 205 > version_from_num OR (
      205 = version_from_num AND version_from_isdev
    ) FROM _postgis_upgrade_info
  THEN
    EXECUTE 'DROP AGGREGATE IF EXISTS ST_Collect (geometry)';
    EXECUTE $postgis_proc_upgrade_parsed_def$ CREATE AGGREGATE ST_Collect (geometry) (
	SFUNC = pgis_geometry_accum_transfn,
	STYPE = internal,
	parallel = safe,
	FINALFUNC = pgis_geometry_collect_finalfn
	);
 $postgis_proc_upgrade_parsed_def$;
  END IF;
END
$postgis_proc_upgrade$;

I guess this was done because it was considered simpler that updating postgresql system tables.

As with the previous issue, since this is executed with every update it's an infinite issue, but the good thing is that starting from PG12 this could be changed by CREATE OR REPLACE without dropping anything.

[robe]

I thought strk had already put in code to catch 12 and do a CREATE OR REPLACE in that case. I admit I haven't tested that and not sure we have tests in place for it.

[strk]

The ticket to implement PG12 support for upgrade of aggregates is #4352 (not done yet, and not planning to in the short term).

I think komzpa was going to try (or already implemented?) a catalogue updating code for #4386 - I suggest looking at that, and if possible make the solution more general (for example including that approach in postgis_upgrade.pl script directly, maybe as part of the broather aggregate handling change to be done as part of #4352)

[strk]

NOTE: ​https://git.osgeo.org/gitea/postgis/postgis/pulls/30 is how I added automated test to check for upgrade in presence of views built with ST_Union . You could do the same for this other function.

[Algunenano]

I plan to clean this file (postgis/postgis_drop_before.sql) a bit, document things as much as possible there and try to drop functions / aggregates that continue to exists (renamed parameters or added defaults).

The fist step is to review the drops and check when and why are there:

• addgeometrycolumn: A signature was dropped and it's not substituted by another. Keeping the DROP is fine.

• ST_MakeEnvelope: A new signature was added (float8, float8, float8, float8) during the 2.0 development, but it was replaced by a default (float8, float8, float8, float8, int) which already existed. I'm dropping the drop (o_O) since I don't think nobody is going to upgrade from 2.0.0dev.

• ST_AsX3D: Signature changed (the old one is no longer valid). Keeping the DROP is fine.

• UpdateGeometrySRID: Changed parameter name. Needs to be dropped conditionally (only if the params match the old signature, not the new one).

• Operators changed between 2.0 and 2.1: They are being renamed manually. Keeping them from now.
  • geometry_gist_sel_2d → gserialized_gist_sel_2d
  • geometry_gist_joinsel_2d → gserialized_gist_joinsel_2d
  • geography_gist_selectivity → gserialized_gist_sel_nd.
  • geography_gist_join_selectivity → gserialized_gist_joinsel_nd

• ST_AsLatLonText: When from multiple functions to one with default args. One signature can be dropped but the other has to be dropped conditionally.

• ST_AsTWKB: Same as ST_AsLatLonText.

• _st_linecrossingdirection / ST_LineCrossingDirection / _st_orderingequals/ st_orderingequals: Named to unnamed. Needs to be dropped conditionally.

• st_askml: Changed with defaults, but these signatures don't conflict with the current one. Simple DROP can be kept.

• st_buffer: Changed with defaults, but the signature doesn't conflict with the current one. Simple DROP can be kept.

• ST_AsMVT / ST_AsMVTGeom / ST_AsGeobuf / pgis_asgeobuf_transfn / pgis_asmvt_transfn: Signatures were changed during dev / RC development during 2.4.0. Dropping the DROPS as they shouldn't be necessary any longer.

• ST_CurveToLine: Changed with defaults, but these signatures don't conflict with the current one. Simple DROP can be kept.

• geometry_columns VIEW: We are already using CREATE OR REPLACE VIEW so we can drop this.

[Algunenano]

Correction: ST_AsTWKB was added and changed for 2.2.0, so keeping the drops for old devel versions is, IMO, unnecessary.

[Algunenano]

WIP in ​https://github.com/postgis/postgis/pull/415

[Algunenano]

geometry_columns VIEW: We are already using CREATE OR REPLACE VIEW so we can drop this.

I was wrong about this:

HINT:  type `SELECT postgis_extensions_upgrade(); to finish the upgrade. After upgrading, if you want to drop raster, run: DROP EXTENSION postgis_raster;
ERROR:  cannot change data type of view column "f_table_schema" from character varying(256) to name

I'll see if we can we really need an unconditional DROP or we can only drop it when the old view is found (similar to the function).

[algunenano]

In 17507:

Fix upgrade issues related to renamed function parameters

References #4334
Closes ​https://github.com/postgis/postgis/pull/415

[algunenano]

In 17508:

Rename update sql files

postgis/postgis_drop_after.sql → postgis/postgis_after_upgrade.sql
postgis/postgis_drop_before.sql → postgis/postgis_before_upgrade.sql

Closes #4334