Frustrating issue with PGXS

On Mon, Jun 25, 2007 at 11:42:28PM +1200, Eddie Stanley wrote:

Hi,

I spent the best part of the day trying to work this out - I was working on a
system setup with PG 8.2, and wanted to work with 8.3 for development.

I installed as follows:

1. CVS checkout
2. ./configure prefix=~/install_dir
3. gmake prefix=~/install_dir
4. gmake install prefix=~/install_dir

Got a datastore up and running with initdb; created some test tables, everything
seemed to be fine.

The problem came when I wanted to build a simple C function - I used the
following makefile:

MODULES = example
PGXS := $(shell ~/install_dir/bin/pg_config --pgxs)
include $(PGXS)

The program compiled fine, however I noticed the compilation was refering to
files in /usr/pkg (from the 8.2 installation). I would have thought specifing
the version of pg_config from the 8.3 installation would be sufficent, but it
wasn't.

In /lib/postgresql/pgxs/src/Makefile.global, I needed to change

PG_CONFIG = pg_config
to
PG_CONFIG = ~/install_dir/bin/pg_config

Could this have been avoided if the Makefile.global had
PG_CONFIG = $(prefix)/bin/pg_config
?

I was actually about to post on this just a couple of days ago - it seems
pgxs really needs pg_config to be in your PATH. The quick fix would be for
you to run

PATH=~/install_dir/bin/:$PATH make

That'll make sure your local pg_config gets picked up instaed. That's what
I ended up donig. It's just a workaround, but it's one that works :-)

//Magnus

Magnus Hagander <magnus@hagander.net> writes:

I was actually about to post on this just a couple of days ago - it seems
pgxs really needs pg_config to be in your PATH.

Correct --- how else is it going to find out where the installation is?

Eddie's proposed solution is of course circular reasoning...

regards, tom lane

On Mon, Jun 25, 2007 at 10:43:37AM -0400, Tom Lane wrote:

Magnus Hagander <magnus@hagander.net> writes:

I was actually about to post on this just a couple of days ago - it seems
pgxs really needs pg_config to be in your PATH.

Correct --- how else is it going to find out where the installation is?

You can specify the full path in the command to pg_config in your Makefile.
It'd be neat if the makefile could fint that out and use it for further
references to pg_config. I haven't had time to look into if this is at all
possible, though.

This is the easy error btw - you can get some fairly funky results if you
have 8.3devel locally and then say 8.0 with different compile options in
your PATH. Then it'll use your 8.3devel pg_config for the first step and
then fall back on the one in the PATH for later steps..

//Magnus

Magnus Hagander <magnus@hagander.net> writes:

On Mon, Jun 25, 2007 at 10:43:37AM -0400, Tom Lane wrote:

Correct --- how else is it going to find out where the installation is?

You can specify the full path in the command to pg_config in your Makefile.
It'd be neat if the makefile could fint that out and use it for further
references to pg_config. I haven't had time to look into if this is at all
possible, though.

The trick is to override this bit in Makefile.global:

PG_CONFIG = pg_config

bindir := $(shell $(PG_CONFIG) --bindir)
datadir := $(shell $(PG_CONFIG) --sharedir)
... etc ...

It might be better if the standard invocation of a PGXS build were not

ifdef USE_PGXS
PGXS := $(shell pg_config --pgxs)
include $(PGXS)

but something like

ifdef USE_PGXS
PG_CONFIG := pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)

to sync these invocations of pg_config with the ones in
Makefile.global. I'm not sure though how to get this setting to
override the one in Makefile.global ... or should we just remove
that one?

regards, tom lane

ifdef USE_PGXS
PGXS := $(shell pg_config --pgxs)
include $(PGXS)

but something like

ifdef USE_PGXS
PG_CONFIG := pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)

to sync these invocations of pg_config with the ones in
Makefile.global. I'm not sure though how to get this setting to
override the one in Makefile.global ... or should we just remove
that one?

That would break existing Makefiles that use the "please take the first
pg_config in the path" feature, which rather make sense (it just means
that you want the extension for your current postgresql).

However you may replace the other appearance with the following:

ifndef PG_CONFIG
PG_CONFIG = pg_config
endif

So as to enable

sh> make PG_CONFIG=/my/manual/path/to/pg_config install

invocations without fear to be overwritten, if some people do not like the
path convention. Otherwise the following does the trick with a temporary
replacement of the PATH environment variable just for one command under
sh-compatible shells:

sh> PATH=/my/manual/path/to:$PATH make install

and is shorter. This could be added to the documentation.

--
Fabien.

Fabien COELHO <coelho@cri.ensmp.fr> writes:

ifdef USE_PGXS
PGXS := $(shell pg_config --pgxs)
include $(PGXS)

but something like

ifdef USE_PGXS
PG_CONFIG := pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)

That would break existing Makefiles that use the "please take the first
pg_config in the path" feature, which rather make sense (it just means
that you want the extension for your current postgresql).

How would it break them? The default definition is still PG_CONFIG =
pg_config, this just moves where that definition appears.

regards, tom lane

Dear Eddie,

MODULES = example
PGXS := $(shell ~/install_dir/bin/pg_config --pgxs)

This is indeed not the intented use of pgxs: it breaks a desirable
property of the makefile that is should be "generic" wrt postgresql, that
is not particular to an installation.

You are really expected to rely on the path, although the definition you
wrote is indeed tempting, although doomed to failure. From the
documentation, one finds:

"""
33.9.7. Extension Building Infrastructure

...

PGXS := $(shell pg_config --pgxs)
include $(PGXS)

The last two lines should always be the same. Earlier in the file, you
assign variables or add custom make rules.

...

The extension is compiled and installed for the PostgreSQL installation
that corresponds to the first pg_config command found in your path.
"""

There could be a clearer comment about the path assumption in "pgxs.mk"
and how to change the path easily from the shell? The "should be the
same" could be changed for "MUST always be the same..." with some
additional comments.

Could this have been avoided if the Makefile.global had
PG_CONFIG = $(prefix)/bin/pg_config ?

That would not work, because "pg_config" is needed to know what the prefix
(or rather bindir) is, as Tom pointed out. I think this is also because
the "installation" may be moved around, so it is not necessarily the
configure-time prefix which is used with some package systems.

His suggestion about using a PG_CONFIG macro in the initial makefile,
which may be redefined if required, would also provide an alternative way
supplying the right pg_config, at the price of one additional line. Mm.
I think a doc improvement is at least welcome.

--
Fabien.

Dear Tom,

That would break existing Makefiles that use the "please take the first
pg_config in the path" feature, which rather make sense (it just means
that you want the extension for your current postgresql).

How would it break them? The default definition is still PG_CONFIG =
pg_config, this just moves where that definition appears.

I think that I was answering to:

...
Tom> I'm not sure though how to get this setting to
Tom> override the one in Makefile.global ...
Tom> or should we just remove that one?

With the assumption that the above "that one" refered to the "PG_CONFIG"
macro definition in "Makefile.global". As existing extension makefiles do
not defined PG_CONFIG, relying on one would break them wrt future
releases? That's why I suggested to replace the "Makefile.global"
definition by a conditional one.

But it is also entirely possible that I did not fully understand your
sentence:-)

--
Fabien.

Fabien COELHO <coelho@cri.ensmp.fr> writes:

With the assumption that the above "that one" refered to the "PG_CONFIG"
macro definition in "Makefile.global". As existing extension makefiles do
not defined PG_CONFIG, relying on one would break them wrt future
releases?

Ah, I see. I was thinking in terms of breaking them intentionally ;-)
but perhaps that's not such a great idea. The reason that I was
thinking that way was that as long as module Makefiles look like

PGXS := $(shell pg_config --pgxs)
include $(PGXS)

there will be room for people to make the same mistake as Eddie, ie,
try to modify that shell command to select a pg_config that's not
first in $PATH.

If we're worrying about cross-version compatibility then it seems there
isn't any really nice way to make both combinations do the ideal thing.
If someone has an updated module Makefile, ie,

PG_CONFIG := pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)

then it will look like changing PG_CONFIG in the Makefile would work;
but that will not work with an older PG release, because Makefile.global
will override the value. So neither way of writing the module Makefile
is going to be foolproof with both old and new PG installations.

Reading between the lines in the gmake manual, it seems like writing
the module Makefile as

override PG_CONFIG := pg_config
etc

might work to override the setting in old copies of Makefile.global,
but this cure is worse than the disease, because it prevents specifying
PG_CONFIG on the make command line. (And I'm not sure it works anyway;
have not tried it.)

Anyone see a way to handle all these cases at the same time?

regards, tom lane

With the assumption that the above "that one" refered to the "PG_CONFIG"
macro definition in "Makefile.global". As existing extension makefiles do
not defined PG_CONFIG, relying on one would break them wrt future
releases?

Ah, I see. I was thinking in terms of breaking them intentionally ;-)

Well, if that is what you want, is was perfect:-)

But ISTM that the remedy (breaking all past makefiles for all extensions)
is not worth the issue.

A better documentation, and possibly following your suggestion with an
explicit PG_CONFIG in contrib makefiles, but without breaking existing
extensions seems quite enough. The error made by Eddie is legitimate, but
it is also something rare, it did not come up in the last two years.

Please find attached a small patch which enhances the documentation on the
issue in my broken English.

If we're worrying about cross-version compatibility then it seems there
isn't any really nice way to make both combinations do the ideal thing.
If someone has an updated module Makefile, ie,

PG_CONFIG := pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)

then it will look like changing PG_CONFIG in the Makefile would work;
but that will not work with an older PG release, because Makefile.global
will override the value.

Okay, there are indeed two different compatibility issues :
- "old" makefiles with possibly "new" pgxs conventions
- possibly "new" makefiles with "old" pgxs conventions

Indeed for "old" Makefile.global the PG_CONFIG is overwritten.

It would be okay if it is made clear that the PG_CONFIG macro MUST not be
updated from within the Makefile, but just from the commande line ?

Well that's still a little bit error prone anyway.

ISTM that the documentation update would suffice.

--
Fabien.

Fabien COELHO <fabien.coelho@ensmp.fr> writes:

But ISTM that the remedy (breaking all past makefiles for all extensions)
is not worth the issue.
A better documentation, and possibly following your suggestion with an
explicit PG_CONFIG in contrib makefiles, but without breaking existing
extensions seems quite enough. The error made by Eddie is legitimate, but
it is also something rare, it did not come up in the last two years.

True. OK, then let's add the ifndef to Makefile.global and change the
existing extension makefiles to

PG_CONFIG := pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)

Any objections?

regards, tom lane

Let's add the ifndef to Makefile.global and change the
existing extension makefiles to

PG_CONFIG := pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)

Any objections?

Maybe the ":=" for pg_config is not necessary, "=" is fine for a
simple string definition ?

Some documentation (not just code) update seems important to me. The patch
I sent which describes the current status may be applied to existing
branches, and something else can be written for the explicit PG_CONFIG
setting.

Otherwise it looks okay AFAIC.

--
Fabien.

Fabien COELHO <coelho@cri.ensmp.fr> writes:

Some documentation (not just code) update seems important to me.

Agreed. I added this to xfunc.sgml's discussion of PGXS makefiles:

Index: doc/src/sgml/xfunc.sgml
===================================================================
RCS file: /cvsroot/pgsql/doc/src/sgml/xfunc.sgml,v
retrieving revision 1.128
diff -c -r1.128 xfunc.sgml
*** doc/src/sgml/xfunc.sgml	6 Jun 2007 23:00:36 -0000	1.128
--- doc/src/sgml/xfunc.sgml	26 Jun 2007 21:57:43 -0000
***************
*** 2071,2080 ****
  DATA_built = isbn_issn.sql
  DOCS = README.isbn_issn

! PGXS := $(shell pg_config --pgxs)
  include $(PGXS)
  </programlisting>
!     The last two lines should always be the same.  Earlier in the
      file, you assign variables or add custom
      <application>make</application> rules.
     </para>
--- 2071,2081 ----
  DATA_built = isbn_issn.sql
  DOCS = README.isbn_issn

! PG_CONFIG = pg_config
! PGXS := $(shell $(PG_CONFIG) --pgxs)
  include $(PGXS)
  </programlisting>
!     The last three lines should always be the same.  Earlier in the
      file, you assign variables or add custom
      <application>make</application> rules.
     </para>
***************
*** 2215,2220 ****
--- 2216,2233 ----
         </para>
        </listitem>
       </varlistentry>
+ 
+      <varlistentry>
+       <term><varname>PG_CONFIG</varname></term>
+       <listitem>
+        <para>
+         path to <application>pg_config</> program for the
+         <productname>PostgreSQL</productname> installation to build against
+         (typically just <literal>pg_config</> to use the first one in your
+         <varname>PATH</>)
+        </para>
+       </listitem>
+      </varlistentry>
      </variablelist>
     </para>

