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

ST_LineLocatePoint for geography and one point linestring crashes backend #354

Closed
robe2 opened this issue Jul 22, 2023 · 14 comments
Closed

Comments

@robe2
Copy link

robe2 commented Jul 22, 2023

Describe the bug
PostGIS garden test triggered a crash on ptarray_locate_point, which is I think mostly a copy of the MobilityDb version.

Details here:
https://trac.osgeo.org/postgis/ticket/5456

backtrace shows crash around here:
https://github.com/MobilityDB/MobilityDB/blob/develop/mobilitydb/src/point/geography_functions.c#L595

I can trigger the same error with MobilityDb's function on my PostGIS 3.3.3 instance.

To Reproduce
Steps to reproduce the behavior:

SELECT ST_LineLocatePoint('0102000020E610000001000000000000000000F03F0000000000000040'::geography, 'POINT(-11.1111111 40)'::geography, false);

Expected behavior
I'm not sure why this should even be allowed? Probably should just error out if you pass in a linestring that has only one point.

Specifications (please complete the following information):
Use the commands:

SELECT version();
SELECT postgis_full_version();
SELECT mobilitydb_full_version();

output

PostgreSQL 15.1, compiled by Visual C++ build 1914, 64-bit
POSTGIS="3.3.3 3.3.3" [EXTENSION] PGSQL="150" GEOS="3.11.2-CAPI-1.17.2" SFCGAL="SFCGAL 1.4.1, CGAL 5.3, BOOST 1.78.0" PROJ="8.2.1" GDAL="GDAL 3.6.4, released 2023/04/17" LIBXML="2.9.14" LIBJSON="0.16" LIBPROTOBUF="1.2.1" WAGYU="0.5.0 (Internal)" TOPOLOGY RASTER
MobilityDB 1.1.0, PostgreSQL 15.0, PostGIS 3.3.3, GEOS 3.11.2-CAPI-1.17.2, PROJ 8.2.1
@robe2
Copy link
Author

robe2 commented Jul 22, 2023

I'm also thinking the fact that we have the same names of functions is going to pose a problem for people upgrading. Not sure the easiest way to fix that. It might be good to start thinking about using prefixes for your custom functions that don't overlap with PostGIS ones. For example pgrouting uses prefixes pgr_ and pgr so their functions never collide with PostGIS ones. h3-pg extension prefixes theirs with h3 and _h3 so their function names never collide with PostGIS ones.

@pramsey
Copy link

pramsey commented Jul 24, 2023

estebanzimanyi added a commit to estebanzimanyi/MobilityDB that referenced this issue Jul 26, 2023
mschoema pushed a commit that referenced this issue Jul 26, 2023
* Fix MEOS functions

* Fix deleteTime function

* Fix Issue #354
@estebanzimanyi
Copy link
Member

Regarding the naming of functions, all the functions added in PostGIS 3.4 coming from MobilityDB were designed for PostGIS and thus they are commented out with #ifdefs when compiled with PostGIS 3.4.
Is this what you meant Regina ?

@robe2
Copy link
Author

robe2 commented Jul 27, 2023

Yes. The issue I see with that is, say a user is upgrading from PostGIS 3.3 to PostGIS 3.4 and they have MobilityDB 1.1 installed for PostGIS 3.3.

If they try to do a PostGIS 3.4 upgrade before they do a MobilityDb upgrade, then the install will fail because of the new security features in PostgreSQL, will prevent replace of a function installed by another extension. I haven't tested myself, but I suspect that is what will happen.

So now you have a dependency order of upgrade which is pretty annoying.
Also if you are doing a pg_upgrade say from PG15 from say MobilityDb 1.1 which was compiled against PostGIS 3.3, to
PG16, again things will fail, because even though the .so file name hasn't changed, if you remove a function from a .so file, when pg_upgrade goes to install it, it will fail and roll back the upgrade.

@estebanzimanyi
Copy link
Member

We finally decided to remove the functions from MobilityDB. It would simplify everything for both PostGIS and MobilityDB. Users that need these functions would be required to upgrade to PostGIS 3.4 but as consequence will have available all goodies in the latest release.

@estebanzimanyi
Copy link
Member

As an aside, we did a revision of the geography functions now in PostGIS 3.4.

@robe2
Copy link
Author

robe2 commented Jul 28, 2023

As an aside, we did a revision of the geography functions now in PostGIS 3.4.

* We generalized the use of a spheroid for these functions PR [Use spheroid in geography functions #359](https://github.com/MobilityDB/MobilityDB/pull/359), [Use spheroid in geography functions #353](https://github.com/MobilityDB/MobilityDB/pull/353)

* Correctly interpolate between geographic points PR [Correctly interpolate between geographic points #349](https://github.com/MobilityDB/MobilityDB/pull/349)
  These changes are now relevant for PostGIS.

Thanks. I'll ticket on our end for a fix before our 3.4.0 release

@estebanzimanyi
Copy link
Member

When packaging PostGIS 3.4.0 for Windows, please use our alpha release
https://github.com/MobilityDB/MobilityDB/releases/tag/v1.1.0-alpha

@robe2
Copy link
Author

robe2 commented Jul 28, 2023

I've ticketed on our end - https://trac.osgeo.org/postgis/ticket/5460

Regarding the v1.1.0. I think I'm having some issues with it, but will get back to you on that once I figure out if it's something wrong on my end. Last I tried last week, it compiled, but was giving a "NOT a WIN32% application" or some such thing when I went to load (even with making sure PostGIS was in loaded libraries). I haven't seen that error in a while. This was against PG16beta / PostGIS 3.4.0 and latest mobility develop branch. I'll put in a separate ticket if I need help on it.

@estebanzimanyi
Copy link
Member

Dear Regina
Another PR that must also be put on your ticket is #353

@robe2
Copy link
Author

robe2 commented Jul 29, 2023

Dear Regina Another PR that must also be put on your ticket is #353

Added, but @pramsey has already committed the others. I'll try to get in before I release beta2. Next time feel free to just update the ticket or provide a patch on a pull request or ticket

@estebanzimanyi
Copy link
Member

I am sorry for that. I didn't know. I will do as instructed next time.

@robe2
Copy link
Author

robe2 commented Aug 1, 2023

I think pramsey caught it as all those changes already seemed to be in place.
Regarding my "NOT a WIN32% application" I traced that back to json-c (something to do with my cmake compiling of that library, fixes it. I have the same issue in PostGIS using that newer one so not specific to mobilitydb. I'll open a ticket if I run into further issues, but at a glance mobilitydb 1.1.0alpha seems to be compiling and loading fine (at least on PG15 / PostGIS 3.4).

I think you can close this ticket out unless you are waiting for your release before you close.

@estebanzimanyi
Copy link
Member

Many thanks Regina !!!

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

No branches or pull requests

3 participants