Should contrib modules install .h files?

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

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

"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)

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

"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)

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

"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)

"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)

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

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

"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)

"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)

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

"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)

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

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

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

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

"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)