Re: meson vs. llvm bitcode files

Hi,

On Thu, 12 Mar 2026 at 13:54, Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
> Some review comments from me.

Thank you for looking into this!

> v9-0001-meson-Add-postgresql-extension.pc-for-building-ex.patch
>
> Need to think about whether "extension" is the correct term.

It looks correct to me. Do you have any suggestions?

> New meson message:
>
> NOTICE: Future-deprecated features used:
> * 0.62.0: {'pkgconfig.generate variable for builtin directories'}

Fixed.

> The comment that introduces postgresql-extension-warnings.pc says
>
> +# Extension modules should likely also use -fwrapv etc. But it it's a
> bit odd
> +# to expose it to a .pc file?
>
> but then -fwrapv ends up in postgresql-extension.pc anyway. Not sure
> what was intended here.

I asked Andres off-list and Andres said that we need to have these
flags inside the .pc file but it is not very nice since these flags
(-fwrapv for example) change the behavior. Maybe Andres could clarify
this better.

> Also, the description "PostgreSQL Extension Support - Compiler
> Warnings" could be clarified, like "with recommended compiler
> warnings" or "with compiler warnings same as core code" or similar.

Done. I changed it to "PostgreSQL Extension Support with compiler
warnings the same as core code". I am not sure about
uppercase/lowercase but it seems okay to me.

> The Requires list in my case is for example
>
> Requires: krb5-gssapi, icu-uc, icu-i18n, ldap, libxml-2.0 >= 2.6.23,
> liblz4, openssl, zlib, libzstd >= 1.4.0
>
> but I don't think these are actually required for building extensions
> (unless a particular extension directly makes use of one of them, in
> which case they should declare that on their own).

It seems that is how meson pkgconfig.generate() handles the
dependencies, please see [1]:

...
* Dependencies provided by pkg-config are added into Requires: or
Requires.private:. If a version was specified when declaring that
dependency it will be written into the generated file too.
...

> If we are going to install these .pc files, we also need to build them
> with with makefiles. Alternatively, we could not install them for now
> and just use them internally.

Unfortunately, these .pc files are always installed in meson build. I
added a WIP patch (0007) for building .pc files with makefiles, I am
not sure if I am following the correct way. I would appreciate any
help on this.

> v9-0002-meson-Test-building-extensions-by-using-postgresq.patch
>
> Not sure if this was meant to be kept or it's just for local testing.

I think we can have it in Postgres, it shows that generated .pc files
work and extensions can be built by using these .pc files and using
meson build. But maybe we can build one extension instead of three (or
a dummy extension), what do you think?

> New meson warnings:
>
> WARNING: Deprecated features used:
> * 0.55.0: {'ExternalProgram.path'}
> * 0.56.0: {'meson.build_root'}

Fixed.

> src/test/modules/ seems like the wrong location, since it's not a
> module or a test module.
>
> I don't know if it's possible to make meson use a different file than
> meson.build, but if so, it might be better to keep these test
> meson.build files together with their extensions, like
> contrib/amcheck/meson-test.build. Similar to how we have "PGXS" build
> support in the makefiles. Otherwise, I'm afraid this will get
> annoying and error-prone if one has to remember to update other files
> under src/test/ when adding for example a new .sql file to amcheck.

I don't think we can use something other than meson.build. I solved
that by editing the test_meson_extension script, now meson-test.build
files live under the actual contrib/${extension}/ directory and the
test script moves them to the correct directory. I needed to use the
get_option('meson_source_dir') hack to get paths of the source files.

> Also, the driver script is at 'src/tools/ci/test_meson_extensions',
> but you are using it outside of CI, so that's not a good location.

You are right, I moved the test_meson_extensions script under the 'src/tools/'.

> v9-0003-meson-WIP-Add-docs-for-postgresql-extension.pc.patch
>
> Let's not rename existing ids.
>
> It seems to me that the .pc file can also be used without meson.
> Let's take that into account a bit. For example, the
> id="extend-postgres-meson" could be id="extend-postgres-pkg-config" or
> similar.

Sorry but I didn't understand how we can add a pkg-config
documentation without renaming existing ids. 'Extension Building
Infrastructure' is covered by <sect1 id="extend-pgxs">. I guess we
would want to add pkg-config documentation under the extension
building infrastructure, but it is something other than PGXS. So, it
being under '<sect1 id="extend-pgxs">' doesn't sound correct to me.

> Your text ends with a colon. Did you mean to add more text? Maybe an
> example meson.build would be good.

Yes, sorry for that. I added an example meson.build file.

> v9-0005-meson-Add-LLVM-bitcode-emissions-for-contrib-libr.patch
>
> +# some libraries include "hstore/hstore.h" instead of "hstore.h"
>
> It seems to me that the former is correct, but if not then we should
> fix it.

I think both of them are correct and the comment is wrong. Source
files in the contrib/hstore directory include "hstore.h" and files
outside of this directory include "hstore/hstore.h". I changed this
comment to '# Files outside of the current directory include hstore as
"hstore/hstore.h"'.

[1] https://mesonbuild.com/Pkgconfig-module.html#implicit-dependencies

--
Regards,
Nazir Bilal Yavuz
Microsoft