Horton

Thread

Commits

  1. Fix out-of-tree build for transform modules.

  2. Provide for contrib and pgxs modules to install include files.

  1. Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-07-01T18:23:03Z

    So I have this immediate problem: a PGXS build of a module, specifically
an hstore transform for a non-core PL, is much harder than it should be
because it has no way to get at hstore.h since that file is never
installed anywhere.

Should that be changed?

-- 
Andrew (irc:RhodiumToad)

  2. Re: Should contrib modules install .h files?

    Andres Freund <andres@anarazel.de> — 2018-07-01T18:30:00Z

    Hi,

On 2018-07-01 19:23:03 +0100, Andrew Gierth wrote:
> So I have this immediate problem: a PGXS build of a module, specifically
> an hstore transform for a non-core PL, is much harder than it should be
> because it has no way to get at hstore.h since that file is never
> installed anywhere.
> 
> Should that be changed?

I've hit this before, and the more capable our extension framework and
more complex individual extensions get, the more we'll hit this. One
question is where to install them - the extensions with headers often
just have them in the source code itself, which isn't a great
solution. contrib/$extension/ or $extension/ both seems ok, but i'd be
somewhat against just $extension.h

Greetings,

Andres Freund

  3. Re: Should contrib modules install .h files?

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-07-02T08:42:21Z

    On 01.07.18 20:23, Andrew Gierth wrote:
> So I have this immediate problem: a PGXS build of a module, specifically
> an hstore transform for a non-core PL, is much harder than it should be
> because it has no way to get at hstore.h since that file is never
> installed anywhere.
> 
> Should that be changed?

Yes, just install it, I think.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  4. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-07-02T10:04:14Z

    >>>>> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

 >> So I have this immediate problem: a PGXS build of a module,
 >> specifically an hstore transform for a non-core PL, is much harder
 >> than it should be because it has no way to get at hstore.h since
 >> that file is never installed anywhere.
 >> 
 >> Should that be changed?

 Peter> Yes, just install it, I think.

I'm thinking that $(includedir_server)/$(MODULEDIR) would be a
reasonable place? MODULEDIR defaults to either "contrib" or "extension"
depending on whether EXTENSION is set.

Something like the attached patch seem reasonable?

-- 
Andrew (irc:RhodiumToad)

  5. Re: Should contrib modules install .h files?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-02T13:26:30Z

    Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> I'm thinking that $(includedir_server)/$(MODULEDIR) would be a
> reasonable place? MODULEDIR defaults to either "contrib" or "extension"
> depending on whether EXTENSION is set.
> Something like the attached patch seem reasonable?

FWIW, I agree with Andres' thought that each contrib module should have
its own subdirectory under $(includedir_server).  Otherwise we're going
to be faced with questions about whether .h files need to be renamed
because they're not globally unique enough.  There are already some that
are pretty shaky from this standpoint:

