Exporting float_to_shortest_decimal_buf(n) with Postgres 17 on Windows

Andrew Kane <andrew@ankane.org> writes:

With Postgres 17 RC1 on Windows, `float_to_shortest_decimal_buf` and
`float_to_shortest_decimal_bufn` are not longer exported. This causes
`unresolved external symbol` linking errors for extensions that rely on
these functions (like pgvector). Can these functions be exported like
previous versions of Postgres?

AFAICS it's in the exact same place it was in earlier versions.
You might need to review your linking commands.

regards, tom lane

On 13 Sep 2024, at 11:07 PM, Andrew Kane <andrew@ankane.org> wrote:

Hi,

With Postgres 17 RC1 on Windows, `float_to_shortest_decimal_buf` and `float_to_shortest_decimal_bufn` are not longer exported. This causes `unresolved external symbol` linking errors for extensions that rely on these functions (like pgvector). Can these functions be exported like previous versions of Postgres?

Probably a Windows thing?
Just tried on Darwin with 17_RC1, and pgvector 0.7.4 build installcheck’s OK.

On Fri, Sep 13, 2024 at 04:58:20PM -0400, Tom Lane wrote:

Andrew Kane <andrew@ankane.org> writes:

With Postgres 17 RC1 on Windows, `float_to_shortest_decimal_buf` and
`float_to_shortest_decimal_bufn` are not longer exported. This causes
`unresolved external symbol` linking errors for extensions that rely on
these functions (like pgvector). Can these functions be exported like
previous versions of Postgres?

AFAICS it's in the exact same place it was in earlier versions.
You might need to review your linking commands.

I do see a fair amount of special handling for f2s.c in the build files. I
wonder if something got broken for Windows in the switch from the MSVC
scripts to meson.

--
nathan

Probably a Windows thing?

Correct, it's only on Windows.

I do see a fair amount of special handling for f2s.c in the build files.

I
wonder if something got broken for Windows in the switch from the MSVC
scripts to meson.

This was my hunch as well since none of the source files changed. Also,
neither function is present with `dumpbin /EXPORTS /SYMBOLS
lib\postgres.lib`, which led me to believe it may need to be addressed
upstream.

- Andrew

On Fri, Sep 13, 2024 at 2:41 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:

On Fri, Sep 13, 2024 at 03:26:42PM -0700, Andrew Kane wrote:

This was my hunch as well since none of the source files changed. Also,
neither function is present with `dumpbin /EXPORTS /SYMBOLS
lib\postgres.lib`, which led me to believe it may need to be addressed
upstream.

Hmm. Perhaps we are not careful enough with the calls of
msvc_gendef.pl in meson.build, missing some spots?
--
Michael

Michael Paquier писал(а) 2024-09-14 03:15:

On Fri, Sep 13, 2024 at 03:26:42PM -0700, Andrew Kane wrote:

This was my hunch as well since none of the source files changed.
Also,
neither function is present with `dumpbin /EXPORTS /SYMBOLS
lib\postgres.lib`, which led me to believe it may need to be addressed
upstream.

Hmm. Perhaps we are not careful enough with the calls of
msvc_gendef.pl in meson.build, missing some spots?
--
Michael

Dear colleagues

I found the reason of this bug and the fix for it.

dumpbin processes libraries to prepare DEF file. libcommon_srv.a does
not have
all necessary object files in the moment of processing due to meson
optimisation:
this library marked as install=false, and all objects depend on
it also marked as install=false.
meson is smart enough to exclude obj-files, that are not required on
later stages.

The option to set install=true is not good: all files are built
correctly,
but not needed library will be installed.

Fortunatelly meson has option to force put all object files to library -
add
dependency with the flag link_whole .

I made the one-line patch and it fixes this issue.

-        'dependencies': opts['dependencies'] + [ssl],
+        'dependencies': opts['dependencies'] + [ssl] + 
[declare_dependency( link_whole  : cflag_libs)],

P.S. Other libraries in this meson.build file (libcommon.a and
libcommon_shlib.a)
are not affected by this issue and patch, as they both marked
install=true and build
without any missed files.

--
Best regards,

Vladlen Popolitov.

On 23/12/2024 12:32, Vladlen Popolitov wrote:

I found the reason of this bug and the fix for it.

Cool!

Fortunatelly meson has option to force put all object files to
library - add dependency with the flag link_whole .

I made the one-line patch and it fixes this issue.

-        'dependencies': opts['dependencies'] + [ssl],
+        'dependencies': opts['dependencies'] + [ssl] + 
[declare_dependency( link_whole  : cflag_libs)],

I'm no meson expert and don't have a Windows system to test on, but this
seems like a weird place to add the option. Could you do this instead:

diff --git a/src/common/meson.build b/src/common/meson.build
index 538e0f43d55..76a7f68fe30 100644
--- a/src/common/meson.build
+++ b/src/common/meson.build
@@ -184,6 +184,7 @@ foreach name, opts : pgcommon_variants

