Thread: meson PGXS compatibility

meson PGXS compatibility

From
Andres Freund
Date:
Hi,

As the meson support stands right now, one cannot easily build an extension
against a postgres built with meson. As discussed at the 2022 unconference
[1], one way to make that easier for extensions is for meson to generate a
complete enough Makefile.global for pgxs.mk to work.

This is a series of patches towards that goal. The first four simplify some
aspects of Makefile.global, and then the fifth generates Makefile.global etc.


0001: autoconf: Unify CFLAGS_SSE42 and CFLAGS_ARMV8_CRC32C

  Right now emit the cflags to build the CRC objects into architecture specific
  variables. That doesn't make a whole lot of sense to me - we're never going to
  target x86 and arm at the same time, so they don't need to be separate
  variables.

  It might be better to instead continue to have CFLAGS_SSE42 /
  CFLAGS_ARMV8_CRC32C be computed by PGAC_ARMV8_CRC32C_INTRINSICS /
  PGAC_SSE42_CRC32_INTRINSICS and then set CFLAGS_CRC based on those. But it
  seems unlikely that we'd need other sets of flags for those two architectures
  at the same time.

  The separate flags could be supported by the meson build instead, it'd just
  add unneccessary complexity.


0002: autoconf: Rely on ar supporting index creation

  With meson we don't require ranlib. But as it is set in Makefile.global and
  used by several platforms, we'd need to detect it.

  FreeBSD, NetBSD, OpenBSD, the only platforms were we didn't use AROPT=crs,
  all have supported the 's' option for a long time.

  On macOS we ran ranlib after installing a static library. This was added a
  long time ago, in 58ad65ec2def. I cannot reproduce an issue in more recent
  macOS versions.

  I'm on the fence about removing the "touch $@" from the rule building static
  libs. That was added because of macos's ranlib not setting fine-grained
  timestamps. On a modern mac ar and ranlib are the same binary, and maybe
  that means that ar has the same issue? Both do set fine-grained
  timestamps:

  cc ../test.c -o test.o -c
  rm -f test.a; ar csr test.a test.o ; ranlib test.a; python3 -c "import os;print(os.stat('test.a').st_mtime_ns)"
  1664999109090448534

  But I don't know how far back that goes. We could just reformulate the
  comment to mention ar instead of ranlib.


  Tom, CCing you due to 58ad65ec2def and 826eff57c4c.


0003: aix: Build SUBSYS.o using $(CC) -r instead of $(LD) -r

  This is the only direct use of $(LD), and xlc -r and gcc -r end up with the
  same set of symbols and similar performance (noise is high, so hard to say if
  equivalent).

  Now that $(LD) isn't needed anymore, remove it from src/Makefile.global

  While at it, add a comment why -r is used.


0004: solaris: Check for -Wl,-E directly instead of checking for gnu LD

  This allows us to get rid of the nontrivial detection of with_gnu_ld,
  simplifying meson PGXS compatibility. It's also nice to delete libtool.m4.

  I don't like the SOLARIS_EXPORT_DYNAMIC variable I invented. If somebody has
  a better idea...


0005: meson: Add PGXS compatibility

  The actual meson PGXS compatibility. Plenty more replacements to do, but
  suffices to build common extensions on a few platforms.

  What level of completeness do we want to require here?


  A few replacements worth thinking about:

  - autodepend - I'm inclined to set it to true when using a gcc like
    compiler. I think extension authors won't be happy if suddenly their
    extensions don't rebuild reliably anymore. An --enable-depend like
    setting doesn't make sense for meson, so we don't have anything to source it
    from.
  - {LDAP,UUID,ICU}_{LIBS,CFLAGS} - might some extension need them?


  For some others I think it's ok to not have replacement. Would be good for
  somebody to check my thinking though:

  - LIBOBJS, PG_CRC32C_OBJS, TAS: Not needed because we don't build
    the server / PLs with the generated makefile
  - ZIC: only needed to build tzdata as part of server build
  - MSGFMT et al: translation doesn't appear to be supported by pgxs, correct?
  - XMLLINT et al: docs don't seem to be supported by pgxs
  - GENHTML et al: supporting coverage for pgxs-in-meson build doesn't seem worth it
  - WINDRES: I don't think extensions are bothering to generate rc files on windows


