Обсуждение: Re: meson's in-tree libpq header search order vs -Dextra_include_dirs

I took another look and got it working, after something else I'm using
insisted on installing libpq.  It's mostly no-brainer hunks like this:

-  dependencies: [frontend_code, libpq],
+  dependencies: [libpq, frontend_code],

For the new src/test/modules/test_oat_hooks/meson.build, I copied what
I'd seen in contrib modules that use libpq:

-  kwargs: pg_test_mod_args,
+  kwargs: pg_test_mod_args + {
+       'dependencies': [libpq] + pg_test_mod_args['dependencies'],
+  }

(I'm still green enough with Meson that I had to look that syntax up:
it's how you replace the values of common keys with the values from
the right hand dict[1], which ain't Pythonesque AFAIK.)

For ecpg and pg_regress, where I got stuck last time, I am still a bit
confused about whether it should go here:

-ecpg_inc = include_directories('.')
+ecpg_inc = [libpq_inc, include_directories('.')]

... or perhaps in intermediate ecpgXXX_inc variables or the final
include_directories directives.  In this version I just did that easy
thing and it works for me.  But is it the right place?

Likewise for pg_regress_inc, also used by ECPG and isolation tests:

-pg_regress_inc = include_directories('.')
+pg_regress_inc = [libpq_inc, include_directories('.')]

[1] https://mesonbuild.com/Reference-manual_elementary_dict.html

Added to commitfest: https://commitfest.postgresql.org/patch/6056/

I'm having to carry this patch in all my development branches for now
or I can't build on one of my regular dev machines, so I'm quite keen
to get this into the tree soon and would back-patch to 16.

I gather no one else is affected yet, but I assume you can see the
problem on a Mac by installing PostgreSQL with Homebrew/MacPorts, or
on CI if you do this to .cirrus.tasks.yml:

   setup_additional_packages_script: |
-    #pkg install -y ...
+    pkg install -y postgresql17-client

On Thu, 9 Oct 2025 at 16:06, Thomas Munro <thomas.munro@gmail.com> wrote:
>
> Added to commitfest: https://commitfest.postgresql.org/patch/6056/
>
> I'm having to carry this patch in all my development branches for now
> or I can't build on one of my regular dev machines, so I'm quite keen
> to get this into the tree soon and would back-patch to 16.
>

Can you confirm you still have this problem on current master? I tried
to reproduce it in debian 12 by installing `postgresql-server-dev-15`
and adding the -Dextra_include_dirs

> I gather no one else is affected yet, but I assume you can see the
> problem on a Mac by installing PostgreSQL with Homebrew/MacPorts, or
> on CI if you do this to .cirrus.tasks.yml:
>
>    setup_additional_packages_script: |
> -    #pkg install -y ...
> +    pkg install -y postgresql17-client
>

-- 
https://www.linkedin.com/in/gonzalemario

On Fri, Oct 10, 2025 at 8:09 AM Mario González Troncoso
<gonzalemario@gmail.com> wrote:
> Can you confirm you still have this problem on current master? I tried

Thanks for looking!  Yes.

> to reproduce it in debian 12 by installing `postgresql-server-dev-15`
> and adding the -Dextra_include_dirs

The problem is with libpq headers, not server headers.

Thinking about how to show this on Debian...  Also it'll fail to fail
if your libpq headers are new enough to be compatible, eg 18.  I guess
if you find an older libpq-dev .deb file, v16 is what my system is
clashing with, and unpack it into a temporary directory (something
like ar x XXX.deb then tar xf data.tar.XXX), and then point to the
headers in -Dextra_include_dirs, you should see it?

Or maybe just make a temporary file somewhere called libpq-fe.h that contains:

#error "wrong header included"

... and point to its parent with -Dextra_include_dirs.  The goal is
for the in-tree libpq-fe.h to be found sooner in the search path (as
it is with configure), completely hiding that poisoned file.

On 14/09/2025 06:23, Thomas Munro wrote:
> Added to commitfest: https://commitfest.postgresql.org/patch/6056/
> 
> I'm having to carry this patch in all my development branches for now
> or I can't build on one of my regular dev machines, so I'm quite keen
> to get this into the tree soon and would back-patch to 16.
> 
> I gather no one else is affected yet, but I assume you can see the
> problem on a Mac by installing PostgreSQL with Homebrew/MacPorts, or
> on CI if you do this to .cirrus.tasks.yml:
> 
>     setup_additional_packages_script: |
> -    #pkg install -y ...
> +    pkg install -y postgresql17-client

The patch seems harmless enough to me, no objections to committing it. 
However, I'm worried that we'll soon break it again. The new rule is 
apparently "include libpq first", but we have no way of enforcing it.

- Heikki

On Sat, Nov 1, 2025 at 7:14 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> The patch seems harmless enough to me, no objections to committing it.

Thanks!

> However, I'm worried that we'll soon break it again. The new rule is
> apparently "include libpq first", but we have no way of enforcing it.

Hmm, my meson-fu is weak, but there must surely be some way to declare
that you want libpq and have it automatically put things in the right
order, will look into that...

On Sat, Nov 1, 2025 at 10:13 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Sat, Nov 1, 2025 at 7:14 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> > However, I'm worried that we'll soon break it again. The new rule is
> > apparently "include libpq first", but we have no way of enforcing it.
>
> Hmm, my meson-fu is weak, but there must surely be some way to declare
> that you want libpq and have it automatically put things in the right
> order, will look into that...

If we really don't want every site that depends on both libpq and
frontend_code to have to remember to respect that order when declaring
dependencies, then we could instead change frontend_code to force
libpq_inc to appear in its include_directories before postgres_inc (=
where extra_include_dirs comes from).  This one-liner fixes the build
on my system:

 # for frontend binaries
 frontend_code = declare_dependency(
-  include_directories: [postgres_inc],
+  include_directories: [libpq_inc, postgres_inc],

That feels a little odd, because libpq is not really a dependency of
frontend_code, and not all frontend_code users also use libpq (though
almost all do).  Does this have any unwanted side-effects?  Is there a
better way to do this in a central place?

There are two other places that already have those two in
include_directories already, so their order should surely be flipped
to match, they just didn't happen to break on my system (I guess by
luck, ie not accessing APIs that changed incompatibly since the
version in my system-installed libpq headers).

On Sat, Nov 1, 2025 at 7:14 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> The patch seems harmless enough to me, no objections to committing it.
> However, I'm worried that we'll soon break it again. The new rule is
> apparently "include libpq first", but we have no way of enforcing it.

Here's a new version of the original approach with fixes for some
copy-and-pastes I'd missed, and a new patch to check the search order
in CI.  It adds a "poisoned" libpq-fe.h header into the search path to
break the build step if you get it wrong in meson or configure (cf
commit 4300d8b6 which fixed a case of this there).  It fails like
this:

[01:45:21.825] In file included from ../src/include/fe_utils/connect_utils.h:15,
[01:45:21.825]                  from ../src/bin/scripts/common.h:13,
[01:45:21.825]                  from ../src/bin/scripts/vacuumdb.c:17:
[01:45:21.825] /tmp/poisoned_headers/libpq-fe.h:1:2: error: #error
"external header hides in-tree header"
[01:45:21.825]     1 | #error "external header hides in-tree header"
[01:45:21.825]       |  ^~~~~

There must surely be a more mesonic way to do this with declarations
rather than the order in a flimsy list that is copied all over the
place (I guess that is probably supposed to be thought of as an
unordered set...?), but I'm not sure I was on the right path with my
v2 as mentioned and it didn't survive this test.  This approach is OK
for now, I think... if someone ever shows up with a patch to tackle it
more fundamentally, the order will presumably become more flexible, so
the new order will still be valid but unimportant.

Will wait another day for a better idea or objection to show up, and
otherwise commit these to 16+.