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

Updating from v3.7.2 to v4.0.0 breaks some functions #86

Closed
mngr777 opened this issue Oct 3, 2022 · 9 comments · Fixed by #87
Closed

Updating from v3.7.2 to v4.0.0 breaks some functions #86

mngr777 opened this issue Oct 3, 2022 · 9 comments · Fixed by #87

Comments

@mngr777
Copy link
Contributor

mngr777 commented Oct 3, 2022

Functions that are renamed and not re-created in h3--3.7.2--4.0.0.sql are left linked to old C function names, that are no longer available.

To reproduce:

  • install v3.7.2;
  • create extension;
  • install v4.0.0;
  • update extension;
  • execute query: select h3_polygon_to_cells(st_setsrid(st_geomfromtext('POLYGON ((30 60, 31 60, 31 61, 30 61, 30 60))'),4326)::polygon, array[]::polygon[], 8):
    [42883] ERROR: could not find function "h3_polyfill" in file "/usr/local/pgsql/lib/h3.so"
@mngr777
Copy link
Contributor Author

mngr777 commented Oct 3, 2022

List of affected functions:

# select proname, prosrc, proargnames from pg_proc where proname like 'h3_%' and prosrc like 'h3_%' and prosrc not like proname;
             proname              |                        prosrc                        |         proargnames         
----------------------------------+------------------------------------------------------+-----------------------------
 h3_cell_to_local_ij              | h3_experimental_h3_to_local_ij                       | {origin,index}
 h3_cells_to_directed_edge        | h3_get_h3_unidirectional_edge                        | {origin,destination}
 h3_cells_to_multi_polygon        | h3_set_to_multi_polygon                              | {"",exterior,holes}
 h3_directed_edge_to_boundary     | h3_get_h3_unidirectional_edge_boundary               | {edge}
 h3_directed_edge_to_cells        | h3_get_h3_indexes_from_unidirectional_edge           | {edge,origin,destination}
 h3_edge_length                   | h3_exact_edge_length                                 | {edge,unit}
 h3_get_base_cell_number          | h3_get_base_cell                                     | 
 h3_get_directed_edge_destination | h3_get_destination_h3_index_from_unidirectional_edge | {edge}
 h3_get_directed_edge_origin      | h3_get_origin_h3_index_from_unidirectional_edge      | {edge}
 h3_get_hexagon_area_avg          | h3_hex_area                                          | {resolution,unit}
 h3_get_icosahedron_faces         | h3_get_faces                                         | 
 h3_get_num_cells                 | h3_num_hexagons                                      | {resolution}
 h3_get_pentagons                 | h3_get_pentagon_indexes                              | {resolution}
 h3_get_res_0_cells               | h3_get_res_0_indexes                                 | 
 h3_great_circle_distance         | h3_point_dist                                        | {a,b,unit}
 h3_is_valid_cell                 | h3_is_valid                                          | 
 h3_is_valid_directed_edge        | h3_unidirectional_edge_is_valid                      | {edge}
 h3_local_ij_to_cell              | h3_experimental_local_ij_to_h3                       | {origin,coord}
 h3_origin_to_directed_edges      | h3_get_h3_unidirectional_edges_from_hexagon          | 
 h3_polygon_to_cells              | h3_polyfill                                          | {exterior,holes,resolution}
(20 rows)

@zachasme
Copy link
Owner

zachasme commented Oct 4, 2022

That's bad, but also great! This must be the same problem as #47.

Do you (or someone else) know how to properly rename functions in update scripts? Am I forced to remove and re-create them if the C function names change?

@mngr777
Copy link
Contributor Author

mngr777 commented Oct 4, 2022

I think, alter function <...> rename to <...>; create or replace function <...> is the correct way.
From create function documentation:

If you drop and then recreate a function, the new function is not the same entity as the old; you will have
to drop existing rules, views, triggers, etc. that refer to the old function. Use CREATE OR REPLACE FUNCTION
to change a function definition without breaking objects that refer to the function.

So this is the correct way of changing a definition of existing function. Since C symbol name is explicitly stored in pg_proc it's probably not related to function name anymore, just happens to be a default.

@Komzpa may be you know for sure?

@Komzpa
Copy link
Contributor

Komzpa commented Oct 4, 2022

The best way we found in PostGIS to handle C level renames is to stub+deprecate the old C name and drop the old function and to create a new one.

The stubs: https://github.com/postgis/postgis/blob/master/postgis/postgis_legacy.c

Really, we never ever change the C names of the functions, that's one reason why they're in various styles in PostGIS - only create new and deprecate old.

The reason why you want to stub out the old C name as that on Postgres version upgrade pg_upgrade may not let you upgrade both extension and cluster at the same time if a C function is missing in new library.

A highly-enterprisey way would be also to move the old function to some h3_legacy_compat extension (I hope you don't, but maybe in some other place that will be useful).

@jaredlander
Copy link

Do the new changes mean that the functions no longer work with geometry types?

Using the example in the documentation I see this behavior.

SELECT h3_lat_lng_to_cell(POINT('37.3615593,-122.0553238'), 5);
 h3_lat_lng_to_cell
--------------------
 85e35e73fffffff
(1 row)

This works as expected, but that requires the data to be pulled in as a string of long/lat.

That won't work for points stored as geometry as seen here.

SELECT h3_lat_lng_to_cell(ST_SetSRID(ST_MakePOINT(37.3615593,-122.0553238), 4326), 5);
ERROR:  function h3_lat_lng_to_cell(geometry, integer) does not exist
LINE 1: SELECT h3_lat_lng_to_cell(ST_SetSRID(ST_MakePOINT(37.3615593...
               ^
HINT:  No function matches the given name and argument types. You might need to add explicit type casts.

With 3.7.2, h3_geo_to_h3() expected a geometry type.

Similarly, h3_cell_to_boundary() now returns what looks like WKT rather than a WKB of the polygon.

@zachasme
Copy link
Owner

As part of 4.0.0, I have extracted the postgis/geometry integration into its own extension, h3_postgis (distributed in the same bundle as h3 on pgxn).

Depending on how you are installing, you might only need to run CREATE EXTENSION h3_postgis;.

I need to be clearer about this in the README, since most people is going to want to install that along side the base extension.

@jaredlander
Copy link

I had no clue h3_postgis existed. Cool idea. Are the function names the same as the new API and you provide geometry objects?

@zachasme
Copy link
Owner

Many of the function names changed as part of 4.0.0 (following the changes to uber/h3).

They are all listed in the auto-generated docs at: https://pgxn.org/dist/h3/docs/api.html#PostGIS.Integration

Most functions should support both geometry and geography.

@jaredlander
Copy link

That's huge. I froze to an earlier version because I couldn't use geometry, but now I'm going to try this other extension. Thanks!

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

Successfully merging a pull request may close this issue.

4 participants