postgrespro / pgsphere

PgSphere provides spherical data types, functions, operators, and indexing for PostgreSQL.
https://pgsphere.org
BSD 3-Clause "New" or "Revised" License
17 stars 14 forks source link

mingw64 built pg_sphere for PG17 doesn't work under windows VC built PostgreSQL EDB release #130

Open robe2 opened 2 weeks ago

robe2 commented 2 weeks ago

I usually build windows binaries under mingw64 and deploy these on PostgreSQL Windows EDB distributions. This works just fine for the pg_sphere 1.5.2 (master) branch for the 14-16 I have tried building so far, except for PostgreSQL 17.

Note I have already built binaries for PostGIS, MobilityDb, pgRouting, h3 and all those build fine and regress fine.

Everything regresses fine on my mingw64 PG 17 install, but when I try to do

CREATE EXTENSION pg_sphere;

on my PostgreSQL Windows EDB 17, pg_sphere.dll": The specified procedure could not be found.

I've narrowed the issue down to output.c, but I can't figure out what is wrong with that file. Perhaps it's using some function that used to be there for windows for PG < 17 perhaps.

How I know it's the culprit is if I change the Makefile to below - leaving out output.o

OBJS = src/sscan.o src/sparse.o src/sbuffer.o src/vector3d.o src/point.o \
             src/euler.o src/circle.o src/circle_sel.o src/line.o src/ellipse.o src/polygon.o \
            src/path.o src/box.o src/gq_cache.o src/gist.o src/gist_support.o src/key.o src/gnomo.o src/epochprop.o src/brin.o

I at least get a library that tries to load, but fails on not being able to find the functions in output.c Any thoughts on what this issue could be?

esabol commented 2 weeks ago

Our GitHub Actions CI workflow builds and tests with PostgreSQL 17 on Linux. That said, it seems the last build was 4 months ago, well before the final release of PostgreSQL 17.0. So it's possible something changed, but I think it's unlikely.

When reporting any issue, please include the exact error message you're seeing. It's hard for anyone to diagnose the cause otherwise. If we knew the exact functions in output.c, that would be useful.

If I had to guess why you're seeing some issue with Windows not finding some function in output.c, my guess would be that Windows is loading an older pg_sphere.dll. But I'm just pulling that out of a hat without more concrete information.

robe2 commented 2 weeks ago

No I don't think it's anything wrong with PG17, cause as I mentioned my mingw64 PG17 works fine and regresses fine.

It's installing the library in windows EDB vc++ builds that fails and just for PG17

I've narrowed down the issue further.

If I remark out all the clauses like

if (sphere_output_precision != INT_MAX)

In the Datum functions of output.c, it installs fine under my PG17 windows VC++ edb build.

which always forces those functions to go thru the out_compat functions.

I just don't understand why this is only in issue with PG17 and not the other versions, I'm assuming maybe it can't resolve SPoint or something for some reason.

FWIW, PG17 is the first version of PostgreSQL to no longer support Visual Studio directly, all those scripts have been ripped out in place of meson. So could be some sort of meson bug involving VC++ that it's not exporting some things.

robe2 commented 2 weeks ago

Narrowed down further issue seems to be calling of the inlined function

static inline void
spherepoint_out_buffer(StringInfo si, const SPoint *sp)
robe2 commented 2 weeks ago

Narrowed down further issue seems to be calling of the inlined function

static inline void
spherepoint_out_buffer(StringInfo si, const SPoint *sp)

I narrowed the issue down further, it seems to be that eventually calling

static void pgs_strinfo_put_d64(StringInfo si, double value)

and in the call to

double_to_shortest_decimal_buf(value, buf);

in https://github.com/postgrespro/pgsphere/blob/master/src/output.c#L163

Perhaps the windows vc++ builds aren't exporting that function out in PG17 so might be a bug upstream. I would expect inclusion of #include "common/shortest_dec.h" as you have is sufficient to bring that function in.

robe2 commented 2 weeks ago

reported upstream as well - https://www.postgresql.org/message-id/000001db1df8%2449765bb0%24dc631310%24%40pcorp.us

esabol commented 2 weeks ago

No I don't think it's anything wrong with PG17, cause as I mentioned my mingw64 PG17 works fine and regresses fine.

Um, I didn't say it was?

vitcpp commented 1 week ago

Just some thoughts...

pgSphere uses a number of functions from postgresql which are declared with extern specifier. I do not see any linkage problems with such functions. The function double_to_shortest_decimal_buf is declared in the header without extern specifier in REL_17_STABLE. With gcc, by default, it should have external linkage as those functions with extern specifier. Probably, under mswin the situation is different. Some compilation options may differ.

P.S. I'm not sure which postgresql core C functions I can use. If double_to_shortest_decimal_buf is restricted from using in extensions, I should fix it in pgsphere. My opinion, it is important function that should unify the output of doubles in extensions. I would rather keep using of this function.

esabol commented 1 week ago

@vitcpp wrote:

The function double_to_shortest_decimal_buf is declared in the header without extern specifier in REL_17_STABLE.

Was it declared with "extern" in previous versions of PostgreSQL (16.x, 15.x, etc.)?

vitcpp commented 1 week ago

@esabol The declaration seems the same since 12.x (without extern), no any differences in shortest_dec.h. I guess the issue is related to some compiler settings.

vitcpp commented 1 week ago

I tried to build pg17 (REL_17_STABLE) and pgsphere under ms win using msys2 + mingw64 runtime (pg17 was compiled with some small fix). I used meson to build pg17. pgsphere was compiled without any changes. I succeeded to start pg17 and do create extension pg_sphere. It was completed without any errors.

@robe2 Could you please provide some details about your env? How can I configure the env like yours?

P.S. I forgot to say, I tried the master branch which is not released yet. After fixing this issue, we can create a new 1.5.2 release.

esabol commented 1 week ago

@esabol The declaration seems the same since 12.x (without extern), no any differences in shortest_dec.h. I guess the issue is related to some compiler settings.

Then I don't see why the OP would see different results between compiling the extension with <=16.x and 17.x.