Bugfix and new feature for PGXS

On Tue, 2013-06-18 at 15:52 +0200, Cédric Villemain wrote:

This allows for example to install hstore header and be able to
include them
in another extension like that:

# include "contrib/hstore/hstore.h"

That's not going to work. hstore's header file is included as #include
"hstore.h" (cf. hstore source code). Having it included under different
paths in different contexts will be a mess.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Le mercredi 19 juin 2013 04:48:11, Peter Eisentraut a écrit :

On Tue, 2013-06-18 at 15:52 +0200, Cédric Villemain wrote:

This allows for example to install hstore header and be able to
include them

in another extension like that:
# include "contrib/hstore/hstore.h"

That's not going to work. hstore's header file is included as #include
"hstore.h" (cf. hstore source code). Having it included under different
paths in different contexts will be a mess.

OK.
At the beginning I though of putting headers in include/contrib but feared
that some header may have the same name.
If you think that it is suitable, I can do that ? (and include the clean:
recipe that I missed on the first shot)

Also, do we want to keep the word 'contrib' ? include/extension looks fine
too...

--
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation

On 6/19/13 5:55 AM, Cédric Villemain wrote:

Le mercredi 19 juin 2013 04:48:11, Peter Eisentraut a écrit :

On Tue, 2013-06-18 at 15:52 +0200, Cédric Villemain wrote:

This allows for example to install hstore header and be able to
include them

in another extension like that:
# include "contrib/hstore/hstore.h"

That's not going to work. hstore's header file is included as #include
"hstore.h" (cf. hstore source code). Having it included under different
paths in different contexts will be a mess.

OK.
At the beginning I though of putting headers in include/contrib but feared
that some header may have the same name.
If you think that it is suitable, I can do that ? (and include the clean:
recipe that I missed on the first shot)

I don't think there is any value in moving the contrib header files around.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 06/19/2013 10:06 AM, Peter Eisentraut wrote:

On 6/19/13 5:55 AM, Cédric Villemain wrote:

Le mercredi 19 juin 2013 04:48:11, Peter Eisentraut a écrit :

On Tue, 2013-06-18 at 15:52 +0200, Cédric Villemain wrote:

This allows for example to install hstore header and be able to
include them

in another extension like that:
# include "contrib/hstore/hstore.h"

That's not going to work. hstore's header file is included as #include
"hstore.h" (cf. hstore source code). Having it included under different
paths in different contexts will be a mess.

OK.
At the beginning I though of putting headers in include/contrib but feared
that some header may have the same name.
If you think that it is suitable, I can do that ? (and include the clean:
recipe that I missed on the first shot)

I don't think there is any value in moving the contrib header files around.

What are they going to be used for anyway? I rubbed up against this not
too long ago. Things will blow up if you use stuff from the module and
the module isn't already loaded.

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 6/19/13 10:19 AM, Andrew Dunstan wrote:

What are they going to be used for anyway? I rubbed up against this not
too long ago. Things will blow up if you use stuff from the module and
the module isn't already loaded.

See transforms.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 06/19/2013 11:26 AM, Peter Eisentraut wrote:

On 6/19/13 10:19 AM, Andrew Dunstan wrote:

What are they going to be used for anyway? I rubbed up against this not
too long ago. Things will blow up if you use stuff from the module and
the module isn't already loaded.

See transforms.

So you're saying to install extension headers, but into the main
directory where we put server headers?

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Le mercredi 19 juin 2013 18:20:11, Andrew Dunstan a écrit :

On 06/19/2013 11:26 AM, Peter Eisentraut wrote:

On 6/19/13 10:19 AM, Andrew Dunstan wrote:

What are they going to be used for anyway? I rubbed up against this not
too long ago. Things will blow up if you use stuff from the module and
the module isn't already loaded.

See transforms.

So you're saying to install extension headers, but into the main
directory where we put server headers?

At the same level than server headers, but I'm open for suggestion.

$ tree -d include
include
├── libpq
└── postgresql
├── contrib
│ └── hstore
├── informix
│ └── esql
├── internal
│ └── libpq
└── server

And all subidrs of server/

--
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation

On 06/19/2013 12:32 PM, Cédric Villemain wrote:

Le mercredi 19 juin 2013 18:20:11, Andrew Dunstan a écrit :

On 06/19/2013 11:26 AM, Peter Eisentraut wrote:

On 6/19/13 10:19 AM, Andrew Dunstan wrote:

What are they going to be used for anyway? I rubbed up against this not
too long ago. Things will blow up if you use stuff from the module and
the module isn't already loaded.

