Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken function renames #87

Merged
merged 4 commits into from
Oct 10, 2022
Merged

Fix broken function renames #87

merged 4 commits into from
Oct 10, 2022

Conversation

zachasme
Copy link
Owner

@zachasme zachasme commented Oct 5, 2022

Thanks to @Komzpa and @mngr777 I finally know how to properly handle C-level function renames.

Previously we went back and altered the upgrade-scripts (so the rest of the tests could pass), but in hindsight that was a bad idea.

This PR tries to restore the upgrade-scripts, and adds stub C functions (inspired by postgis) instead.

At this point, user installations could be in pretty different states, which should be taken into account.

I've started out with a simple commit with a failing test, exposing the issue. I'll need to create the proper upgrade script as well.

Closes #86.
Closes #47.

@zachasme zachasme self-assigned this Oct 5, 2022
Due to mishandling of C-level renames in previous releases,
this upgrade attempts to align user-installations as much as possible
by copying the full-install script in its entirety
(except casts/operators/etc)
@zachasme zachasme marked this pull request as ready for review October 5, 2022 08:59
@zachasme
Copy link
Owner Author

zachasme commented Oct 5, 2022

@Komzpa + @mngr777: I went with a combination of your suggestions, which will be to stub out old C-functions and do an alter function <...> rename to <...>; create or replace function <...>.

In order to align user-installations as much as possible I've copied almost the entire installation script into the next upgrade (except types/casts/operators/operator classes). This makes also the new test pass.

How do you feel about this approach?

h3/src/deprecated.c Outdated Show resolved Hide resolved
Forgot to remove reference to PostGIS in the copied code - oops!

Co-authored-by: Darafei Praliaskouski <me@komzpa.net>
@mngr777
Copy link
Contributor

mngr777 commented Oct 7, 2022

Functions and casts that are supposed to be dropped by h3--3.7.2--4.0.0.sql are not actually dropped, because h3 extension depends on them.
After installing v3.7.2 and upgrading to 'unreleased' in upgrade-correction branch:

# select proname, prosrc, proargnames from pg_proc where proname like 'h3_%' and prosrc not like 'h3_%';
           proname            |                                prosrc                                 |    proargnames     
------------------------------+-----------------------------------------------------------------------+--------------------
 h3_to_children_slow          |  SELECT __h3_to_children_aux($1, $2, -1)                              | {index,resolution}
 h3_hex_area                  |  SELECT h3_hex_area($1, CASE WHEN $2 THEN 'km' ELSE 'm' END)          | {resolution,km}
 h3_edge_length               |  SELECT h3_edge_length($1, CASE WHEN $2 THEN 'km' ELSE 'm' END)       | {resolution,km}
 h3_geo_to_h3                 |  SELECT h3_geo_to_h3($1::point, $2);                                  | {"",resolution}
 h3_geo_to_h3                 |  SELECT h3_geo_to_h3($1::geometry, $2);                               | {"",resolution}
 h3_to_geometry               |  SELECT ST_SetSRID(h3_to_geo($1)::geometry, 4326)                     | 
 h3_to_geography              |  SELECT h3_to_geometry($1)::geography                                 | 
 h3_to_geo_boundary_geometry  |  SELECT ST_SetSRID(h3_to_geo_boundary($1, $2)::geometry, 4326)        | {"",extend}
 h3_to_geo_boundary_geography |  SELECT h3_to_geo_boundary_geometry($1, $2)::geography                | {"",extend}
 h3_polyfill                  |  SELECT h3_polyfill(exterior, holes, resolution) FROM (              +| {multi,resolution}
                              |         SELECT                                                       +| 
                              |             -- extract exterior ring of each polygon                 +| 
                              |             ST_MakePolygon(ST_ExteriorRing(poly))::polygon exterior, +| 
                              |             -- extract holes of each polygon                         +| 
                              |             (SELECT array_agg(hole)                                  +| 
                              |                 FROM (                                               +| 
                              |                     SELECT ST_MakePolygon(ST_InteriorRingN(          +| 
                              |                         poly,                                        +| 
                              |                         generate_series(1, ST_NumInteriorRings(poly))+| 
                              |                     ))::polygon AS hole                              +| 
                              |                 ) q_hole                                             +| 
                              |             ) holes                                                  +| 
                              |         -- extract single polygons from multipolygon                 +| 
                              |         FROM (                                                       +| 
                              |             select (st_dump(multi)).geom as poly                     +| 
                              |         ) q_poly GROUP BY poly                                       +| 
                              |     ) h3_polyfill;                                                    | 
 h3_polyfill                  |  SELECT h3_polyfill($1::geometry, $2)                                 | {multi,resolution}

Trying to drop a function manually:

# DROP FUNCTION h3_polyfill(geometry, integer);
ERROR:  cannot drop function h3_polyfill(geometry,integer) because extension h3 requires it
HINT:  You can drop extension h3 instead.

The dependency has to be removed first:

# alter extension h3 drop function h3_polyfill(geometry, integer);
ALTER EXTENSION
# drop function if exists h3_polyfill(geometry, integer);
DROP FUNCTION

Strangely, alter extension h3 update doesn't show any messages. Creating h3_postgis after update fails:

# create extension h3_postgis;
ERROR:  cast from type h3index to type geometry already exists

@Komzpa your expertise is needed again.

I only found an example in PostGIS auto-generated update file postgis--ANY--3.3.0dev.sql (there are lots of this statements):

DO $$ BEGIN ALTER EXTENSION postgis DROP CAST (raster AS bytea);EXCEPTION WHEN object_not_in_prerequisite_state OR undefined_function OR undefined_object THEN  RAISE NOTICE '%', SQLERRM; WHEN OTHERS THEN  RAISE EXCEPTION 'Got % - %', SQLSTATE, SQLERRM; END $$ LANGUAGE 'plpgsql';
DO $$ BEGIN ALTER EXTENSION postgis DROP FUNCTION st_bytea(raster);EXCEPTION WHEN object_not_in_prerequisite_state OR undefined_function OR undefined_object THEN  RAISE NOTICE '%', SQLERRM; WHEN OTHERS THEN  RAISE EXCEPTION 'Got % - %', SQLSTATE, SQLERRM; END $$ LANGUAGE 'plpgsql';

@Komzpa
Copy link
Contributor

Komzpa commented Oct 7, 2022

Upgrade scripts running in ALTER EXTENSION contexts glue and unglue the functions to extensions automatically, you are not supposed to run the extensions upgrade scripts manually - in this case indeed you need to attach/detach functions manually.

If you moved some functions between extensions, I don't know how to do that starting PG15 anymore - previously you could unpackage the functions and then CREATE EXTENSION FROM unpackaged; now it is forbidden for security reasons. So drop & create new.

Probably some drops are just lost in the upgrade script?

PostGIS has weird upgrade scripts that need to account for many kinds of broken installs fixed by drunk monkeys, so upgrading from the state where someone removed a function that was supposed to be there should be supported, that's why the weird construct.

@mngr777
Copy link
Contributor

mngr777 commented Oct 7, 2022

I just had some update files left in Postgres extension directory.
Everything seems to work after I removed them.
Sorry about that.

@zachasme zachasme merged commit 1b906aa into main Oct 10, 2022
@zachasme zachasme deleted the upgrade-correction branch October 10, 2022 08:29
@zachasme
Copy link
Owner Author

Thanks for the input, I've released a new version that should fix the broken functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants