Обсуждение: Frustrating issue with PGXS

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
?


Eddie

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

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

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

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_configPGXS := $(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
PATHis 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