Horton

Thread

Commits

  1. meson: Export all libcommon functions in Windows builds

  1. Exporting float_to_shortest_decimal_buf(n) with Postgres 17 on Windows

    Andrew Kane <andrew@ankane.org> — 2024-09-13T20:07:13Z

    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?

Thanks,
Andrew

  2. Re: Exporting float_to_shortest_decimal_buf(n) with Postgres 17 on Windows

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-09-13T20:58:20Z

    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

  3. Re: Exporting float_to_shortest_decimal_buf(n) with Postgres 17 on Windows

    Florents Tselai <florents.tselai@gmail.com> — 2024-09-13T21:27:47Z

    
> 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

  4. Re: Exporting float_to_shortest_decimal_buf(n) with Postgres 17 on Windows

    Nathan Bossart <nathandbossart@gmail.com> — 2024-09-13T21:41:23Z

    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

  5. Re: Exporting float_to_shortest_decimal_buf(n) with Postgres 17 on Windows

    Andrew Kane <andrew@ankane.org> — 2024-09-13T22:26:42Z

    > 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 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
>

  6. Re: Exporting float_to_shortest_decimal_buf(n) with Postgres 17 on Windows

    Michael Paquier <michael@paquier.xyz> — 2024-09-14T00:15:25Z

    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

  7. Re: Exporting float_to_shortest_decimal_buf(n) with Postgres 17 on Windows

    Vladlen Popolitov <v.popolitov@postgrespro.ru> — 2024-12-23T10:32:04Z

    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.

  8. Re: Exporting float_to_shortest_decimal_buf(n) with Postgres 17 on Windows

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-12-23T11:05:27Z

    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)

  9. Re: Exporting float_to_shortest_decimal_buf(n) with Postgres 17 on Windows

    Vladlen Popolitov <v.popolitov@postgrespro.ru> — 2024-12-23T12:14:05Z

    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.

  10. Re: Exporting float_to_shortest_decimal_buf(n) with Postgres 17 on Windows

    Vladlen Popolitov <v.popolitov@postgrespro.ru> — 2024-12-23T12:16:50Z

    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.

  11. Re: Exporting float_to_shortest_decimal_buf(n) with Postgres 17 on Windows

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-12-25T16:19:49Z

    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)

  12. Re: Exporting float_to_shortest_decimal_buf(n) with Postgres 17 on Windows

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-25T16:34:47Z

    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

  13. Re: Exporting float_to_shortest_decimal_buf(n) with Postgres 17 on Windows

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-12-25T17:25:18Z

    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)

  14. Re: Exporting float_to_shortest_decimal_buf(n) with Postgres 17 on Windows

    Regina Obe <r@pcorp.us> — 2025-01-03T16:08:14Z

    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

  15. Re: Exporting float_to_shortest_decimal_buf(n) with Postgres 17 on Windows

    Vladlen Popolitov <v.popolitov@postgrespro.ru> — 2025-01-09T11:13:11Z

    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.

  16. RE: Exporting float_to_shortest_decimal_buf(n) with Postgres 17 on Windows

    Regina Obe <lr@pcorp.us> — 2025-01-09T16:35:18Z

    > 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