My colleague Bilal has set up testing and verified that a few extensions build
with the pgxs compatibility layer, on linux at last. Currently pg_qualstats,
pg_cron, hypopg, orafce, postgis, pg_partman work. He also tested pgbouncer,
but for him that failed both with autoconf and meson generated pgxs.

I wonder if and where we could have something like this tested continually?

Greetings,

Andres Freund

[1] https://wiki.postgresql.org/wiki/PgCon_2022_Developer_Unconference#Meson_new_build_system_proposal

Attachment

Re: meson PGXS compatibility

From
Tom Lane
Date:
Andres Freund <andres@anarazel.de> writes:
>   On macOS we ran ranlib after installing a static library. This was added a
>   long time ago, in 58ad65ec2def. I cannot reproduce an issue in more recent
>   macOS versions.

I agree that shouldn't be necessary anymore (and if it is, we'll find
out soon enough).

>   I'm on the fence about removing the "touch $@" from the rule building static
>   libs. That was added because of macos's ranlib not setting fine-grained
>   timestamps. On a modern mac ar and ranlib are the same binary, and maybe
>   that means that ar has the same issue? Both do set fine-grained
>   timestamps:

Please see the commit message for 826eff57c4c: the issue seems to arise
only with specific combinations of software, in particular with non-Apple
versions of "make" (although maybe later Apple builds have fixed make's
failure to read sub-second timestamps?).  That's a relatively recent hack,
and I'm very hesitant to conclude that we don't need it anymore just
because you failed to reproduce an issue locally.  It very possibly isn't
a problem in a meson build, though, depending on how much meson depends on
file timestamps.

            regards, tom lane



Re: meson PGXS compatibility

From
Andres Freund
Date:
Hi,

On 2022-10-05 16:20:22 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> >   On macOS we ran ranlib after installing a static library. This was added a
> >   long time ago, in 58ad65ec2def. I cannot reproduce an issue in more recent
> >   macOS versions.
> 
> I agree that shouldn't be necessary anymore (and if it is, we'll find
> out soon enough).

Cool.


> >   I'm on the fence about removing the "touch $@" from the rule building static
> >   libs. That was added because of macos's ranlib not setting fine-grained
> >   timestamps. On a modern mac ar and ranlib are the same binary, and maybe
> >   that means that ar has the same issue? Both do set fine-grained
> >   timestamps:
> 
> Please see the commit message for 826eff57c4c: the issue seems to arise
> only with specific combinations of software, in particular with non-Apple
> versions of "make" (although maybe later Apple builds have fixed make's
> failure to read sub-second timestamps?).

My understanding, from that commit message, was that the issue originates in
apple's ranlib setting the timestamp to its components but only queries / sets
it using second granularity. I verified that apple's ranlib and ar these days
just set the current time, at a high granularity, as the mtime.  Whether or
not make then hides the problem seems not that relevant if the source of the
problem is gone, no?


> That's a relatively recent hack, and I'm very hesitant to conclude that we
> don't need it anymore just because you failed to reproduce an issue locally.

Yea, that's why I was hesitant as well. I'll reformulate the comment to
reference ar instead of ranlib instead.


> It very possibly isn't a problem in a meson build, though, depending on how
> much meson depends on file timestamps.

Most of the timestamp sensitive stuff is dealt with by ninja, rather than
meson. ninja does take timestamps into account when determining what to
rebuild - although I suspect this specific problem wouldn't occur even with a
problematic ar/ranlib version, because the relevant timestamps will be on the
.c (etc) files, rather than the .a. Ninja has the whole dependency graph, so
it knows what dependencies it has to rebuild, without needing to check
timestamps of intermediary objects.

Ninja does support build rules where it checks the timestamps of build outputs
to see if targets depending on those build outputs have to be rebuilt, or not,
because the target didn't change. But the relevant option ("restat") isn't set
for compiler / linker invocations in the build.ninja meson generates.

Restat is however set for the "custom_command"s we use to generate all kinds
of sources. Sometimes that leads to the set of build steps shrinking
rapidly. E.g. a touch src/include/catalog/pg_namespace.dat starts leads to
ninja considering 1135 targets out of date, but as genbki.pl doesn't end up
changing any files, it's done immediately after that...

[1/1135 1   0%] Generating src/include/catalog/generated_catalog_headers with a custom command

Greetings,

Andres Freund



Re: meson PGXS compatibility

From
Tom Lane
Date:
Andres Freund <andres@anarazel.de> writes:
> My understanding, from that commit message, was that the issue originates in
> apple's ranlib setting the timestamp to its components but only queries / sets
> it using second granularity. I verified that apple's ranlib and ar these days
> just set the current time, at a high granularity, as the mtime.  Whether or
> not make then hides the problem seems not that relevant if the source of the
> problem is gone, no?

Well, (a) it seemed to happen in only some circumstances even back then,
so maybe your testing didn't catch it; and (b) even assuming that Apple
has fixed it in recent releases, there may still be people using older,
un-fixed versions.  Why's it such a problem to keep the "touch" step?

            regards, tom lane



Re: meson PGXS compatibility

From
Andres Freund
Date:
Hi,

On 2022-10-05 16:58:46 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > My understanding, from that commit message, was that the issue originates in
> > apple's ranlib setting the timestamp to its components but only queries / sets
> > it using second granularity. I verified that apple's ranlib and ar these days
> > just set the current time, at a high granularity, as the mtime.  Whether or
> > not make then hides the problem seems not that relevant if the source of the
> > problem is gone, no?
> 
> Well, (a) it seemed to happen in only some circumstances even back then,
> so maybe your testing didn't catch it; and (b) even assuming that Apple
> has fixed it in recent releases, there may still be people using older,
> un-fixed versions.  Why's it such a problem to keep the "touch" step?

It isn't! That's why I said that I was on the fence about removing the touch
in my first email and then that I'd leave the touch there and just
s/ranlib/ar/ in my reply to you.

Greetings,

Andres Freund



Re: meson PGXS compatibility

From
Peter Eisentraut
Date:
On 05.10.22 22:07, Andres Freund wrote:
> My colleague Bilal has set up testing and verified that a few extensions build
> with the pgxs compatibility layer, on linux at last. Currently pg_qualstats,
> pg_cron, hypopg, orafce, postgis, pg_partman work. He also tested pgbouncer,
> but for him that failed both with autoconf and meson generated pgxs.

pgbouncer doesn't use pgxs.



Re: meson PGXS compatibility

From
Andres Freund
Date:
Hi,

On 2022-10-06 11:34:26 +0200, Peter Eisentraut wrote:
> On 05.10.22 22:07, Andres Freund wrote:
> > My colleague Bilal has set up testing and verified that a few extensions build
> > with the pgxs compatibility layer, on linux at last. Currently pg_qualstats,
> > pg_cron, hypopg, orafce, postgis, pg_partman work. He also tested pgbouncer,
> > but for him that failed both with autoconf and meson generated pgxs.
> 
> pgbouncer doesn't use pgxs.

Ah, right. It'd still be interesting to make sure it works, but looks like the
only integration point is pg_config --includedir and pg_config --libdir, so
it should...

Greetings,

Andres Freund



Re: meson PGXS compatibility

From
Andres Freund
Date:
Hi,

On 2022-10-05 13:07:10 -0700, Andres Freund wrote:
> 0003: aix: Build SUBSYS.o using $(CC) -r instead of $(LD) -r
>
>   This is the only direct use of $(LD), and xlc -r and gcc -r end up with the
>   same set of symbols and similar performance (noise is high, so hard to say if
>   equivalent).
>
>   Now that $(LD) isn't needed anymore, remove it from src/Makefile.global
>
>   While at it, add a comment why -r is used.

Unfortunately experimenting further with this it turns out I was wrong: While
xlc -r results in the same set of symbols, that's not true with gcc -r, at
least with some versions of gcc. gcc ends up exposing some of the libgcc
symbols.

That can be rectified by adding -nostartfiles -nodefaultlibs, but that
basically makes the change as-is pointless.

I think it'd still be good to get rid of setting LD via configure.ac,
mirroring the detection logic in meson sounds like a bad plan.

Given this is aix specific, and only the aix linker works on aix (binutils'
doesn't), I think the best plan might be to just hardcode ld in the rule
generating postgres.imp.

Greetings,

Andres Freund



Re: meson PGXS compatibility

From
Andres Freund
Date:
Hi,

On 2022-10-05 13:07:10 -0700, Andres Freund wrote:
> 0004: solaris: Check for -Wl,-E directly instead of checking for gnu LD
> 
>   This allows us to get rid of the nontrivial detection of with_gnu_ld,
>   simplifying meson PGXS compatibility. It's also nice to delete libtool.m4.
> 
>   I don't like the SOLARIS_EXPORT_DYNAMIC variable I invented. If somebody has
>   a better idea...

A cleaner approach could be to add a LDFLAGS_BE and emit the -Wl,-E into it
from both configure and meson, solely based on whether -Wl,-E is supported.  I
haven't verified cygwin, but on our other platforms that seems to do the right
thing.

Greetings,

Andres Freund



Re: meson PGXS compatibility

From
Peter Eisentraut
Date:
On 05.10.22 22:07, Andres Freund wrote:
> 0001: autoconf: Unify CFLAGS_SSE42 and CFLAGS_ARMV8_CRC32C

I wonder, there has been some work lately to use SIMD and such in other 
places.  Would that affect what kinds of extra CPU-specific compiler 
flags and combinations we might need?

Seems fine otherwise.
> 0005: meson: Add PGXS compatibility
> 
>    The actual meson PGXS compatibility. Plenty more replacements to do, but
>    suffices to build common extensions on a few platforms.
> 
>    What level of completeness do we want to require here?

I have tried this with a few extensions.  Seems to work alright.  I 
don't think we need to overthink this.  The way it's set up, if someone 
needs additional variables set, they can easily be added.




Re: meson PGXS compatibility

From
John Naylor
Date:

On Wed, Oct 12, 2022 at 12:50 PM Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>
> On 05.10.22 22:07, Andres Freund wrote:
> > 0001: autoconf: Unify CFLAGS_SSE42 and CFLAGS_ARMV8_CRC32C
>
> I wonder, there has been some work lately to use SIMD and such in other
> places.  Would that affect what kinds of extra CPU-specific compiler
> flags and combinations we might need?

In short, no. The functionality added during this cycle only uses instructions available by default on the platform. CRC on Arm depends on armv8-a+crc, and CRC on x86-64 depends on SSE4.2. We can't assume those currently.

--
John Naylor
EDB: http://www.enterprisedb.com

Re: meson PGXS compatibility

From
Andres Freund
Date:
Hi,

On 2022-10-10 14:35:14 -0700, Andres Freund wrote:
> On 2022-10-05 13:07:10 -0700, Andres Freund wrote:
> > 0004: solaris: Check for -Wl,-E directly instead of checking for gnu LD
> > 
> >   This allows us to get rid of the nontrivial detection of with_gnu_ld,
> >   simplifying meson PGXS compatibility. It's also nice to delete libtool.m4.
> > 
> >   I don't like the SOLARIS_EXPORT_DYNAMIC variable I invented. If somebody has
> >   a better idea...
> 
> A cleaner approach could be to add a LDFLAGS_BE and emit the -Wl,-E into it
> from both configure and meson, solely based on whether -Wl,-E is supported.  I
> haven't verified cygwin, but on our other platforms that seems to do the right
> thing.

I think that does look better. See the attached 0003.


The attached v3 of this patch has an unchanged 0001 (CRC cflags).

For 0002, I still removed LD from Makefile.global, but instead just hardcoded
ld in the export file generation portion of the backend build - there's no
working alternative linkers, and we already hardcode a bunch of other paths in
mkldexport.

0003 is changed significantly - as proposed in the message quoted above, I
introduced LDFLAGS_EX_BE and moved the detection -Wl,-E (I used
-Wl,--export-dynamic, which we previously only used on FreeBSD) into
configure, getting rid of export_dynamic.

0004, the patch introducing PGXS compat, saw a few changes too:
- I implemented one of the FIXMEs, the correct determination of strip flags
- I moved the bulk of the pgxs compat code to src/makefiles/meson.build - imo
  src/meson.build got bulked up too much with pgxs-emulation code
- some minor cleanups

Greetings,

Andres Freund

Attachment

Re: meson PGXS compatibility

From
Andres Freund
Date:
Hi,

On 2022-10-12 07:50:07 +0200, Peter Eisentraut wrote:
> On 05.10.22 22:07, Andres Freund wrote:
> > 0001: autoconf: Unify CFLAGS_SSE42 and CFLAGS_ARMV8_CRC32C
> 
> I wonder, there has been some work lately to use SIMD and such in other
> places.  Would that affect what kinds of extra CPU-specific compiler flags
> and combinations we might need?

The current infrastructure is very CRC specific, so I don't think this change
would stand in the way of using sse specific flags in other places.


> > 0005: meson: Add PGXS compatibility
> > 
> >    The actual meson PGXS compatibility. Plenty more replacements to do, but
> >    suffices to build common extensions on a few platforms.
> > 
> >    What level of completeness do we want to require here?
> 
> I have tried this with a few extensions.  Seems to work alright.  I don't
> think we need to overthink this.  The way it's set up, if someone needs
> additional variables set, they can easily be added.

Yea, I am happy enough with it now that the bulk is out of src/meson.build and
strip isn't set to an outright wrong value.

Thanks!

Andres



Re: meson PGXS compatibility

From
Peter Eisentraut
Date:
On 13.10.22 07:23, Andres Freund wrote:
>>> 0005: meson: Add PGXS compatibility
>>>
>>>     The actual meson PGXS compatibility. Plenty more replacements to do, but
>>>     suffices to build common extensions on a few platforms.
>>>
>>>     What level of completeness do we want to require here?
>>
>> I have tried this with a few extensions.  Seems to work alright.  I don't
>> think we need to overthink this.  The way it's set up, if someone needs
>> additional variables set, they can easily be added.
> 
> Yea, I am happy enough with it now that the bulk is out of src/meson.build and
> strip isn't set to an outright wrong value.

How are you planning to proceed with this?  I thought it was ready, but 
it hasn't moved in a while.




Re: meson PGXS compatibility

From
Andres Freund
Date:
Hi,

On 2022-12-01 08:43:19 +0100, Peter Eisentraut wrote:
> On 13.10.22 07:23, Andres Freund wrote:
> > > > 0005: meson: Add PGXS compatibility
> > > > 
> > > >     The actual meson PGXS compatibility. Plenty more replacements to do, but
> > > >     suffices to build common extensions on a few platforms.
> > > > 
> > > >     What level of completeness do we want to require here?
> > > 
> > > I have tried this with a few extensions.  Seems to work alright.  I don't
> > > think we need to overthink this.  The way it's set up, if someone needs
> > > additional variables set, they can easily be added.
> > 
> > Yea, I am happy enough with it now that the bulk is out of src/meson.build and
> > strip isn't set to an outright wrong value.
> 
> How are you planning to proceed with this?  I thought it was ready, but it
> hasn't moved in a while.

I basically was hoping for a review of the prerequisite patches I posted at
[1], particularly 0003 (different approach than before, comparatively large).

Here's an updated version of the patches. There was a stupid copy-paste bug in
the prior version of the 0003 / export_dynamic patch.

I'll push 0001, 0002 shortly. I don't think 0002 is the most elegant approach,
but it's not awful. I'd still like some review for 0003, but will try to push
it in a few days if that's not forthcoming.

Greetings,

Andres Freund

[1] https://www.postgresql.org/message-id/20221013051648.ufz7ud2b5tioctyt%40awork3.anarazel.de

Attachment

Re: meson PGXS compatibility

From
Andres Freund
Date:
Hi,

On 2022-12-01 12:17:51 -0800, Andres Freund wrote:
> I'll push 0001, 0002 shortly. I don't think 0002 is the most elegant approach,
> but it's not awful. I'd still like some review for 0003, but will try to push
> it in a few days if that's not forthcoming.

Pushed 0003 (move export_dynamic determination to configure.ac) and 0004 (PGXS
compatibility). Hope there's no fallout from 0003.

Greetings,

Andres Freund