Thread: Re: pgsql: Merge catalog/pg_foo_fn.h headers back into pg_foo.hheaders.

On Sun, Apr 08, 2018 at 06:35:39PM +0000, Tom Lane wrote:
> Merge catalog/pg_foo_fn.h headers back into pg_foo.h headers.
>
> Traditionally, include/catalog/pg_foo.h contains extern declarations
> for functions in backend/catalog/pg_foo.c, in addition to its function
> as the authoritative definition of the pg_foo catalog's rowtype.
> In some cases, we'd been forced to split out those extern declarations
> into separate pg_foo_fn.h headers so that the catalog definitions
> could be #include'd by frontend code.  That problem is gone as of
> commit 9c0a0de4c, so let's undo the splits to make things less
> confusing.

This patch or one of its relatives has visibly broken parallel builds
for me. "make -j 4 install" directly called after a configure complains:
In file included from ../../src/include/catalog/catalog.h:22:0,
                 from relpath.c:21:
../../src/include/catalog/pg_class.h:22:10: fatal error: catalog/pg_class_d.h: No such file or directory
 #include "catalog/pg_class_d.h"
          ^~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

I did not take the time to look into the problem, that's just a head's
up.  I am on HEAD at b3b7f789 so it is not like I lack any fixes.
Please note that "make -j 4" directly called works.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Sun, Apr 08, 2018 at 06:35:39PM +0000, Tom Lane wrote:
>> Merge catalog/pg_foo_fn.h headers back into pg_foo.h headers.

> This patch or one of its relatives has visibly broken parallel builds
> for me. "make -j 4 install" directly called after a configure complains:

Hm.  I'd tested "make -j all", but not going directly to "install".
Does it help if you add

$(SUBDIRS:%=install-%-recurse): | submake-generated-headers

to src/Makefile?

(That seems like a bit of a brute-force solution, though.  Anybody
have a better answer?)

            regards, tom lane

On Sun, Apr 08, 2018 at 11:05:09PM -0400, Tom Lane wrote:
> Hm.  I'd tested "make -j all", but not going directly to "install".
> Does it help if you add
>
> $(SUBDIRS:%=install-%-recurse): | submake-generated-headers
>
> to src/Makefile?
>
> (That seems like a bit of a brute-force solution, though.  Anybody
> have a better answer?)

That takes care of the problem from the root of the directory, but when
doing the same from src/bin/ then the same issue shows up even if
src/Makefile is patched to handle install targets.

At the same time, trying to trigger a parallel install from src/backend
makes the build unhappy:
make: *** No rule to make target '../../src/common/libpgcommon_srv.a',
needed by 'postgres'.  Stop.
make: *** Waiting for unfinished jobs....

That may not be worth bothering, just mentioning.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Sun, Apr 08, 2018 at 11:05:09PM -0400, Tom Lane wrote:
>> Hm.  I'd tested "make -j all", but not going directly to "install".
>> Does it help if you add
>> $(SUBDIRS:%=install-%-recurse): | submake-generated-headers
>> to src/Makefile?

> That takes care of the problem from the root of the directory, but when
> doing the same from src/bin/ then the same issue shows up even if
> src/Makefile is patched to handle install targets.

Hm.  Not sure how far we want to go in that direction.  It's never really
been the case that you could configure a maintainer-clean tree and then
go and build in any random subdirectory without taking care of the
generated headers.  In principle, we could do something like the hack
Peter did with temp-install, where it's basically forced to be a
prerequisite of "make check" in EVERY subdirectory and then we buy back
some of the inefficiency by making it a no-op in child make runs.  Not
sure that's a good thing though.

Independently of that, though, I noticed that part of the issue in your
"make install" example is that relpath.c, along with some other frontend
code, includes catalog/catalog.h which includes pg_class.h (and thereby
now pg_class_d.h).  Since src/common/Makefile thinks the only generated
header it needs to worry about is errcodes.h, building src/common first
now falls over.  But allowing frontend code to include catalog.h is
pretty insane: that pulls in relcache.h as well, and surely we don't
want to tie our hands to the point that relcache.h has to be frontend
clean.

What we need to do is take OIDCHARS and TABLESPACE_VERSION_DIRECTORY
out of catalog.h and put them in some more frontend-friendly header.
My first thought was pg_tablespace_d.h, but my second one is relpath.h.
Comments?

            regards, tom lane

I wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> That takes care of the problem from the root of the directory, but when
>> doing the same from src/bin/ then the same issue shows up even if
>> src/Makefile is patched to handle install targets.

> Hm.  Not sure how far we want to go in that direction.  It's never really
> been the case that you could configure a maintainer-clean tree and then
> go and build in any random subdirectory without taking care of the
> generated headers.  In principle, we could do something like the hack
> Peter did with temp-install, where it's basically forced to be a
> prerequisite of "make check" in EVERY subdirectory and then we buy back
> some of the inefficiency by making it a no-op in child make runs.  Not
> sure that's a good thing though.

After further contemplation I decided that that was, in fact, the only
reasonable way to improve matters.  If we have multiple subdirectories
independently firing the "make generated-headers" action, then we have
parallel make hazards of just the same sort I was trying to prevent.
So it's really an all-or-nothing proposition.  The MAKELEVEL hack
plus wiring the prerequisite into the recursion rules is the best way
to make that happen.

Hence, done that way.

            regards, tom lane

On Mon, Apr 09, 2018 at 04:46:34PM -0400, Tom Lane wrote:
> After further contemplation I decided that that was, in fact, the only
> reasonable way to improve matters.  If we have multiple subdirectories
> independently firing the "make generated-headers" action, then we have
> parallel make hazards of just the same sort I was trying to prevent.
> So it's really an all-or-nothing proposition.  The MAKELEVEL hack
> plus wiring the prerequisite into the recursion rules is the best way
> to make that happen.