***************
*** 2222,2234 ****
Put this makefile as <literal>Makefile</literal> in the directory
which holds your extension. Then you can do
<literal>make</literal> to compile, and later <literal>make
! install</literal> to install your module. The extension is
compiled and installed for the
<productname>PostgreSQL</productname> installation that
! corresponds to the first <command>pg_config</command> command
! found in your path.
</para>

     <para>
      The scripts listed in the <varname>REGRESS</> variable are used for
      regression testing of your module, just like <literal>make
--- 2235,2260 ----
      Put this makefile as <literal>Makefile</literal> in the directory
      which holds your extension. Then you can do
      <literal>make</literal> to compile, and later <literal>make
!     install</literal> to install your module.  By default, the extension is
      compiled and installed for the
      <productname>PostgreSQL</productname> installation that
!     corresponds to the first <command>pg_config</command> program
!     found in your path.  You can use a different installation by
!     setting <varname>PG_CONFIG</varname> to point to its
!     <command>pg_config</command> program, either within the makefile
!     or on the <literal>make</literal> command line.
     </para>

+    <caution>
+     <para>
+      Changing <varname>PG_CONFIG</varname> only works when building
+      against <productname>PostgreSQL</productname> 8.3 or later.
+      With older releases it does not work to set it to anything except
+      <literal>pg_config</>; you must alter your <varname>PATH</>
+      to select the installation to build against.
+     </para>
+    </caution>
+ 
     <para>
      The scripts listed in the <varname>REGRESS</> variable are used for
      regression testing of your module, just like <literal>make

It might be worth backpatching the Makefile.global.in patch (ie, the
ifndef addition) to the 8.2 branch, which would allow us to say "8.2.5
or later" instead of "8.3 or later", and would bring correspondingly
nearer the time when people can actually use the feature without
thinking much. Comments?

regards, tom lane

It might be worth backpatching the Makefile.global.in patch (ie, the
ifndef addition) to the 8.2 branch, which would allow us to say "8.2.5
or later" instead of "8.3 or later", and would bring correspondingly
nearer the time when people can actually use the feature without
thinking much. Comments?

My 2 euro cents (about $0.027) conservative comments on the subject:

- It is more work for a minor issue. I would just update the doc for
previous releases.
- New features belong to new releases, on principles. It is not really a
bug which would require an update of previous releases, and changing the
PATH is a valid workaround anyway.
- Do it your way:-)

--
Fabien.

Am Dienstag, 26. Juni 2007 16:12 schrieb Tom Lane:

True. �OK, then let's add the ifndef to Makefile.global and change the
existing extension makefiles to

��������PG_CONFIG := pg_config
��������PGXS := $(shell $(PG_CONFIG) --pgxs)
��������include $(PGXS)

Any objections?

Yes. I think that solution is wrong. It merely creates other possibilities
to use mismatching combinations. What was the problem with just making all
uses of pg_config in Makefile.global use a hardcoded bindir directly?

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Dear Peter,

What was the problem with just making all uses of pg_config in
Makefile.global use a hardcoded bindir directly?

Because bindir is given by pg_config:-)

ISTM that the underlying issue, which was not foreseen in the initial pgxs
and fixed later, is that some distributions use a different installation
prefix at compile time and once the software is actually installed. You
would end up with a /tmp/build/.. path which does not exist. If the
installations are movable, you can only rely on pg_config.

--
Fabien.

Fabien COELHO <coelho@cri.ensmp.fr> writes:

What was the problem with just making all uses of pg_config in
Makefile.global use a hardcoded bindir directly?

Because bindir is given by pg_config:-)

ISTM that the underlying issue, which was not foreseen in the initial pgxs
and fixed later, is that some distributions use a different installation
prefix at compile time and once the software is actually installed.

Right, the installation tree is supposed to be relocatable. Otherwise
we would not need to use pg_config to find the paths at all; we'd just
hardwire all of them when constructing Makefile.global.

regards, tom lane

Peter Eisentraut <peter_e@gmx.net> writes:

Am Dienstag, 26. Juni 2007 16:12 schrieb Tom Lane:

PG_CONFIG := pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)

Any objections?

Yes. I think that solution is wrong. It merely creates other possibilities
to use mismatching combinations.

Well, it's certainly *possible* to screw it up, but the idea is that the
"obvious" way of putting in a path will work; whereas before the obvious
way did not work. So I think it's a step forward.

regards, tom lane