See transforms.

So you're saying to install extension headers, but into the main
directory where we put server headers?

At the same level than server headers, but I'm open for suggestion.

$ tree -d include
include
├── libpq
└── postgresql
├── contrib
│ └── hstore
├── informix
│ └── esql
├── internal
│ └── libpq
└── server

And all subidrs of server/

This is what Peter was arguing against, isn't it?

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 6/19/13 12:20 PM, Andrew Dunstan wrote:

So you're saying to install extension headers, but into the main
directory where we put server headers?

Yes, if we choose to install some extension headers, that is where we
should put them.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Le mercredi 19 juin 2013 18:48:21, Andrew Dunstan a écrit :

On 06/19/2013 12:32 PM, Cédric Villemain wrote:

Le mercredi 19 juin 2013 18:20:11, Andrew Dunstan a écrit :

On 06/19/2013 11:26 AM, Peter Eisentraut wrote:

On 6/19/13 10:19 AM, Andrew Dunstan wrote:

What are they going to be used for anyway? I rubbed up against this
not too long ago. Things will blow up if you use stuff from the
module and the module isn't already loaded.

See transforms.

So you're saying to install extension headers, but into the main
directory where we put server headers?

At the same level than server headers, but I'm open for suggestion.

$ tree -d include
include
├── libpq
└── postgresql

├── contrib
│ └── hstore
├── informix
│ └── esql
├── internal
│ └── libpq
└── server

And all subidrs of server/

This is what Peter was arguing against, isn't it?

Now I have a doubt :)
I believe he answered the proposal to put all headers on the same flat
directory, instead of a tree.
--
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation

Peter Eisentraut wrote:

On 6/19/13 12:20 PM, Andrew Dunstan wrote:

So you're saying to install extension headers, but into the main
directory where we put server headers?

Yes, if we choose to install some extension headers, that is where we
should put them.

The question of the name of the directory still stands. "contrib" would
be the easiest answer, but it's slightly wrong because
externally-supplied modules could also want to install headers.
"extension" might be it, but there are things that aren't extensions
(right? if not, that would be my choice).

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Le mercredi 19 juin 2013 21:06:23, Alvaro Herrera a écrit :

Peter Eisentraut wrote:

On 6/19/13 12:20 PM, Andrew Dunstan wrote:

So you're saying to install extension headers, but into the main
directory where we put server headers?

Yes, if we choose to install some extension headers, that is where we
should put them.

The question of the name of the directory still stands. "contrib" would
be the easiest answer, but it's slightly wrong because
externally-supplied modules could also want to install headers.
"extension" might be it, but there are things that aren't extensions
(right? if not, that would be my choice).

yes, I think the same.
auth_delay for example is not an extension as in CREATE EXTENSION. So...it is
probably better to postpone this decision and keep on the idea to just install
headers where there should be will traditional name (contrib).

--
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation

On Wed, 2013-06-19 at 20:58 +0200, Cédric Villemain wrote:

I believe he answered the proposal to put all headers on the same flat
directory, instead of a tree.

The headers are used as

#include "hstore.h"
#include "ltree.h"
etc.

in the existing source code.

If you want to install the for use by others, you can do one of three
things:

1) Install them into $(pg_config --includedir-server), so other users
will just include them in the same way as shown above.

2) Install them in a different directory, but keep the same #include
lines. That would require PGXS changes, perhaps a new pg_config option,
or something that produces the right -I option to find them.

3) Install them in a different directory and require a different
#include line. But then you have to change the existing uses as well,
which would probably require moving files around.

Both 2 and 3 require a lot of work, possibly compatibility breaks, for
no obvious reason.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 06/20/2013 11:26 AM, Peter Eisentraut wrote:

3) Install them in a different directory and require a different
#include line. But then you have to change the existing uses as well,
which would probably require moving files around.

If I understand you correctly, your concern seems to be with referring
to the extensions headers within that extension. For example, a pgxs
build of hstore using "contrib/hstore/hstore.h" wouldn't work if
"hstore.h" was in the current location, the root of the hstore contrib
module. It'd be fine for a non-pgxs build, since we'd just add
$(top_srcdir) to the include path when building contrib modules in-tree.

So, for pgxs, we'd have to construct a temporary include dir, copying
hstore.h into a temporary 'contrib/hstore' subdir for the build. Which
is messy, but would work, and would confine the change mostly to pgxs.
Or we'd have to move it there permanently in the source tree, which
seems kind of excessive. There's a certain appeal in having extensions
follow the same model as the main server code:

