Обсуждение: 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.


>
> Thanks,
> Andrew

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

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)

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
havingdescribed in:
 

https://www.postgresql.org/message-id/000001db1df8%2449765bb0%24dc631310%24%40pcorp.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:
> 
> https://www.postgresql.org/message-id/000001db1df8%2449765bb0%24dc631310%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.

> 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