lib = static_library('libpgcommon@0@'.format(name),
link_with: cflag_libs,
+ link_whole: cflag_libs,
c_pch: pch_c_h,
kwargs: opts + {
'include_directories': [

--
Heikki Linnakangas
Neon (https://neon.tech)

Heikki Linnakangas писал(а) 2024-12-23 14:05:

On 23/12/2024 12:32, Vladlen Popolitov wrote:

I found the reason of this bug and the fix for it.

Cool!

Fortunatelly meson has option to force put all object files to
library - add dependency with the flag link_whole .

I made the one-line patch and it fixes this issue.

-        'dependencies': opts['dependencies'] + [ssl],
+        'dependencies': opts['dependencies'] + [ssl] + 
[declare_dependency( link_whole  : cflag_libs)],

I'm no meson expert and don't have a Windows system to test on, but
this seems like a weird place to add the option. Could you do this
instead:

diff --git a/src/common/meson.build b/src/common/meson.build
index 538e0f43d55..76a7f68fe30 100644
--- a/src/common/meson.build
+++ b/src/common/meson.build
@@ -184,6 +184,7 @@ foreach name, opts : pgcommon_variants

lib = static_library('libpgcommon@0@'.format(name),
link_with: cflag_libs,
+ link_whole: cflag_libs,
c_pch: pch_c_h,
kwargs: opts + {
'include_directories': [

Yes, it is also working option. I applied it and tested in the current
master under Windows,
it works.

Attached patch changes this line in meson.build

link_with: cflag_libs,
+ link_whole: cflag_libs,
c_pch: pch_c_h,

--
Best regards,

Vladlen Popolitov.

Vladlen Popolitov писал(а) 2024-12-23 15:14:

Yes, it is also working option. I applied it and tested in the current
master under Windows,
it works.

Attached patch changes this line in meson.build

link_with: cflag_libs,
+ link_whole: cflag_libs,
c_pch: pch_c_h,

I also created entry in the commit fest with this patch
https://commitfest.postgresql.org/51/5457/

--
Best regards,

Vladlen Popolitov.

On 23/12/2024 14:16, Vladlen Popolitov wrote:

Vladlen Popolitov писал(а) 2024-12-23 15:14:

 Yes, it is also working option. I applied it and tested in the
current master under Windows,
it works.

Attached patch changes this line in meson.build

       link_with: cflag_libs,
+      link_whole: cflag_libs,
       c_pch: pch_c_h,

I also created entry in the commit fest with this patch
https://commitfest.postgresql.org/51/5457/

Ok, committed that, thanks1

--
Heikki Linnakangas
Neon (https://neon.tech)

Heikki Linnakangas <hlinnaka@iki.fi> writes:

Ok, committed that, thanks1

The question this patch brings to my mind is whether libpgport
doesn't need the same treatment.

regards, tom lane

On 25/12/2024 18:34, Tom Lane wrote:

Heikki Linnakangas <hlinnaka@iki.fi> writes:

Ok, committed that, thanks1

The question this patch brings to my mind is whether libpgport
doesn't need the same treatment.

Good point. Yes it does.

I tested that by adding a dummy call to COMP_CRC32C() in a test module,
and letting cirrus CI build it:

[17:10:32.715] dummy_index_am.c.obj : error LNK2001: unresolved external
symbol pg_comp_crc32c

Committed the same fix for libpqport.

--
Heikki Linnakangas
Neon (https://neon.tech)

I tested this with a patched version of the EDB PG 17 installer that includes this patch and it fixed the issue I was having described in:

/messages/by-id/000001db1df8$49765bb0$dc631310$@pcorp.us

It would be great if this was backported to PG17.

The new status of this patch is: Needs review

Regina Obe писал(а) 2025-01-03 23:08:

I tested this with a patched version of the EDB PG 17 installer that
includes this patch and it fixed the issue I was having described in:

/messages/by-id/000001db1df8$49765bb0$dc631310$@pcorp.us

It would be great if this was backported to PG17.

The new status of this patch is: Needs review

Hi Regina,

As I see, this patch applied to master and immediately backported to
PG16 and PG17 (changes
will appear in new versions, old versions are unchangeable). If this
answered your question,
status should be returned to committed again.
--
Best regards,

Vladlen Popolitov.

Regina Obe писал(а) 2025-01-03 23:08:

I tested this with a patched version of the EDB PG 17 installer that
includes this patch and it fixed the issue I was having described in:

https://www.postgresql.org/message-

id/000001db1df8%2449765bb0%24dc6313

10%24%40pcorp.us

It would be great if this was backported to PG17.

The new status of this patch is: Needs review

Hi Regina,

As I see, this patch applied to master and immediately backported to
PG16 and PG17 (changes
will appear in new versions, old versions are unchangeable). If this answered
your question, status should be returned to committed again.
--
Best regards,

Vladlen Popolitov.

Apologies for my confusion. I've flipped it back to committed.

Thanks,
Regina