Oh.  Good idea.  Thanks for the fix.  Hopefully that won't cause much
noise when bisecting bugs, this got in pretty quickly..

No more complains from here.
--
Michael

Hi,

On Mon, Apr 9, 2018 at 10:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> After further contemplation I decided that that was, in fact, the only
> reasonable way to improve matters.  If we have multiple subdirectories
> independently firing the "make generated-headers" action, then we have
> parallel make hazards of just the same sort I was trying to prevent.
> So it's really an all-or-nothing proposition.  The MAKELEVEL hack
> plus wiring the prerequisite into the recursion rules is the best way
> to make that happen.
>
> Hence, done that way.

Compilation of external extensions using PGXS appears to be broken
since this commit:

make[1]: *** /tmp/pgbuild/lib/postgresql/pgxs/src/makefiles/../../src/backend:
No such file or directory.  Stop.
make: *** [/tmp/pgbuild/lib/postgresql/pgxs/src/makefiles/../../src/Makefile.global:370:
submake-generated-headers] Error 2


I think the best fix if to define NO_GENERATED_HEADERS in pgxs.mk,
patch attached.

On Tue, Apr 10, 2018 at 10:08:16AM +0200, Julien Rouhaud wrote:
> Compilation of external extensions using PGXS appears to be broken
> since this commit:
>
> make[1]: *** /tmp/pgbuild/lib/postgresql/pgxs/src/makefiles/../../src/backend:
> No such file or directory.  Stop.
> make: *** [/tmp/pgbuild/lib/postgresql/pgxs/src/makefiles/../../src/Makefile.global:370:
> submake-generated-headers] Error 2

Indeed.  Any external module is blowing up...
--
Michael

Julien Rouhaud <rjuju123@gmail.com> writes:
> Compilation of external extensions using PGXS appears to be broken
> since this commit:

Ouch, sorry.

> I think the best fix if to define NO_GENERATED_HEADERS in pgxs.mk,
> patch attached.

Hm.  I wonder if we don't also want NO_TEMP_INSTALL, like the doc/src/sgml
makefile.  I don't know whether "make check" could be useful in a PGXS
build, but certainly that recipe for making a temp install isn't gonna
work.

            regards, tom lane

On Tue, Apr 10, 2018 at 3:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Julien Rouhaud <rjuju123@gmail.com> writes:
>> I think the best fix if to define NO_GENERATED_HEADERS in pgxs.mk,
>> patch attached.
>
> Hm.  I wonder if we don't also want NO_TEMP_INSTALL, like the doc/src/sgml
> makefile.  I don't know whether "make check" could be useful in a PGXS
> build, but certainly that recipe for making a temp install isn't gonna
> work.

If I understand correctly, PGXS.mk already forbids a "make check" if
PGXS is defined.  And it seems that postgres' contribs rely on
including PGXS.mk, setting NO_PGXS and doing a "make check", so
NO_TEMP_INSTALL shouldn't be needed.

Julien Rouhaud <rjuju123@gmail.com> writes:
> On Tue, Apr 10, 2018 at 3:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hm.  I wonder if we don't also want NO_TEMP_INSTALL, like the doc/src/sgml
>> makefile.  I don't know whether "make check" could be useful in a PGXS
>> build, but certainly that recipe for making a temp install isn't gonna
>> work.

> If I understand correctly, PGXS.mk already forbids a "make check" if
> PGXS is defined.  And it seems that postgres' contribs rely on
> including PGXS.mk, setting NO_PGXS and doing a "make check", so
> NO_TEMP_INSTALL shouldn't be needed.

Well, the question that's bothering me is how come "make check" in
an external build doesn't try to execute the temp-install rule before
printing that error message.  Experimentation shows that it doesn't,
but it sure looks to me like it should: there's nothing conditional
about the "check: temp-install" dependency in Makefile.global.  And
none of the three if-guards in the temp-install rule itself should
prevent this, so what is preventing it?  I don't see it.

For this reason, I thought that adding NO_TEMP_INSTALL would be a good
safety factor in case somebody changes/breaks whatever unobvious
thing is keeping this from failing, and so I went ahead and did it.

            regards, tom lane

On Tue, Apr 10, 2018 at 6:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Well, the question that's bothering me is how come "make check" in
> an external build doesn't try to execute the temp-install rule before
> printing that error message.  Experimentation shows that it doesn't,
> but it sure looks to me like it should: there's nothing conditional
> about the "check: temp-install" dependency in Makefile.global.  And
> none of the three if-guards in the temp-install rule itself should
> prevent this, so what is preventing it?  I don't see it.

I just checked, and for the record the second rule (ifneq
($(abs_top_builddir),) is actually preventing it.  On top of
Makefile.global:

# Set top_srcdir, srcdir, and VPATH.
ifdef PGXS
top_srcdir = $(top_builddir)
[...]
else # not PGXS
[...]
abs_top_builddir = @abs_top_builddir@
[...]
endif # not PGXS

>
> For this reason, I thought that adding NO_TEMP_INSTALL would be a good
> safety factor in case somebody changes/breaks whatever unobvious
> thing is keeping this from failing, and so I went ahead and did it.

It makes sense.  Thanks for correcting and pushing!

Julien Rouhaud <rjuju123@gmail.com> writes:
> On Tue, Apr 10, 2018 at 6:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> none of the three if-guards in the temp-install rule itself should
>> prevent this, so what is preventing it?  I don't see it.

> I just checked, and for the record the second rule (ifneq
> ($(abs_top_builddir),) is actually preventing it.

Ah-hah.  I'd misread that maze of twisty little ifdefs that
sets up abs_top_builddir et al.  Thanks for clarifying!

            regards, tom lane