contrib$ ls */*.h
bloom/bloom.h                   pg_trgm/trgm.h
btree_gist/btree_gist.h         pgcrypto/blf.h
btree_gist/btree_utils_num.h    pgcrypto/imath.h
btree_gist/btree_utils_var.h    pgcrypto/mbuf.h
cube/cubedata.h                 pgcrypto/md5.h
hstore/hstore.h                 pgcrypto/pgcrypto.h
intarray/_int.h                 pgcrypto/pgp.h
isn/EAN13.h                     pgcrypto/px-crypt.h
isn/ISBN.h                      pgcrypto/px.h
isn/ISMN.h                      pgcrypto/rijndael.h
isn/ISSN.h                      pgcrypto/sha1.h
isn/UPC.h                       postgres_fdw/postgres_fdw.h
isn/isn.h                       seg/segdata.h
ltree/crc32.h                   sepgsql/sepgsql.h
ltree/ltree.h                   tablefunc/tablefunc.h
pageinspect/pageinspect.h

Not sure about whether the MODULEDIR part is useful.  Seems like
making a distinction between extensions and other contrib is
likely to create more headaches than it avoids.

BTW, it's somewhat interesting to think about whether we ought to
change the coding conventions so that extensions refer to their
own headers with a subdirectory, e.g., #include "bloom/bloom.h".
Having done that, all of contrib could build with a single
centrally-provided -I switch pointing at BUILDDIR/contrib/,
and there would be a path to allowing the code to build out of
tree by pointing that common -I at $(includedir_server)/ or
$(includedir_server)/MODULEDIR.  This seems like it could be
a lot less messy as we accrete more cross-module references.

			regards, tom lane

  6. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-07-02T14:10:42Z

    >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 > Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
 >> I'm thinking that $(includedir_server)/$(MODULEDIR) would be a
 >> reasonable place? MODULEDIR defaults to either "contrib" or
 >> "extension" depending on whether EXTENSION is set. Something like
 >> the attached patch seem reasonable?

 Tom> FWIW, I agree with Andres' thought that each contrib module should
 Tom> have its own subdirectory under $(includedir_server). Otherwise
 Tom> we're going to be faced with questions about whether .h files need
 Tom> to be renamed because they're not globally unique enough. There
 Tom> are already some that are pretty shaky from this standpoint:

I'm not suggesting that all modules should install a .h file or that all
of a module's .h files should be installed.

A slight snag in trying to use a subdir for each module is that there is
not in fact anywhere in the existing makefiles that uses or assigns such
a name. Indeed some contrib subdirs install multiple modules.

 Tom> Not sure about whether the MODULEDIR part is useful. Seems like
 Tom> making a distinction between extensions and other contrib is
 Tom> likely to create more headaches than it avoids.

Sure, but that's just copied from DATA and DOCS which already do it that
way. For DATA there seems some justification based on CREATE EXTENSION,
but for docs?

-- 
Andrew (irc:RhodiumToad)

  7. Re: Should contrib modules install .h files?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-02T14:16:22Z

    Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>  Tom> FWIW, I agree with Andres' thought that each contrib module should
>  Tom> have its own subdirectory under $(includedir_server). Otherwise
>  Tom> we're going to be faced with questions about whether .h files need
>  Tom> to be renamed because they're not globally unique enough. There
>  Tom> are already some that are pretty shaky from this standpoint:

> I'm not suggesting that all modules should install a .h file or that all
> of a module's .h files should be installed.

I agree with that, which implies the need for a new macro comparable to
DATA and DOCS that lists the .h files to be installed.

> A slight snag in trying to use a subdir for each module is that there is
> not in fact anywhere in the existing makefiles that uses or assigns such
> a name. Indeed some contrib subdirs install multiple modules.

So, given that we have to add something to the module makefiles anyway,
we could also add a macro specifying the subdirectory name to use.
(Although in practice this should always be equal to the contrib/
subdirectory name, so maybe we could extract it on that basis?)

			regards, tom lane

  8. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-07-02T15:11:07Z

    >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 >> A slight snag in trying to use a subdir for each module is that
 >> there is not in fact anywhere in the existing makefiles that uses or
 >> assigns such a name. Indeed some contrib subdirs install multiple
 >> modules.

 Tom> So, given that we have to add something to the module makefiles
 Tom> anyway, we could also add a macro specifying the subdirectory name
 Tom> to use. (Although in practice this should always be equal to the
 Tom> contrib/ subdirectory name, so maybe we could extract it on that
 Tom> basis?)

Using the subdir name may work for in-tree contrib/ builds but it's not
so good for PGXS, which should not be making assumptions about the build
directory name.

How about this: it's most likely that modules that install include files
will also be using MODULE_big, so use that as the default name; if a
makefile that uses only MODULES also wants to install include files,
have it define MODULE_NAME (or some such variable) itself.

-- 
Andrew (irc:RhodiumToad)

  9. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-07-02T15:15:56Z

    >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 Tom> BTW, it's somewhat interesting to think about whether we ought to
 Tom> change the coding conventions so that extensions refer to their
 Tom> own headers with a subdirectory, e.g., #include "bloom/bloom.h".
 Tom> Having done that, all of contrib could build with a single
 Tom> centrally-provided -I switch pointing at BUILDDIR/contrib/, and
 Tom> there would be a path to allowing the code to build out of tree by
 Tom> pointing that common -I at $(includedir_server)/ or
 Tom> $(includedir_server)/MODULEDIR. This seems like it could be a lot
 Tom> less messy as we accrete more cross-module references.

I'm slightly skeptical of this because it could cause unexpected issues
when you rebuild (especially in the PGXS case) a module that has already
been installed; without care, you'd end up getting the module's own
headers from the installed version rather than the one being built,
which would be very bad.

-- 
Andrew (irc:RhodiumToad)

  10. Re: Should contrib modules install .h files?

    Andres Freund <andres@anarazel.de> — 2018-07-02T15:19:02Z

    On 2018-07-02 16:11:07 +0100, Andrew Gierth wrote:
> >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> 
>  >> A slight snag in trying to use a subdir for each module is that
>  >> there is not in fact anywhere in the existing makefiles that uses or
>  >> assigns such a name. Indeed some contrib subdirs install multiple
>  >> modules.
> 
>  Tom> So, given that we have to add something to the module makefiles
>  Tom> anyway, we could also add a macro specifying the subdirectory name
>  Tom> to use. (Although in practice this should always be equal to the
>  Tom> contrib/ subdirectory name, so maybe we could extract it on that
>  Tom> basis?)
> 
> Using the subdir name may work for in-tree contrib/ builds but it's not
> so good for PGXS, which should not be making assumptions about the build
> directory name.
> 
> How about this: it's most likely that modules that install include files
> will also be using MODULE_big, so use that as the default name; if a
> makefile that uses only MODULES also wants to install include files,
> have it define MODULE_NAME (or some such variable) itself.

For LLVM bitcode generation I made it so MODULE_big uses that as the
directory name, whereas MODULES just iterates over the components. Now
for the bitcode there was less need to make it selective, so it's a bit
easier.  We could just define that HEADERS_$component is referenced for
each component in MODULES or something like that?

Greetings,

Andres Freund

  11. Re: Should contrib modules install .h files?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-02T15:22:25Z

    Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>  Tom> So, given that we have to add something to the module makefiles
>  Tom> anyway, we could also add a macro specifying the subdirectory name
>  Tom> to use. (Although in practice this should always be equal to the
>  Tom> contrib/ subdirectory name, so maybe we could extract it on that
>  Tom> basis?)

> Using the subdir name may work for in-tree contrib/ builds but it's not
> so good for PGXS, which should not be making assumptions about the build
> directory name.

Fair point.

> How about this: it's most likely that modules that install include files
> will also be using MODULE_big, so use that as the default name; if a
> makefile that uses only MODULES also wants to install include files,
> have it define MODULE_NAME (or some such variable) itself.

AFAIR, you're supposed to define at most one of those macros anyway,
so I don't see why it couldn't be like "use MODULE_big if set, else
use MODULE if set, else fail if MODULE_NAME isn't set".

			regards, tom lane

  12. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-07-02T18:23:25Z

    >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 >> How about this: it's most likely that modules that install include
 >> files will also be using MODULE_big, so use that as the default
 >> name; if a makefile that uses only MODULES also wants to install
 >> include files, have it define MODULE_NAME (or some such variable)
 >> itself.

 Tom> AFAIR, you're supposed to define at most one of those macros
 Tom> anyway, so I don't see why it couldn't be like "use MODULE_big if
 Tom> set, else use MODULE if set, else fail if MODULE_NAME isn't set".

OK, I'm working on an updated patch, that will allow this:

if using MODULE_big:

MODULE_big = mymodule
HEADERS = whatever.h

# gets installed as mymodule/whatever.h  in whatever dir we decide on
# also works if you define HEADERS_mymodule = whatever.h

if not using MODULE_big:

MODULES = foo bar baz
HEADERS_foo = foo.h
HEADERS_bar = bar.h
# baz doesn't have any headers

# foo.h installed as foo/foo.h
# bar.h installed as bar/bar.h

Two questions arise:

1) include/server has a lot of files and subdirs, so using
   include/server/$(MODULE)/ looks likely to be error-prone. So it
   should be something like include/server/contrib/$(MODULE)/ or
   include/server/extension/$(MODULE)/. Which one, or should it use
   $(MODULEDIR) to choose between the two the way that DATA and DOCS do?
   Or something else?

2) Specifying HEADERS_blah for some name "blah" that's not listed in
   MODULES or MODULE_big should do what:

  a) install into blah/ anyway
  b) be ignored with a warning
  c) be silently ignored
  d) be an error

-- 
Andrew (irc:RhodiumToad)

  13. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-07-02T20:27:42Z

    >>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:

 Andrew> OK, I'm working on an updated patch

and here it is.

This installs to $(includedir_server)/$(MODULEDIR)/$MODULE/file.h
(e.g. include/server/extension/hstore/hstore.h for an actual example),
and errors if HEADERS_xxx is defined for anything that's not a module
listed in MODULES or MODULE_big.

-- 
Andrew (irc:RhodiumToad)

  14. Re: Should contrib modules install .h files?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-02T20:46:04Z

    Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> Two questions arise:

> 1) include/server has a lot of files and subdirs, so using
>    include/server/$(MODULE)/ looks likely to be error-prone. So it
>    should be something like include/server/contrib/$(MODULE)/ or
>    include/server/extension/$(MODULE)/. Which one, or should it use
>    $(MODULEDIR) to choose between the two the way that DATA and DOCS do?
>    Or something else?

Might as well follow the MODULEDIR precedent (though I'm not wedded
to that if somebody has an argument for something else).

> 2) Specifying HEADERS_blah for some name "blah" that's not listed in
>    MODULES or MODULE_big should do what:
>   a) install into blah/ anyway
>   b) be ignored with a warning
>   c) be silently ignored
>   d) be an error

I'd definitely vote for "error".  Likewise if any .h file listed in
the macro doesn't exist.

			regards, tom lane

  15. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-07-02T21:05:27Z

    >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 Tom> Might as well follow the MODULEDIR precedent (though I'm not wedded
 Tom> to that if somebody has an argument for something else).
 [...]
 Tom> I'd definitely vote for "error".  Likewise if any .h file listed in
 Tom> the macro doesn't exist.

OK, so that matches the patch I just posted. If any .h file listed
doesn't exist, then $(INSTALL_DATA) should fail (and a quick test seems
to confirm that it does).

I only added one HEADERS= entry, for contrib/hstore; the other modules
should also be reviewed to see if they should install any headers, but
the basic stuff should be nailed down first.

-- 
Andrew (irc:RhodiumToad)

  16. Re: Should contrib modules install .h files?

    Craig Ringer <craig@2ndquadrant.com> — 2018-07-03T04:43:50Z

    On 2 July 2018 at 02:23, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote:

> So I have this immediate problem: a PGXS build of a module, specifically
> an hstore transform for a non-core PL, is much harder than it should be
> because it has no way to get at hstore.h since that file is never
> installed anywhere.
>
> Should that be changed?
>
>
I think there's agreement in the thread that it should, and strong +1 from
me.

I just wanted to pipe up with something Petr pointed out during pglogical
development, which is that Pg offers a handy tool to help extensions link
up with each other - find_rendezvous_variable(...) from dfmgr.c / fmgr.h .

It's a real shame it's not more visible in contrib/ examples and the docs.
Any suggestions on where it should appear in the docs? Somewhere in
extend.sgml, presumably.

You still need a header from the other extension to *use* it, but it
provides a massively easier way to find a struct of API function pointers.
Prior to using it, I had a hack where I dlopen()ed the other shared library
directly, and I'd also trialled using the fmgr to call a 'returns internal'
function to get the API pointers struct that way.

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

  17. Re: Should contrib modules install .h files?

    Pavel Stehule <pavel.stehule@gmail.com> — 2018-07-03T06:04:23Z

    2018-07-03 6:43 GMT+02:00 Craig Ringer <craig@2ndquadrant.com>:

> On 2 July 2018 at 02:23, Andrew Gierth <andrew@tao11.riddles.org.uk>
> wrote:
>
>> So I have this immediate problem: a PGXS build of a module, specifically
>> an hstore transform for a non-core PL, is much harder than it should be
>> because it has no way to get at hstore.h since that file is never
>> installed anywhere.
>>
>> Should that be changed?
>>
>>
> I think there's agreement in the thread that it should, and strong +1 from
> me.
>

+1

similar issue is with plpgsql. The header files are exported by not generic
way.

Regards

Pavel


> I just wanted to pipe up with something Petr pointed out during pglogical
> development, which is that Pg offers a handy tool to help extensions link
> up with each other - find_rendezvous_variable(...) from dfmgr.c / fmgr.h
> .
>
> It's a real shame it's not more visible in contrib/ examples and the docs.
> Any suggestions on where it should appear in the docs? Somewhere in
> extend.sgml, presumably.
>
> You still need a header from the other extension to *use* it, but it
> provides a massively easier way to find a struct of API function pointers.
> Prior to using it, I had a hack where I dlopen()ed the other shared library
> directly, and I'd also trialled using the fmgr to call a 'returns internal'
> function to get the API pointers struct that way.
>
> --
>  Craig Ringer                   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services
>
>

  18. Re: Should contrib modules install .h files?

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-07-03T07:29:55Z

    On 02.07.18 15:26, Tom Lane wrote:
> FWIW, I agree with Andres' thought that each contrib module should have
> its own subdirectory under $(includedir_server).  Otherwise we're going
> to be faced with questions about whether .h files need to be renamed
> because they're not globally unique enough.

Then they perhaps should be renamed.  That seems like a much simpler
solution.

The use case being discussed here is installing a data type extension's
header so you can write a transform for it.  The extension's name as
well as the data type's own name already have to be pretty much globally
unique if you want it to be useful.  So it doesn't seem very difficult
to me to have the extension install a single header file with that same
name.

The other side of this is that the PLs have to install their header
files.  Which the in-core PLs already do.  Would we we want to move
their header files under a new per-extension directory scheme?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  19. Re: Should contrib modules install .h files?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-04T21:00:56Z

    Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> This installs to $(includedir_server)/$(MODULEDIR)/$MODULE/file.h
> (e.g. include/server/extension/hstore/hstore.h for an actual example),
> and errors if HEADERS_xxx is defined for anything that's not a module
> listed in MODULES or MODULE_big.

I've not studied this patch in any great detail, but the basic plan seems
sane.  However, don't we also need to teach the MSVC build system about
this?

			regards, tom lane

  20. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-07-05T10:45:52Z

    >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 >> This installs to $(includedir_server)/$(MODULEDIR)/$MODULE/file.h
 >> (e.g. include/server/extension/hstore/hstore.h for an actual
 >> example), and errors if HEADERS_xxx is defined for anything that's
 >> not a module listed in MODULES or MODULE_big.

 Tom> I've not studied this patch in any great detail, but the basic
 Tom> plan seems sane. However, don't we also need to teach the MSVC
 Tom> build system about this?

Yes, also needs to update the pgxs docs.

Looking at the windows build, is there anything needed beyond adding a
check for the HEADERS* vars in Install.pm?

-- 
Andrew (irc:RhodiumToad)

  21. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-07-05T12:51:44Z

    >>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:

Patch with docs (including an example) and Install.pm changes (blind).
I'll stick this in the open CF to see what cfbot thinks of it.

Anyone have any objection to putting this into 11beta if it works, as
well as 12devel?

-- 
Andrew (irc:RhodiumToad)

  22. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-07-05T13:25:17Z

    >>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:

 Andrew> Patch with docs (including an example) and Install.pm changes (blind).
 Andrew> I'll stick this in the open CF to see what cfbot thinks of it.

Meh. Oversight in handling empty or missing vars. Also incorrect dir in
msvc version. Fixed I hope.

-- 
Andrew (irc:RhodiumToad)

  23. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-07-05T14:25:24Z

    >>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:

 Andrew> Patch with docs (including an example) and Install.pm changes
 Andrew> (blind). I'll stick this in the open CF to see what cfbot
 Andrew> thinks of it.

 Andrew> Meh. Oversight in handling empty or missing vars. Also
 Andrew> incorrect dir in msvc version. Fixed I hope.

Take 3 for the windows version: don't assume the directories exist
because contrib install is done before the main include files stuff.

-- 
Andrew (irc:RhodiumToad)

  24. Re: Should contrib modules install .h files?

    Michael Paquier <michael@paquier.xyz> — 2018-07-06T01:42:59Z

    On Thu, Jul 05, 2018 at 03:25:24PM +0100, Andrew Gierth wrote:
> Take 3 for the windows version: don't assume the directories exist
> because contrib install is done before the main include files stuff.

It seems to me that this gives a good reason to not play with
REL_11_STABLE at this point.
--
Michael

  25. Re: Should contrib modules install .h files?

    Michael Paquier <michael@paquier.xyz> — 2018-07-06T01:45:07Z

    On Wed, Jul 04, 2018 at 05:00:56PM -0400, Tom Lane wrote:
> Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
>> This installs to $(includedir_server)/$(MODULEDIR)/$MODULE/file.h
>> (e.g. include/server/extension/hstore/hstore.h for an actual example),
>> and errors if HEADERS_xxx is defined for anything that's not a module
>> listed in MODULES or MODULE_big.
> 
> I've not studied this patch in any great detail, but the basic plan seems
> sane.  However, don't we also need to teach the MSVC build system about
> this?

+1.  I have not looked at this patch in details myself, but the concept
looks good.
--
Michael

  26. Re: Should contrib modules install .h files?

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-07-06T22:02:36Z

    On 05.07.18 14:51, Andrew Gierth wrote:
> Anyone have any objection to putting this into 11beta if it works, as
> well as 12devel?

Yes, I have expressed concerns about this approach elsewhere in this thread.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  27. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-07-06T22:14:24Z

    >>>>> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

 > On 02.07.18 15:26, Tom Lane wrote:
 >> FWIW, I agree with Andres' thought that each contrib module should
 >> have its own subdirectory under $(includedir_server). Otherwise
 >> we're going to be faced with questions about whether .h files need
 >> to be renamed because they're not globally unique enough.

 Peter> Then they perhaps should be renamed. That seems like a much
 Peter> simpler solution.

Personally I think that more -I options is less pain than having to
rename things or deal with conflicts.

Where exactly are you suggesting that they should be installed? Directly
in $(installdir_server), or in $(installdir_server)/extension or
equivalent?

 Peter> The use case being discussed here is installing a data type
 Peter> extension's header so you can write a transform for it. The
 Peter> extension's name as well as the data type's own name already
 Peter> have to be pretty much globally unique if you want it to be
 Peter> useful. So it doesn't seem very difficult to me to have the
 Peter> extension install a single header file with that same name.

That's assuming a single header file, which might be a bit more
restrictive than absolutely necessary.

 Peter> The other side of this is that the PLs have to install their
 Peter> header files. Which the in-core PLs already do. Would we we want
 Peter> to move their header files under a new per-extension directory
 Peter> scheme?

The in-core PLs could reasonably be grandfathered in in their current
locations, at least for now.

-- 
Andrew (irc:RhodiumToad)

  28. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-07-23T00:48:21Z

    >>>>> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

 >> Anyone have any objection to putting this into 11beta if it works,
 >> as well as 12devel?

 Peter> Yes, I have expressed concerns about this approach elsewhere in
 Peter> this thread.

No response to my followup to you in 2+ weeks. Last call?

-- 
Andrew (irc:RhodiumToad)

  29. Re: Should contrib modules install .h files?

    Michael Paquier <michael@paquier.xyz> — 2018-07-23T00:51:33Z

    On Mon, Jul 23, 2018 at 01:48:21AM +0100, Andrew Gierth wrote:
> >>>>> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> 
>>> Anyone have any objection to putting this into 11beta if it works,
>>> as well as 12devel?
> 
>  Peter> Yes, I have expressed concerns about this approach elsewhere in
>  Peter> this thread.
> 
> No response to my followup to you in 2+ weeks. Last call?

FWIW, I don't have any objections with experimenting on HEAD, but I
would vote for letting 11 out of this.
--
Michael

  30. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-07-23T01:02:51Z

    >>>>> "Michael" == Michael Paquier <michael@paquier.xyz> writes:

 >> No response to my followup to you in 2+ weeks. Last call?

 Michael> FWIW, I don't have any objections with experimenting on HEAD,
 Michael> but I would vote for letting 11 out of this.

Why?

-- 
Andrew.

  31. Re: Should contrib modules install .h files?

    Andres Freund <andres@anarazel.de> — 2018-07-23T01:18:28Z

    Hi,

On 2018-07-23 02:02:51 +0100, Andrew Gierth wrote:
> >>>>> "Michael" == Michael Paquier <michael@paquier.xyz> writes:
> 
>  >> No response to my followup to you in 2+ weeks. Last call?
> 
>  Michael> FWIW, I don't have any objections with experimenting on HEAD,
>  Michael> but I would vote for letting 11 out of this.
> 
> Why?

Because that's the default assumption when considerint to committ a new
feature to a feature frozen branch?  I don't really have a strong
feeling about this specific case either way, personally. But it's still
the default assumption.

Greetings,

Andres Freund

  32. Re: Should contrib modules install .h files?

    Stephen Frost <sfrost@snowman.net> — 2018-07-23T01:42:08Z

    Greetings,

* Andrew Gierth (andrew@tao11.riddles.org.uk) wrote:
> >>>>> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> 
>  > On 02.07.18 15:26, Tom Lane wrote:
>  >> FWIW, I agree with Andres' thought that each contrib module should
>  >> have its own subdirectory under $(includedir_server). Otherwise
>  >> we're going to be faced with questions about whether .h files need
>  >> to be renamed because they're not globally unique enough.
> 
>  Peter> Then they perhaps should be renamed. That seems like a much
>  Peter> simpler solution.
> 
> Personally I think that more -I options is less pain than having to
> rename things or deal with conflicts.

Yeah, I don't care for the idea that we should expect all extensions,
forever going forward, to provide one single .h file which has to be
unique and non-conflicting with all other extensions, ever.

When I think about the demands of extensions, I tend to consider PostGIS
the prime example and I certainly would understand if they wanted to
install multiple headers (they have some 72 .h files from what I'm
seeing...).

So, +1 from me for having a directory for each extension.

> Where exactly are you suggesting that they should be installed? Directly
> in $(installdir_server), or in $(installdir_server)/extension or
> equivalent?

I certainly wouldn't want extension headers being mixed in with server
headers.  I haven't got any great ideas about contrib-vs-extension, but
I'm, at least, leaning towards 'extension' as being the best answer
here.

>  Peter> The use case being discussed here is installing a data type
>  Peter> extension's header so you can write a transform for it. The
>  Peter> extension's name as well as the data type's own name already
>  Peter> have to be pretty much globally unique if you want it to be
>  Peter> useful. So it doesn't seem very difficult to me to have the
>  Peter> extension install a single header file with that same name.
> 
> That's assuming a single header file, which might be a bit more
> restrictive than absolutely necessary.

Agreed that having a single header file is overly and unnecessairly
restrictive.

>  Peter> The other side of this is that the PLs have to install their
>  Peter> header files. Which the in-core PLs already do. Would we we want
>  Peter> to move their header files under a new per-extension directory
>  Peter> scheme?
> 
> The in-core PLs could reasonably be grandfathered in in their current
> locations, at least for now.

Grandfathering them seems fine to me, but I don't hold that position
very strongly.

Thanks!

Stephen

  33. Re: Should contrib modules install .h files?

    Stephen Frost <sfrost@snowman.net> — 2018-07-23T01:47:22Z

    Greetings,

* Andrew Gierth (andrew@tao11.riddles.org.uk) wrote:
> >>>>> "Michael" == Michael Paquier <michael@paquier.xyz> writes:
> 
>  >> No response to my followup to you in 2+ weeks. Last call?
> 
>  Michael> FWIW, I don't have any objections with experimenting on HEAD,
>  Michael> but I would vote for letting 11 out of this.
> 
> Why?

While I agree that this patch is a good improvements for us to have,
it's not something we'd back-patch to released versions and while we can
play a little looser with v11 since it's in beta, I tend to agree with
Michael that this change isn't really appropriate to include in v11 now
that we're in beta.

The current situation isn't great, but it's what people are familiar
with and may already be expecting in v11.  Ideally, we want extension
authors and others testing v11 to find actual bugs and breaking things
for them post feature-freeze and beta release isn't going to encourage
them to play with beta versions in the future, meaning we'll end up with
folks waiting until the release to do testing and end up with a poorer
release for it.

Thanks!

Stephen

  34. Re: Should contrib modules install .h files?

    Michael Paquier <michael@paquier.xyz> — 2018-07-23T03:24:05Z

    On Sun, Jul 22, 2018 at 09:42:08PM -0400, Stephen Frost wrote:
> When I think about the demands of extensions, I tend to consider PostGIS
> the prime example and I certainly would understand if they wanted to
> install multiple headers (they have some 72 .h files from what I'm
> seeing...).
> 
> So, +1 from me for having a directory for each extension.

Definitely.  If we were to choose the one-file per extension choice,
most large extension maintainers would logically scream at us.  If for
example you look at Citus, in src/include/distributed there are a bunch
of them.  Then based on that folks could always tweak their CFLAGS
pointing to the path of the extension if they need to.

We cannot ensure either that multiple extensions do not use the same
header file names, which discards any design using a single installation
location with multiple files.

So, like Stephen, that's a +1 from me.
--
Michael

  35. Re: Should contrib modules install .h files?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-23T04:15:53Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Sun, Jul 22, 2018 at 09:42:08PM -0400, Stephen Frost wrote:
>> So, +1 from me for having a directory for each extension.

> So, like Stephen, that's a +1 from me.

Same here.  One-file-per-extension is too strongly biased to tiny
extensions (like most of our contrib examples).

I don't have a real strong opinion on whether it's too late to
push this into v11.  I do not think it'd break anything other than
packagers' lists of files to be installed ... but it does seem
like a new feature, and we're past feature freeze.

			regards, tom lane

  36. Re: Should contrib modules install .h files?

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-07-23T04:39:40Z

    On 2018-Jul-23, Tom Lane wrote:

> Michael Paquier <michael@paquier.xyz> writes:
> > On Sun, Jul 22, 2018 at 09:42:08PM -0400, Stephen Frost wrote:
> >> So, +1 from me for having a directory for each extension.
> 
> > So, like Stephen, that's a +1 from me.
> 
> Same here.  One-file-per-extension is too strongly biased to tiny
> extensions (like most of our contrib examples).
> 
> I don't have a real strong opinion on whether it's too late to
> push this into v11.  I do not think it'd break anything other than
> packagers' lists of files to be installed ... but it does seem
> like a new feature, and we're past feature freeze.

Frankly, I'd rather make things as easy as possible for third-party
extension writers.  I'd go as far as backpatching further (considering
transforms were introduced in 9.5) but I hesitate on that, because of
the packagers argument.  pg11 seems fair game to me, though.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  37. Re: Should contrib modules install .h files?

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-07-23T15:22:16Z

    On 23.07.18 06:15, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> On Sun, Jul 22, 2018 at 09:42:08PM -0400, Stephen Frost wrote:
>>> So, +1 from me for having a directory for each extension.
> 
>> So, like Stephen, that's a +1 from me.
> 
> Same here.  One-file-per-extension is too strongly biased to tiny
> extensions (like most of our contrib examples).

Nobody said anything about one-file-per-extension.  You can of course
have hstore_this.h and hstore_that.h or if you want to have many, use
postgis/this.h and postgis/that.h.  That's how every C package in the
world works.  We don't need to legislate further here other than, use
sensible naming.

Also, let's recall that the point of this exercise is that you want to
install the header files so that you can build things (another
extension) that somehow interacts with those extensions.  Then, even if
you put things in separate directories per extension, you still need to
make sure that all the installed header files don't clash, since you'll
be adding the -I options of several of them.  In a way, doing it this
way will make things less robust, since it will appear to give extension
authors license to use generic header names.

> I don't have a real strong opinion on whether it's too late to
> push this into v11.  I do not think it'd break anything other than
> packagers' lists of files to be installed ... but it does seem
> like a new feature, and we're past feature freeze.

Certainly a new feature.  I suggest submitting it to the next commit fest.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  38. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-07-23T15:33:31Z

    >>>>> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

 Peter> Nobody said anything about one-file-per-extension. You can of
 Peter> course have hstore_this.h and hstore_that.h or if you want to
 Peter> have many, use postgis/this.h and postgis/that.h.

So now you want the extension to be able to _optionally_ specify a
subdirectory?

(just having the extension do  HEADERS=foo/bar.h  will not work, because
as with every other similar makefile variable, that means "install the
file foo/bar.h as bar.h, not as foo/bar.h".)

-- 
Andrew (irc:RhodiumToad)

  39. Re: Should contrib modules install .h files?

    Andres Freund <andres@anarazel.de> — 2018-07-23T15:55:30Z

    On 2018-07-23 17:22:16 +0200, Peter Eisentraut wrote:
> On 23.07.18 06:15, Tom Lane wrote:
> > Michael Paquier <michael@paquier.xyz> writes:
> >> On Sun, Jul 22, 2018 at 09:42:08PM -0400, Stephen Frost wrote:
> >>> So, +1 from me for having a directory for each extension.
> > 
> >> So, like Stephen, that's a +1 from me.
> > 
> > Same here.  One-file-per-extension is too strongly biased to tiny
> > extensions (like most of our contrib examples).
> 
> Nobody said anything about one-file-per-extension.  You can of course
> have hstore_this.h and hstore_that.h or if you want to have many, use
> postgis/this.h and postgis/that.h.  That's how every C package in the
> world works.  We don't need to legislate further here other than, use
> sensible naming.

PGXS provides a certain way of doing things. It's far from insane for us
to procscribe that we go for $extension/.  I fail to see any sort of
advantage of using $extension_$header.h. It only serves to create
additional work.


> Also, let's recall that the point of this exercise is that you want to
> install the header files so that you can build things (another
> extension) that somehow interacts with those extensions.  Then, even if
> you put things in separate directories per extension, you still need to
> make sure that all the installed header files don't clash, since you'll
> be adding the -I options of several of them.

I'd suggest just using the name with the $extension/ prefix going
forward.


> > I don't have a real strong opinion on whether it's too late to
> > push this into v11.  I do not think it'd break anything other than
> > packagers' lists of files to be installed ... but it does seem
> > like a new feature, and we're past feature freeze.
> 
> Certainly a new feature.  I suggest submitting it to the next commit fest.

Given that there seems to be fairly widespread agreement, anyone that
commented but you, and the patch has been reviewed, I see zero reason to
wait for the next CF for a committer's patch.

Greetings,

Andres Freund

  40. Re: Should contrib modules install .h files?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-23T16:12:30Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> Also, let's recall that the point of this exercise is that you want to
> install the header files so that you can build things (another
> extension) that somehow interacts with those extensions.  Then, even if
> you put things in separate directories per extension, you still need to
> make sure that all the installed header files don't clash, since you'll
> be adding the -I options of several of them.  In a way, doing it this
> way will make things less robust, since it will appear to give extension
> authors license to use generic header names.

Personally, I'd recommend using *one* -I switch and having .c files
reference extension headers with #include "extensionname/headername.h".

As I said before, I think that we should change the existing contrib
modules to be coded likewise, all using a single -I switch that points
at SRCDIR/contrib.  That'd help give people the right coding model
to follow.

			regards, tom lane

  41. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-07-23T16:32:20Z

    >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 Tom> As I said before, I think that we should change the existing
 Tom> contrib modules to be coded likewise, all using a single -I switch
 Tom> that points at SRCDIR/contrib. That'd help give people the right
 Tom> coding model to follow.

I don't see that playing nicely with PGXS?

-- 
Andrew.

  42. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-07-31T19:19:49Z

    >>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:

Final patch just to let cfbot test the windows code for me.

A review of contrib/ suggested that cube, hstore, isn, ltree and seg
were the only modules that had useful headers to install, so do those.

-- 
Andrew (irc:RhodiumToad)

  43. Re: Should contrib modules install .h files?

    Andres Freund <andres@anarazel.de> — 2018-07-31T20:17:00Z

    On 2018-07-31 20:19:49 +0100, Andrew Gierth wrote:
> Final patch just to let cfbot test the windows code for me.
> 
> A review of contrib/ suggested that cube, hstore, isn, ltree and seg
> were the only modules that had useful headers to install, so do those.

I'm a bit surprised that you decided to push to the 11 branch - the
consensus in this thread seem to have gone the other way by my read?
Given that that's the rule, and pushing non-fixes is the exception, I'm
not particularly ok with just ignoring that?

- Andres

  44. Re: Should contrib modules install .h files?

    Peter Geoghegan <pg@bowt.ie> — 2018-07-31T20:22:27Z

    On Tue, Jul 31, 2018 at 1:17 PM, Andres Freund <andres@anarazel.de> wrote:
> I'm a bit surprised that you decided to push to the 11 branch - the
> consensus in this thread seem to have gone the other way by my read?
> Given that that's the rule, and pushing non-fixes is the exception, I'm
> not particularly ok with just ignoring that?

+1

-- 
Peter Geoghegan

  45. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-07-31T21:46:15Z

    >>>>> "Andres" == Andres Freund <andres@anarazel.de> writes:

 Andres> I'm a bit surprised that you decided to push to the 11 branch -
 Andres> the consensus in this thread seem to have gone the other way by
 Andres> my read?

There were also quite a few non-objections or "don't care"s to the idea
of pushing it to 11beta, and no real justification for not doing so was
ever given other than "it's a new feature", which I don't really agree
with (it's certainly fixing a defect, even if you don't want to call it
a bug). I actually think an argument could be made for backpatching it
even further (though I don't intend to pursue that).

The reason for making the change in the first place is to support
out-of-tree development (particularly of PLs and their transforms) which
isn't strongly tied to pg's development and release cycle; not pushing
to 11-beta would mean delaying the issue for another year and forcing
yet another cycle of workarounds. (Right now for example I can take
advantage of the fact that hstore.h didn't change between 9.5, 9.6 and
10 except in non-significant whitespace; if I can rely on hstore.h being
actually installed in pg11 onwards, then I can draw a line under the
issue and move on. Otherwise, I have to deal with the fact that hstore.h
_does_ change between 10 and 11 and then wait for 12 to see what that
ends up doing.)

(Obviously I'm biased by the fact that this is an issue that impacts me
personally.)

-- 
Andrew (irc:RhodiumToad)

  46. Re: Should contrib modules install .h files?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-31T21:53:19Z

    Peter Geoghegan <pg@bowt.ie> writes:
> On Tue, Jul 31, 2018 at 1:17 PM, Andres Freund <andres@anarazel.de> wrote:
>> I'm a bit surprised that you decided to push to the 11 branch - the
>> consensus in this thread seem to have gone the other way by my read?
>> Given that that's the rule, and pushing non-fixes is the exception, I'm
>> not particularly ok with just ignoring that?

> +1

By my count of people expressing opinions, we had Michael -1, Stephen -1,
me -0.1 or so, Alvaro +1, Peter -1, presumably +1 from Andrew; and Andres
made a comment about not waiting, which perhaps Andrew read as a +1 for
backpatching.  So it's not unreasonable for Andrew to have decided that
it was about tied.  Nonetheless, it does seem like a feature and we're
past feature freeze, so the default assumption ought to be "no backpatch"
IMO.

			regards, tom lane

  47. Re: Should contrib modules install .h files?

    Andres Freund <andres@anarazel.de> — 2018-07-31T22:00:21Z

    On 2018-07-31 17:53:19 -0400, Tom Lane wrote:
> Peter Geoghegan <pg@bowt.ie> writes:
> > On Tue, Jul 31, 2018 at 1:17 PM, Andres Freund <andres@anarazel.de> wrote:
> >> I'm a bit surprised that you decided to push to the 11 branch - the
> >> consensus in this thread seem to have gone the other way by my read?
> >> Given that that's the rule, and pushing non-fixes is the exception, I'm
> >> not particularly ok with just ignoring that?
> 
> > +1
> 
> By my count of people expressing opinions, we had Michael -1, Stephen -1,
> me -0.1 or so, Alvaro +1, Peter -1, presumably +1 from Andrew; and Andres
> made a comment about not waiting, which perhaps Andrew read as a +1 for
> backpatching.  So it's not unreasonable for Andrew to have decided that
> it was about tied.  Nonetheless, it does seem like a feature and we're
> past feature freeze, so the default assumption ought to be "no backpatch"
> IMO.

Yea, I don't think it's an entirely unreasonable to decide to backpatch
based on these votes, but I think if the stated opinions are like you
count, it's pretty reasonable to at least announce that the more
controversial choice is the plan and give a chance to more vigorously
object.

Greetings,

Andres Freund

  48. Re: Should contrib modules install .h files?

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-07-31T22:34:54Z

    On 23/07/2018 18:32, Andrew Gierth wrote:
>>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> 
>  Tom> As I said before, I think that we should change the existing
>  Tom> contrib modules to be coded likewise, all using a single -I switch
>  Tom> that points at SRCDIR/contrib. That'd help give people the right
>  Tom> coding model to follow.
> 
> I don't see that playing nicely with PGXS?

I'm also not on board that my random third-party extension now has to
refer to its own header files as "subdirectory/headerfile.h".  Which
will mess up existing extensions that have header files in their tree.

Or at least I'm not totally sure what the exact proposal and real-world
implications are, with regard to existing extensions with one or more
header files.

By all means, let's make it easier for large or small extensions to
manage their header files with PGXS.  But let's separate what PGXS can
and should do from what the extension's own file layout is.

But I think there are some fundamentally incompatible goals here with
regard to how the final -I options are supposed to look.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  49. Re: Should contrib modules install .h files?

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-07-31T22:38:25Z

    On 31/07/2018 23:46, Andrew Gierth wrote:
> not pushing
> to 11-beta would mean delaying the issue for another year and forcing
> yet another cycle of workarounds.

But that applies to every single patch.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  50. Re: Should contrib modules install .h files?

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-07-31T22:40:25Z

    On 31/07/2018 21:19, Andrew Gierth wrote:
>>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> 
> Final patch just to let cfbot test the windows code for me.
> 
> A review of contrib/ suggested that cube, hstore, isn, ltree and seg
> were the only modules that had useful headers to install, so do those.

I'm missing some guidance what an extension using those headers is
supposed to do.  How does it get the right -I options?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  51. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-07-31T23:33:11Z

    >>>>> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

 >> A review of contrib/ suggested that cube, hstore, isn, ltree and seg
 >> were the only modules that had useful headers to install, so do
 >> those.

 Peter> I'm missing some guidance what an extension using those headers
 Peter> is supposed to do. How does it get the right -I options?

All of the below assumes PGXS.

If your extension is relying on pg11+, or you have checked the pg
version when constructing the makefile, you can just do:

PG_CPPFLAGS += -I$(includedir_server)/extension/hstore

and #include "hstore.h" will work.

If you need to workaround for old versions in one makefile, as I do,
then you can do something along these lines (this goes before the
include $(PGXS) line):

# MAJORVERSION and includedir_server are not defined yet, but will be
# defined before PG_CPPFLAGS is expanded. So we use conditional
# expansions rather than 'ifeq' syntax.

# for pg11+, hstore.h will be installed here
HSTORE_INCDIR = $(includedir_server)/extension/hstore

# for pg 9.5/9.6/10, we have a local copy of hstore.h since it happens
# to be the same, barring non-semantic whitespace, between the three
# versions
HSTORE_INCDIR_OLD = old_inc

PG_CPPFLAGS += -I$(HSTORE_INCDIR$(if $(filter 9.% 10,$(MAJORVERSION)),_OLD))

If you need to distinguish more versions for whatever reason you can do
this:

HSTORE_INCDIR = $(includedir_server)/extension/hstore
HSTORE_INCDIR_10 =  whatever
HSTORE_INCDIR_9.6 = whatever
HSTORE_INCDIR_9.5 = whatever
HSTORE_INCDIR_9.4 = whatever
HSTORE_INCDIR_9.3 = whatever
PG_CPPFLAGS += -I$(HSTORE_INCDIR$(if $(filter 9.% 10,$(MAJORVERSION)),_$(MAJORVERSION)))

-- 
Andrew (irc:RhodiumToad)

  52. Re: Should contrib modules install .h files?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-01T02:44:32Z

    Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>  Peter> I'm missing some guidance what an extension using those headers
>  Peter> is supposed to do. How does it get the right -I options?

> If your extension is relying on pg11+, or you have checked the pg
> version when constructing the makefile, you can just do:
> PG_CPPFLAGS += -I$(includedir_server)/extension/hstore
> and #include "hstore.h" will work.

I remain of the opinion that it'd be smarter to do

PG_CPPFLAGS += -I$(includedir_server)/extension

then

#include "hstore/hstore.h"

This way requires fewer -I options and is far more robust against header
name conflicts.

			regards, tom lane

  53. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-08-01T03:52:28Z

    >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 >> If your extension is relying on pg11+, or you have checked the pg
 >> version when constructing the makefile, you can just do:
 >> PG_CPPFLAGS += -I$(includedir_server)/extension/hstore
 >> and #include "hstore.h" will work.

 Tom> I remain of the opinion that it'd be smarter to do

 Tom> PG_CPPFLAGS += -I$(includedir_server)/extension

 Tom> then

 Tom> #include "hstore/hstore.h"

 Tom> This way requires fewer -I options and is far more robust against
 Tom> header name conflicts.

Sure, it works for the simple cases like hstore, but how does it handle
the case of a pgxs extension that installs more than one include file,
one of which includes another?

-- 
Andrew (irc:RhodiumToad)

  54. Re: Should contrib modules install .h files?

    Andres Freund <andres@anarazel.de> — 2018-08-01T03:55:15Z

    On 2018-08-01 04:52:28 +0100, Andrew Gierth wrote:
> >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> 
>  >> If your extension is relying on pg11+, or you have checked the pg
>  >> version when constructing the makefile, you can just do:
>  >> PG_CPPFLAGS += -I$(includedir_server)/extension/hstore
>  >> and #include "hstore.h" will work.
> 
>  Tom> I remain of the opinion that it'd be smarter to do
> 
>  Tom> PG_CPPFLAGS += -I$(includedir_server)/extension
> 
>  Tom> then
> 
>  Tom> #include "hstore/hstore.h"
> 
>  Tom> This way requires fewer -I options and is far more robust against
>  Tom> header name conflicts.
> 
> Sure, it works for the simple cases like hstore, but how does it handle
> the case of a pgxs extension that installs more than one include file,
> one of which includes another?

I might be momentarily daft (just was on a conference call for a while
;)), but why'd that be problematic? The header files can just specify
the full path, and they'll find each other because of the aforementioned
-I?

Greetings,

Andres Freund

  55. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-08-01T04:17:27Z

    >>>>> "Andres" == Andres Freund <andres@anarazel.de> writes:

 >> Sure, it works for the simple cases like hstore, but how does it
 >> handle the case of a pgxs extension that installs more than one
 >> include file, one of which includes another?

 Andres> I might be momentarily daft (just was on a conference call for
 Andres> a while ;)), but why'd that be problematic? The header files
 Andres> can just specify the full path, and they'll find each other
 Andres> because of the aforementioned -I?

The #includes in the header files need to work both for the module's own
build, and also for building modules depending on it.

So existing practice would be that the module's own source dir (for
module "foo") would look something like:

./Makefile
./foo.h    (contains #include "foo_2.h")
./foo_2.h
./foo.c    (contains #include "foo.h")

and eventually foo.h and foo_int.h get installed as
$(includedir_server)/extension/foo/foo.h and /foo_2.h

To make this work with Tom's scheme means changing the directory layout
of the original module. For contrib modules it's easy because the "." in
the above paths is already "contrib/foo", but for PGXS the module should
not be making assumptions about the name of its build dir, so you end up
having to rejig the layout to something like

./Makefile
./foo
./foo/foo.h (contains #include "foo/foo_2.h")
./foo/foo_2.h
./foo.c  (contains #include "foo/foo.h", compiled with -I$(srcdir))

or

./Makefile
./include/foo
./include/foo/foo.h
./include/foo/foo_2.h
./foo.c  (compiled with -I$(srcdir)/include)

Either way, it's a forced change to the PGXS module's file layout if it
wants to install headers that work for other people using Tom's
suggested approach. I'm not on board with this unless there's a better
solution than I've seen so far.

-- 
Andrew (irc:RhodiumToad)

  56. Re: Should contrib modules install .h files?

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-08-01T06:56:47Z

    On 01/08/2018 06:17, Andrew Gierth wrote:
> Either way, it's a forced change to the PGXS module's file layout if it
> wants to install headers that work for other people using Tom's
> suggested approach. I'm not on board with this unless there's a better
> solution than I've seen so far.

The problem is that the way we've left it now is so that 50% of the
users will do it wrong.

This reminds me vividly of libxml headers.  For example, on my system
there is a file at

/usr/include/libxml2/libxml/parser.h

How do you use that?

-I/usr/include and #include "libxml2/libxml/parser.h"

or

-I/usr/include/libxml2 and #include "libxml/parser.h"

or

-I/usr/include/libxml2/libxml and #include "parser.h"

Obviously, one can find this out with some experimentation or research,
but in our case there won't be much guidance available.

In the libxml case, the best solution is xml2-config or pkg-config.  I
would like to see something similar here, where users don't have to
fiddle with PG_CPPFLAGS directly.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  57. Re: Should contrib modules install .h files?

    Robert Haas <robertmhaas@gmail.com> — 2018-08-02T13:18:57Z

    On Tue, Jul 31, 2018 at 5:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> By my count of people expressing opinions, we had Michael -1, Stephen -1,
> me -0.1 or so, Alvaro +1, Peter -1, presumably +1 from Andrew; and Andres
> made a comment about not waiting, which perhaps Andrew read as a +1 for
> backpatching.  So it's not unreasonable for Andrew to have decided that
> it was about tied.  Nonetheless, it does seem like a feature and we're
> past feature freeze, so the default assumption ought to be "no backpatch"
> IMO.

Yeah, I would have voted -1 if I'd realized that it was close.  Now
we're in a situation where we have patch not everyone likes not only
in master (which is OK, because we've got a year to decide whether to
change anything) but also in v11 (where we have a lot less time).
That's not so great.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  58. Re: Should contrib modules install .h files?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-02T14:54:00Z

    Robert Haas <robertmhaas@gmail.com> writes:
> Yeah, I would have voted -1 if I'd realized that it was close.  Now
> we're in a situation where we have patch not everyone likes not only
> in master (which is OK, because we've got a year to decide whether to
> change anything) but also in v11 (where we have a lot less time).
> That's not so great.

It seems like there were two separate areas of disagreement/questioning,
one being the file layout (whether to add per-extension subdirectories)
and then one about how one would actually use this, ie what would the
-I switch(es) look like and where would they get injected.

My impression is that there was consensus for per-extension
subdirectories, but the usage scenario isn't totally designed yet.
In principle, only the layout question has to be resolved to make
it OK to ship this in v11.  On the other hand, if there's no
very practical way to use the per-extension subdirectory layout,
we might have to rethink that layout.  So I'd be happier about this
if that question were answered promptly.  Failing a satisfactory
answer in the near future, I think we need to revert in v11.

Also, "near future" means "before Monday".  I don't want to ship beta3
with this in place if we end up reverting later, because it'd mean
thrashing packagers' file manifests, which they won't appreciate.
It might be best to revert in v11 for now, and then we can put it back
after beta3 if there's agreement that the questions are satisfactorily
resolved.

			regards, tom lane

  59. Re: Should contrib modules install .h files?

    Andres Freund <andres@anarazel.de> — 2018-08-02T16:35:04Z

    On 2018-08-02 10:54:00 -0400, Tom Lane wrote:
> Also, "near future" means "before Monday".  I don't want to ship beta3
> with this in place if we end up reverting later, because it'd mean
> thrashing packagers' file manifests, which they won't appreciate.
> It might be best to revert in v11 for now, and then we can put it back
> after beta3 if there's agreement that the questions are satisfactorily
> resolved.

+1

  60. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-08-02T16:46:22Z

    >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 Tom> It seems like there were two separate areas of
 Tom> disagreement/questioning, one being the file layout (whether to
 Tom> add per-extension subdirectories) and then one about how one would
 Tom> actually use this, ie what would the -I switch(es) look like and
 Tom> where would they get injected.

 Tom> My impression is that there was consensus for per-extension
 Tom> subdirectories, but the usage scenario isn't totally designed yet.
 Tom> In principle, only the layout question has to be resolved to make
 Tom> it OK to ship this in v11.

Currently, everything is agnostic about the usage scenario - the
existing extension include files will work with either
-I$(includedir_server)/extension and #include "hstore/hstore.h", or with
-I$(includedir_server)/extension/hstore and #include "hstore.h".

 Tom> On the other hand, if there's no very practical way to use the
 Tom> per-extension subdirectory layout,

What constitutes "practical"?

Right now it seems unlikely that there's much of a use case for
referring to more than two different extensions at a time (a pl and a
data type, for building a transform module outside either the pl's or
the type's source tree). Referring to one is more likely (in my case,
hstore_pllua is written to build inside pllua-ng's source tree, so all
it needs is to get at hstore.h).

-- 
Andrew (irc:RhodiumToad)

  61. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-08-02T16:53:17Z

    >>>>> "Andres" == Andres Freund <andres@anarazel.de> writes:

 >> Also, "near future" means "before Monday". I don't want to ship
 >> beta3 with this in place if we end up reverting later, because it'd
 >> mean thrashing packagers' file manifests, which they won't
 >> appreciate. It might be best to revert in v11 for now, and then we
 >> can put it back after beta3 if there's agreement that the questions
 >> are satisfactorily resolved.

 Andres> +1

On the other hand, _I'm_ getting pressure from at least one packager to
nail down a final release of pllua-ng so they can build it along with
beta3 (in place of the old broken pllua), which obviously I can't do if
I keep having to fiddle with workarounds for hstore.h.

-- 
Andrew (irc:RhodiumToad)

  62. Re: Should contrib modules install .h files?

    Andres Freund <andres@anarazel.de> — 2018-08-02T16:56:22Z

    On 2018-08-02 17:53:17 +0100, Andrew Gierth wrote:
> >>>>> "Andres" == Andres Freund <andres@anarazel.de> writes:
> 
>  >> Also, "near future" means "before Monday". I don't want to ship
>  >> beta3 with this in place if we end up reverting later, because it'd
>  >> mean thrashing packagers' file manifests, which they won't
>  >> appreciate. It might be best to revert in v11 for now, and then we
>  >> can put it back after beta3 if there's agreement that the questions
>  >> are satisfactorily resolved.
> 
>  Andres> +1
> 
> On the other hand, _I'm_ getting pressure from at least one packager to
> nail down a final release of pllua-ng so they can build it along with
> beta3 (in place of the old broken pllua), which obviously I can't do if
> I keep having to fiddle with workarounds for hstore.h.

I just don't have a lot of sympathy for that, given the
months-after-freeze-window timing. It's not like *any* of this just
started to be problematic in v11.

Greetings,

Andres Freund

  63. Re: Should contrib modules install .h files?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-02T17:13:11Z

    Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>  Tom> My impression is that there was consensus for per-extension
>  Tom> subdirectories, but the usage scenario isn't totally designed yet.
>  Tom> In principle, only the layout question has to be resolved to make
>  Tom> it OK to ship this in v11.

> Currently, everything is agnostic about the usage scenario - the
> existing extension include files will work with either
> -I$(includedir_server)/extension and #include "hstore/hstore.h", or with
> -I$(includedir_server)/extension/hstore and #include "hstore.h".

Well, the point is we don't want agnosticism --- we want a clear
usage model for people to follow.

>  Tom> On the other hand, if there's no very practical way to use the
>  Tom> per-extension subdirectory layout,

> What constitutes "practical"?

Something that copes with selecting the right headers if you're rebuilding
a module whose headers are already installed (as you mentioned upthread).
Something that copes with different modules installing headers with the
same base name.  Allowing for that was the driving force for going with
subdirectory-per-extension, but if we really want that to work, there
seems to be no alternative but for extensions to write qualified header
names (#include "hstore/hstore.h" not #include "hstore.h").  Andres,
for one, seemed to think that wouldn't play nicely with PGXS, though
I'm not sure why not.  Maybe he only meant that an extension's *own*
headers shouldn't be referenced that way.

Maybe this all just works without much thought, but given that smart
people like Peter E. seem to be unsure of that, I'd sure like to see
a concrete set of rules that extensions should follow for this.

There's also a question of whether we need to change anything in
contrib/ so that it plays by whatever rules we set.  There's an
expectation that contrib modules should be buildable with PGXS,
so they need to follow the rules.

			regards, tom lane

  64. Re: Should contrib modules install .h files?

    Robert Haas <robertmhaas@gmail.com> — 2018-08-02T17:54:39Z

    On Thu, Aug 2, 2018 at 12:56 PM, Andres Freund <andres@anarazel.de> wrote:
>> On the other hand, _I'm_ getting pressure from at least one packager to
>> nail down a final release of pllua-ng so they can build it along with
>> beta3 (in place of the old broken pllua), which obviously I can't do if
>> I keep having to fiddle with workarounds for hstore.h.
>
> I just don't have a lot of sympathy for that, given the
> months-after-freeze-window timing. It's not like *any* of this just
> started to be problematic in v11.

Yeah, I agree.  Andrew, it seems to me that you brought this problem
on yourself.  You rammed a patch through four months after feature
freeze with a marginal consensus and are now complaining about having
to spend more time on it.  Well, that's a self-inflicted injury.  If
you don't commit features with a marginal consensus and/or don't do it
four months after feature freeze, you won't get nearly as much
pushback.

Don't get me wrong -- I think you've done a lot of great work over the
years and I'm glad to have you involved both as a contributor and as a
committer of your own patches and those of others.  It's just that
your email here reads as if you think that your commit privileges are
for your benefit rather than that of the project, and that's not how
it works.  Everybody here is free to pursue the things they think are
interesting and the diversity of such things is part of the strength
of the project, but nobody is free to ram through changes that make
life better for them and worse for other people, even if they don't
agree with the complaints those other people make.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  65. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-08-02T17:56:22Z

    >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 Tom> Maybe this all just works without much thought, but given that
 Tom> smart people like Peter E. seem to be unsure of that, I'd sure
 Tom> like to see a concrete set of rules that extensions should follow
 Tom> for this.

I'll comment on the more substantive stuff later since I just noticed a
few relevant points that I need to investigate. But while investigating,
I found...

 Tom> There's also a question of whether we need to change anything in
 Tom> contrib/ so that it plays by whatever rules we set.  There's an
 Tom> expectation that contrib modules should be buildable with PGXS,
 Tom> so they need to follow the rules.

... that at least all of the *_plperl transform modules in contrib/ fail
to build with USE_PGXS already (i.e. for as long as they have ever
existed), because they rely on plperl_helpers.h which is never installed
anywhere, and trying to get it via $(top_srcdir) obviously can't work in
PGXS.

Haven't tried the python ones yet.

-- 
Andrew.

  66. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-08-02T18:13:05Z

    >>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:

 Tom> There's also a question of whether we need to change anything in
 Tom> contrib/ so that it plays by whatever rules we set.  There's an
 Tom> expectation that contrib modules should be buildable with PGXS,
 Tom> so they need to follow the rules.

 Andrew> ... that at least all of the *_plperl transform modules in
 Andrew> contrib/ fail to build with USE_PGXS already (i.e. for as long
 Andrew> as they have ever existed), because they rely on
 Andrew> plperl_helpers.h which is never installed anywhere, and trying
 Andrew> to get it via $(top_srcdir) obviously can't work in PGXS.

 Andrew> Haven't tried the python ones yet.

And none of the plpython transforms can even parse their makefiles with
USE_PGXS, let alone build, because they have an "include" directive
pointing into src/pl/plpython.

-- 
Andrew (irc:RhodiumToad)

  67. Re: Should contrib modules install .h files?

    Andres Freund <andres@anarazel.de> — 2018-08-02T18:21:44Z

    On 2018-08-02 19:13:05 +0100, Andrew Gierth wrote:
> >>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> 
>  Tom> There's also a question of whether we need to change anything in
>  Tom> contrib/ so that it plays by whatever rules we set.  There's an
>  Tom> expectation that contrib modules should be buildable with PGXS,
>  Tom> so they need to follow the rules.
> 
>  Andrew> ... that at least all of the *_plperl transform modules in
>  Andrew> contrib/ fail to build with USE_PGXS already (i.e. for as long
>  Andrew> as they have ever existed), because they rely on
>  Andrew> plperl_helpers.h which is never installed anywhere, and trying
>  Andrew> to get it via $(top_srcdir) obviously can't work in PGXS.
> 
>  Andrew> Haven't tried the python ones yet.
> 
> And none of the plpython transforms can even parse their makefiles with
> USE_PGXS, let alone build, because they have an "include" directive
> pointing into src/pl/plpython.

FWIW, I'd be perfectly on board with just burying this policy. Designating
one contrib module (or something in src/test/examples or such) as a PGXS
example, and always building it with pgxs seems like it'd do a much
better job than the current policy.

Greetings,

Andres Freund

  68. Re: Should contrib modules install .h files?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-02T18:31:38Z

    Andres Freund <andres@anarazel.de> writes:
> On 2018-08-02 19:13:05 +0100, Andrew Gierth wrote:
>> And none of the plpython transforms can even parse their makefiles with
>> USE_PGXS, let alone build, because they have an "include" directive
>> pointing into src/pl/plpython.

> FWIW, I'd be perfectly on board with just burying this policy. Designating
> one contrib module (or something in src/test/examples or such) as a PGXS
> example, and always building it with pgxs seems like it'd do a much
> better job than the current policy.

No, I think that'd be pretty wrongheaded.  One of the big reasons for
contrib to exist is to serve as a testbed proving that our extension
features actually work.  What you suggest would reduce the scope of
that testing, which seems like the wrong direction to be going in.

It's particularly bad for these cases, since what they demonstrate is
that it's impossible to build transform modules for plperl or plpython
out-of-tree at the moment.  That doesn't seem to me to be something
we should just ignore; it goes against not only our larger commitment
to extensibility, but also the entire point of the transforms feature.

			regards, tom lane

  69. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-08-02T18:42:31Z

    >>>>> "Andres" == Andres Freund <andres@anarazel.de> writes:

 Tom> There's also a question of whether we need to change anything in
 Tom> contrib/ so that it plays by whatever rules we set.  There's an
 Tom> expectation that contrib modules should be buildable with PGXS,
 Tom> so they need to follow the rules.

 Andrew> ... that at least all of the *_plperl transform modules in
 Andrew> contrib/ fail to build with USE_PGXS already (i.e. for as long
 Andrew> as they have ever existed), because they rely on
 Andrew> plperl_helpers.h which is never installed anywhere, and trying
 Andrew> to get it via $(top_srcdir) obviously can't work in PGXS.

 Andrew> Haven't tried the python ones yet.

 >> And none of the plpython transforms can even parse their makefiles
 >> with USE_PGXS, let alone build, because they have an "include"
 >> directive pointing into src/pl/plpython.

 Andres> FWIW, I'd be perfectly on board with just burying this policy.
 Andres> Designating one contrib module (or something in
 Andres> src/test/examples or such) as a PGXS example, and always
 Andres> building it with pgxs seems like it'd do a much better job than
 Andres> the current policy.

I suggest that modules that actually _can't_ build with PGXS should have
the PGXS logic removed from their makefiles (perhaps replaced with an
error). But for the rest, it's a convenience to be able to build single
modules using USE_PGXS without having to configure the whole source tree.

-- 
Andrew (irc:RhodiumToad)

  70. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-08-02T23:14:07Z

    >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 Tom> It's particularly bad for these cases, since what they demonstrate
 Tom> is that it's impossible to build transform modules for plperl or
 Tom> plpython out-of-tree at the moment.

Right. Both plperl and plpython install _some_ header files (which
behavior was added in the commit for the transforms feature), but they
don't install all of the header files that the existing transform
modules actually use. Is this supposed to be a principled choice, with
an out-of-tree transform expected to provide their own code instead of
using those headers, or is it just an oversight?

 Tom> That doesn't seem to me to be something we should just ignore; it
 Tom> goes against not only our larger commitment to extensibility, but
 Tom> also the entire point of the transforms feature.

Yeah, if an out-of-tree data type can't provide a plperl or plpython
transform for itself, something's broken. And that does seem to be the
case at present.

-- 
Andrew (irc:RhodiumToad)

  71. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-08-02T23:31:21Z

    >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 Tom> Something that copes with different modules installing headers
 Tom> with the same base name. Allowing for that was the driving force
 Tom> for going with subdirectory-per-extension, but if we really want
 Tom> that to work, there seems to be no alternative but for extensions
 Tom> to write qualified header names (#include "hstore/hstore.h" not
 Tom> #include "hstore.h"). Andres, for one, seemed to think that
 Tom> wouldn't play nicely with PGXS,

I think that was me, not Andres?

But I think I was partially wrong and that it's possible that this can
be made to work at least in most cases, as long as we can rely on the
same-directory rule for #include "foo.h". (i.e. the first place to look
is always the same directory as the file containing the #include
statement).

I'm going to test this now, trying to do an out-of-both-trees build
of a transform function for an out-of-tree PL that uses multiple .h
files.

-- 
Andrew (irc:RhodiumToad)

  72. Re: Should contrib modules install .h files?

    Michael Paquier <michael@paquier.xyz> — 2018-08-03T21:01:34Z

    On Tue, Jul 31, 2018 at 01:22:27PM -0700, Peter Geoghegan wrote:
> On Tue, Jul 31, 2018 at 1:17 PM, Andres Freund <andres@anarazel.de> wrote:
>> I'm a bit surprised that you decided to push to the 11 branch - the
>> consensus in this thread seem to have gone the other way by my read?
>> Given that that's the rule, and pushing non-fixes is the exception, I'm
>> not particularly ok with just ignoring that?
> 
> +1

+1.  I have been surprised to see that patch pushed to REL_11_STABLE
when looking at the new log entries today.
--
Michael

  73. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-08-03T21:08:04Z

    >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 Tom> On the other hand, if there's no very practical way to use the
 Tom> per-extension subdirectory layout,

 >> What constitutes "practical"?

OK, after considerable experiment I think I can answer these points: the
most "practical" way is to do this (or an equivalent), as you originally
suggested:

PG_CPPFLAGS = -I$(includedir_server)/extension

We could add a mention of this to the PGXS docs and the header comment
of pgxs.mk as being the recommended practice; would that be enough?
Or should the logic go into the pgxs makefile as suggested below?

 Tom> Something that copes with selecting the right headers if you're
 Tom> rebuilding a module whose headers are already installed (as you
 Tom> mentioned upthread).

The module would reference its own headers using #include "foo.h",
which would not find extension/module/foo.h, so no problem here.

No additional constraints apply to the module's own source layout
as long as foo.h, if it includes say foo_2.h, does so as
#include "foo_2.h" (if that's not in the same subdir as foo.h, the
module's own build would need to supply -I options accordingly - no
change here).

 Tom> Something that copes with different modules installing headers
 Tom> with the same base name.

A client of two modules foo and bar can under this scheme do

#include "foo/x.h"
#include "bar/x.h"

and it will reference the correct files; however, if both x.h files
use conflicting symbols for their multiple-inclusion guards, then they
can't both be included in the same source file without explicit #undef
hacks (but this is really an unrelated problem)

Internally to foo and bar, they would both do #include "x.h"

If both foo/x.h and bar/x.h contain #include "y.h", then that will
resolve to extension/foo/y.h in the first case and extension/bar/y.h in
the second case under the "look in the directory of the including file
first" rule.

One case that doesn't "just work" would be what PostGIS currently does.
Like other extensions it doesn't (afaik) currently try and install any
PG-related header files, but if it was modified to do so, some changes
in those header files would be needed because a lot of them have things
like #include "../postgis_config.h" which would fail.

Another case that doesn't "just work" would be if some extension has a
file foo.h that does #include "x/y.h" to include another file that's
part of the same extension, expecting to get extension/foo/x/y.h. Right
now, the install rule I added to the pgxs makefile puts all header files
for a module in the same dir; if we wanted to support making a whole
subdirectory tree under extension/modulename, then that'd require more
work in the makefiles.

Making an out-of-tree build for hstore_plperl etc. work under this
scheme would require changes. The in-tree build has this:

PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl -I$(top_srcdir)/contrib/hstore
#include "hstore.h"

This would have to be:

PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl -I$(top_srcdir)/contrib
#include "hstore/hstore.h"

for the in-tree build, and

PG_CPPFLAGS = -I$(includedir_server)/extension
#include "hstore/hstore.h"

for the out-of-tree build.

This logic could perhaps be best moved into the pgxs makefile itself,
either unconditionally adding -I options to CPPFLAGS, or conditionally
adding them based on a WANT_EXTENSION_HEADERS flag of some sort set by
the module makefile.

-- 
Andrew (irc:RhodiumToad)

  74. Re: Should contrib modules install .h files?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-05T16:51:14Z

    [ back to this after a detour to ON CONFLICT land ]

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> OK, after considerable experiment I think I can answer these points: the
> most "practical" way is to do this (or an equivalent), as you originally
> suggested:
> PG_CPPFLAGS = -I$(includedir_server)/extension

Yeah, that's where I thought we'd end up.

> We could add a mention of this to the PGXS docs and the header comment
> of pgxs.mk as being the recommended practice; would that be enough?
> Or should the logic go into the pgxs makefile as suggested below?

My thought is just to document how to do this.  It'll still be true that
most extensions have no dependencies on other extensions, so they won't
need any such headers; sticking extra -I switches into their builds by
default can only cause trouble.

>  Tom> Something that copes with selecting the right headers if you're
>  Tom> rebuilding a module whose headers are already installed (as you
>  Tom> mentioned upthread).

> The module would reference its own headers using #include "foo.h",
> which would not find extension/module/foo.h, so no problem here.

Check, although we also need to document that you should do it that
way.  Also, at least with gcc, the rule about "look in the calling
file's directory first" would prevent problems (except in VPATH builds
... does PGXS support that?  Should we recommend "-I." before the
"-I$(includedir_server)/extension" switch?)

> One case that doesn't "just work" would be what PostGIS currently does.
> Like other extensions it doesn't (afaik) currently try and install any
> PG-related header files, but if it was modified to do so, some changes
> in those header files would be needed because a lot of them have things
> like #include "../postgis_config.h" which would fail.

Yeah, I don't doubt that extensions will have to make minor mods to
adapt to this scheme.  As long as they're minor, I don't think it's
a problem.

> Another case that doesn't "just work" would be if some extension has a
> file foo.h that does #include "x/y.h" to include another file that's
> part of the same extension, expecting to get extension/foo/x/y.h. Right
> now, the install rule I added to the pgxs makefile puts all header files
> for a module in the same dir; if we wanted to support making a whole
> subdirectory tree under extension/modulename, then that'd require more
> work in the makefiles.

Hm.  Do we know of any extensions big enough that they need subdirectories
of headers?  I don't mind leaving that for later as long as it's not a
present need somewhere.  On the other hand, couldn't it "just work" to
write "x/y.h" in the list of headers to install?

> Making an out-of-tree build for hstore_plperl etc. work under this
> scheme would require changes. The in-tree build has this:
> PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl -I$(top_srcdir)/contrib/hstore
> #include "hstore.h"
> This would have to be:
> PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl -I$(top_srcdir)/contrib
> #include "hstore/hstore.h"
> for the in-tree build, and
> PG_CPPFLAGS = -I$(includedir_server)/extension
> #include "hstore/hstore.h"
> for the out-of-tree build.

> This logic could perhaps be best moved into the pgxs makefile itself,
> either unconditionally adding -I options to CPPFLAGS, or conditionally
> adding them based on a WANT_EXTENSION_HEADERS flag of some sort set by
> the module makefile.

I think we'd want to press forward on making that happen, so that
hstore_plperl and friends can serve as copy-and-pasteable prototype
code for out-of-tree transform modules.  Do you have an idea how to
fix the other problem you mentioned with the plpython makefiles?

			regards, tom lane

  75. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-08-05T20:22:31Z

    >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 >> The module would reference its own headers using #include "foo.h",
 >> which would not find extension/module/foo.h, so no problem here.

 Tom> Check, although we also need to document that you should do it
 Tom> that way. Also, at least with gcc, the rule about "look in the
 Tom> calling file's directory first" would prevent problems (except in
 Tom> VPATH builds ... does PGXS support that? Should we recommend "-I."
 Tom> before the "-I$(includedir_server)/extension" switch?)

PGXS is supposed to support VPATH builds. I do not believe -I. is
needed in the normal case, but we should probably document that if the
module uses any -I flags of its own they should normally go first.

 Tom> Hm. Do we know of any extensions big enough that they need
 Tom> subdirectories of headers? I don't mind leaving that for later as
 Tom> long as it's not a present need somewhere. On the other hand,
 Tom> couldn't it "just work" to write "x/y.h" in the list of headers to
 Tom> install?

It doesn't "just work" because (a) all the existing makefile variables
that give files to install assume that any path given is the source path
only, not to be preserved in the copy; (b) we don't want to constrain
the source file layout in a way that would force .h files to be in a
specific place.

Compare the DATA variable in pgxs: DATA = foo/bar.sql means to install
$(srcdir)/foo/bar.sql as $(DESTDIR)$(datadir)/$(datamoduledir)/bar.sql,
_not_ as $(DESTDIR)$(datadir)/$(datamoduledir)/foo/bar.sql. Making
HEADERS behave otherwise would be inconsistent and inflexible.

For example, suppose my extension source dir looks like this:

./Makefile
./foo.control
./src
./src/foo.h
./src/foo1.c
./src/foo2.c
./scripts/foo--1.0.sql

I would have these in the makefile:

HEADERS = src/foo.h
DATA = scripts/foo--1.0.sql

and it seems clear to me that that should install foo.h as
$(includedir_server)/extension/foo/foo.h and not as foo/src/foo.h.

 >> Making an out-of-tree build for hstore_plperl etc. work [...]

 Tom> I think we'd want to press forward on making that happen, so that
 Tom> hstore_plperl and friends can serve as copy-and-pasteable
 Tom> prototype code for out-of-tree transform modules. Do you have an
 Tom> idea how to fix the other problem you mentioned with the plpython
 Tom> makefiles?

The plpython makefiles are including a makefile to munge regression
tests for python3 vs python2. The most obvious fix would be to install
this makefile as lib/pgxs/src/pl/plpython/regress-python3-mangle.mk
(I don't think any other changes would be needed).

I'll do some tests on this.

-- 
Andrew (irc:RhodiumToad)

  76. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-08-05T22:03:37Z

    >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 >> This logic could perhaps be best moved into the pgxs makefile
 >> itself, either unconditionally adding -I options to CPPFLAGS, or
 >> conditionally adding them based on a WANT_EXTENSION_HEADERS flag of
 >> some sort set by the module makefile.

 Tom> I think we'd want to press forward on making that happen, so that
 Tom> hstore_plperl and friends can serve as copy-and-pasteable
 Tom> prototype code for out-of-tree transform modules. Do you have an
 Tom> idea how to fix the other problem you mentioned with the plpython
 Tom> makefiles?

Here's a patch that fixes (not necessarily in the best way) the PGXS
builds of all the contrib/*_pl{perl,python} modules.

Open questions:

 - is there a better way of doing the conditional setting of
   PG_CPPFLAGS?

 - the choice of which .h files to install from plperl and plpython is
   not principled - I just installed the ones needed for the contrib
   modules to work. Particularly for plpython this list needs to be
   reviewed - but I'm not a pythonist and that should be done by someone
   who is.

-- 
Andrew (irc:RhodiumToad)

  77. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-08-05T22:07:31Z

    >>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:

 Andrew> Here's a patch that fixes (not necessarily in the best way) the
 Andrew> PGXS builds of all the contrib/*_pl{perl,python} modules.

Oh, obviously this patch doesn't fix the windows Install.pm yet, but
that'd be easier to do after finalizing the list of include files to
install.

-- 
Andrew (irc:RhodiumToad)

  78. Re: Should contrib modules install .h files?

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-09-14T17:24:22Z

    On 01/08/2018 00:34, Peter Eisentraut wrote:
> On 23/07/2018 18:32, Andrew Gierth wrote:
>>>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>>
>>  Tom> As I said before, I think that we should change the existing
>>  Tom> contrib modules to be coded likewise, all using a single -I switch
>>  Tom> that points at SRCDIR/contrib. That'd help give people the right
>>  Tom> coding model to follow.
>>
>> I don't see that playing nicely with PGXS?
> 
> I'm also not on board that my random third-party extension now has to
> refer to its own header files as "subdirectory/headerfile.h".  Which
> will mess up existing extensions that have header files in their tree.
> 
> Or at least I'm not totally sure what the exact proposal and real-world
> implications are, with regard to existing extensions with one or more
> header files.
> 
> By all means, let's make it easier for large or small extensions to
> manage their header files with PGXS.  But let's separate what PGXS can
> and should do from what the extension's own file layout is.
> 
> But I think there are some fundamentally incompatible goals here with
> regard to how the final -I options are supposed to look.

Was this ever resolved?

Seems necessary to resolve for PG11.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  79. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-14T17:37:48Z

    >>>>> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

 >> But I think there are some fundamentally incompatible goals here
 >> with regard to how the final -I options are supposed to look.

 Peter> Was this ever resolved?

There are open questions about plperl and plpython, which currently
don't install what's needed for out-of-tree transforms. See the
subthread under this message:

https://www.postgresql.org/message-id/87o9ej8bgl.fsf%40news-spur.riddles.org.uk

Fixing contrib/hstore_plperl etc. to work as examples of how to do
things would obviously first require fixing plperl and plpython, and
while I'm happy to do that I do need feedback first regarding the
plpython headers (I do not use python myself) and the other issues I
mentioned in the message that contains a draft patch.

-- 
Andrew (irc:RhodiumToad)

  80. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-15T19:46:34Z

    >>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:

 Andrew> Here's a patch that fixes (not necessarily in the best way) the
 Andrew> PGXS builds of all the contrib/*_pl{perl,python} modules.

 Andrew> Oh, obviously this patch doesn't fix the windows Install.pm
 Andrew> yet, but that'd be easier to do after finalizing the list of
 Andrew> include files to install.

So while looking into this, I found that to the best of my ability to
determine, the windows Install.pm has _never_ installed the plperl or
plpython header files. That's more than I'm in a position to fix, having
no windows box to test with.

So what I propose to do is to commit a cleaned-up version of the patch
posted above, with these changes:

 - install all the plpy_*.h headers, not just a few; I know of no reason
   to exclude any of them, and in the absence of feedback it seems
   better to install them all;

 - tidy up the makefile variables in the python transform modules to
   remove some duplication

 - fix Mkvcbuild.pm to account for the changes

This will allow hstore_plperl etc. to stand as examples of how to do
cross-module #includes both in and out of tree.

I'll post the patch for review shortly.

-- 
Andrew (irc:RhodiumToad)

  81. Re: Should contrib modules install .h files?

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-15T20:03:47Z

    >>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:

 Andrew> So what I propose to do is to commit a cleaned-up version of
 Andrew> the patch posted above, with these changes:

 Andrew>  - install all the plpy_*.h headers, not just a few; I know of no reason
 Andrew>    to exclude any of them, and in the absence of feedback it seems
 Andrew>    better to install them all;

 Andrew>  - tidy up the makefile variables in the python transform modules to
 Andrew>    remove some duplication

 Andrew>  - fix Mkvcbuild.pm to account for the changes

 Andrew> This will allow hstore_plperl etc. to stand as examples of how
 Andrew> to do cross-module #includes both in and out of tree.

 Andrew> I'll post the patch for review shortly.

And here it is.

-- 
Andrew (irc:RhodiumToad)