hstore/
src/
contrib
hstore
crc32.c hstore_compat.c hstore_gin.c hstore_gist.c
hstore_io.c hstore_op.c
include
contrib
hstore
hstore.h

... but that's pretty significant disruption for something I was hoping
would be a rather small change.

As you note, the other option is to just refer to extension headers by
their unqualified name. I'm *really* not a fan of that idea due to the
obvious clash possibilities with Pg's own headers or system headers,
especially given that the whole idea of extensions is that they're user
supplied. Not having any kind of namespacing is worrying. What could
possibly work would be to install extension headers in
contrib/[modulename]/ and automatically have pgxs and the in-tree build
add appropriate -I directives to those subdirs based on dependencies
declared in the Makefile. Again, though, that makes the whole thing a
lot more complex than I'd like.

Drat.

A final option: When building the extension its self, use "hstore.h".
When referring to it from elsewhere, use "contrib/hstore/hstore.h". In
pgxs's case it'd be installed, in an in-tree build it'd be handled by
adding ${top_srcdir} to the include path when we're building contribs.

Opinions all?

* Give up on being able to use one extension's header from another
entirely, except in the special case of in-tree builds

* Install install-enabled contrib headers into an include/contrib/
that's always on the pgxs include path, so you can still just "#include
hstore.h" . For in-tree builds, explicit add other modules' contrib dirs
as Peter has done in the proposed plperl_hstore and plpython_hstore.
(Use unqualified names).

* Install install-enabled contrib headers to
include/contrib/[modulename]/ . Within the module, use the unqualified
header name. When referring to it from outside the module, use #include
"contrib/modulename/header.h". Add $(top_srcdir) to the include path
when building extensions in-tree.

Personally, I'm all for just using the local path when building the
module, and the qualified path elsewhere. It requires only a relatively
simple change, and I don't think that using "hstore.h" within hstore,
and "contrib/hstore/hstore.h" elsewhere is of great concern.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Le jeudi 20 juin 2013 05:26:21, Peter Eisentraut a écrit :

On Wed, 2013-06-19 at 20:58 +0200, Cédric Villemain wrote:

I believe he answered the proposal to put all headers on the same flat
directory, instead of a tree.

The headers are used as

#include "hstore.h"
#include "ltree.h"
etc.

in the existing source code.

If you want to install the for use by others, you can do one of three
things:

1) Install them into $(pg_config --includedir-server), so other users
will just include them in the same way as shown above.

I don't like this one because extension can overwrite sever headers.

2) Install them in a different directory, but keep the same #include
lines. That would require PGXS changes, perhaps a new pg_config option,
or something that produces the right -I option to find them.

Patch of PGXS is not a problem.
pg_config patch is a requirement only if users set their own
$(includedir_contrib) variable. I didn't though about it, but it is probably
better to add that. This looks trivial too.

To include hstore header we write «#include "hstore.h"» but we add :
-I$(includedir_contrib)/hstore to the FLAGS in the extension makefile which
does require hstore. It changes nothing to hstore Makefile itself.

The main difference is that before we had to -I$(top_srcdir)/../contrib/hstore
*iff* we are not building with PGXS (else we need to have the hstore source
available somewhere), now we can do -I$(includedir_contrib)/hstore with PGXS
(hstore need to be installed first, as we USE_PGXS)

Then PGXS offers to catch both case transparently so we can do :
-I${include_contrib_header) in Makefile
and pgxs.mk takes care of adjusting include_contrib_header (or whatever its
name) according to USE_PGXS value.

3) Install them in a different directory and require a different
#include line. But then you have to change the existing uses as well,
which would probably require moving files around.

Having to change existing source code do keep the same behavior is not
attractive at all.

Both 2 and 3 require a lot of work, possibly compatibility breaks, for
no obvious reason.

2 is a good solution and not a lot of work, IMO.

Removing the need of setting -I$(include...) in the contrib Makefile can be
done later by adding a PGXS variable to define the contrib requirements, this
is something different from the current feature (build contrib depending on
another(s) without full source tree)
--
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation

Opinions all?

* Give up on being able to use one extension's header from another
entirely, except in the special case of in-tree builds

There are already a good number of use cases with only hstore and intarray,
I'm in favor of this feature.

* Install install-enabled contrib headers into an include/contrib/
that's always on the pgxs include path, so you can still just "#include
hstore.h" . For in-tree builds, explicit add other modules' contrib dirs
as Peter has done in the proposed plperl_hstore and plpython_hstore.
(Use unqualified names).

I don't like the idea to share a flat directory with different header files,
filenames can overlap.

* Install install-enabled contrib headers to
include/contrib/[modulename]/ . Within the module, use the unqualified
header name. When referring to it from outside the module, use #include
"contrib/modulename/header.h". Add $(top_srcdir) to the include path
when building extensions in-tree.

not either, see my other mail. we still #include hstore.h when we want, we
just add that the header so it is available for PGXS builds. Contrib Makefile
still need to -I$(includedir_contrib)/hstore. What's new is that the header is
installed, not that we don't have to set FLAGS.

Personally, I'm all for just using the local path when building the
module, and the qualified path elsewhere. It requires only a relatively
simple change, and I don't think that using "hstore.h" within hstore,
and "contrib/hstore/hstore.h" elsewhere is of great concern.

--
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation

On 6/20/13 1:21 AM, Craig Ringer wrote:

As you note, the other option is to just refer to extension headers by
their unqualified name. I'm *really* not a fan of that idea due to the
obvious clash possibilities with Pg's own headers or system headers,
especially given that the whole idea of extensions is that they're user
supplied. Not having any kind of namespacing is worrying.

We already install all PostgreSQL server header files into a separate
directory, so the only clashes you have to worry about are with other
extensions and with the backend. And you have to do that anyway,
because you will have namespace issues for the shared libraries, the
symbols in those libraries, and the extension names.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 6/20/13 1:21 AM, Craig Ringer wrote:

Personally, I'm all for just using the local path when building the
module, and the qualified path elsewhere. It requires only a relatively
simple change, and I don't think that using "hstore.h" within hstore,
and "contrib/hstore/hstore.h" elsewhere is of great concern.

It doesn't work if those header files include other header files (as in
the case of plpython, for example).

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 06/18/2013 09:52 AM, Cédric Villemain wrote:

I've rebased the current set of pending patches I had, to fix pgxs and added 2
new patches.

Bugfixes have already been presented and sent in another thread, except the
last one which only fix comment in pgxs.mk.

The new feature consists in a new variable to allow the installation of
contrib header files.
A new variable MODULEHEADER can be used in extension Makefile to list the
header files to install.

The installation path default to $includedir/$includedir_contrib/$extension.
2 new variables are set to define directories: $includedir_contrib and
$includedir_extension, the former default to include/contrib and the later to
$includedir_contrib/$extension ($extension is the name of the extension).

This allows for example to install hstore header and be able to include them
in another extension like that:

# include "contrib/hstore/hstore.h"

For packagers of PostgreSQL, this allows to have a postgresql-X.Y-contrib-dev
package.
For developers of extension it's killing the copy-and-paste-this-function
dance previously required.

I've not updated contribs' Makfefile: I'm not sure what we want to expose.

I've 2 other patches to write to automatize a bit more the detection of things
to do when building with USE_PGXS, based on the layout. Better get a good
consensus on this before writing them.

Bugfix:
0001-fix-SHLIB_PREREQS-when-building-with-USE_PGXS.patch
0002-Create-data-directory-if-extension-when-required.patch
0003-set-VPATH-for-out-of-tree-extension-builds.patch
0004-adds-support-for-VPATH-with-USE_PGXS.patch
0006-Fix-suggested-layout-for-extension.patch

New feature:
0005-Allows-extensions-to-install-header-file.patch

I'll do a documentation patch based on what is accepted in HEAD and/or
previous branches.

I have just spent an hour or two wrestling with the first four of these.
Items 5 and six are really separate items, which I'm not considering at
the moment.

The trouble I have is that there are no instructions on how to modify
your Makefile or prepare a vpath tree, so I have been flying blind. I
started out doing what Postgres does, namely to prepare the tree using
config/prep_buildtree. This doesn't work at all - the paths don't get
set properly unless you invoke make with the path to the real makefile,
and I'm not sure they all get set correctly then. But that's not what we
expect of a vpath setup, or at least not what I expect. I expect that
with an appropriately set up make file and a correctly set up build tree
I can use an identical make invocation to the one I would use for an
in-tree build. That is, after setting up I should be able to ignore the
fact that it's a vpath build.

FYI I deliberately chose a non-core extension with an uncommon layout
for testing: json_build <https://github.com/pgexperts/json_build&gt;

So, patches 1, 2 and 6 look sane enough, and I'm inclined to commit them
just to get them out of the way. That means two of the commitfest items
I am signed up for will be committed and two marked "Returned with
comments". I think we need some more discussion about how VPATH builds
should work with extensions, and subject to what limitations if any.

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers