Thread
Commits
-
Fix pgxs.mk to not try to build generated headers in external builds.
- 1a40485af6e4 11.0 landed
-
Fix partial-build problems introduced by having more generated headers.
- 3b8f6e75f3c8 11.0 landed
-
Further cleanup of client dependencies on src/include/catalog headers.
- af1a949109d8 11.0 landed
-
Merge catalog/pg_foo_fn.h headers back into pg_foo.h headers.
- cefa3871534d 11.0 cited
-
Switch client-side code to include catalog/pg_foo_d.h not pg_foo.h.
- 9c0a0de4c91b 11.0 cited
-
pgsql: Merge catalog/pg_foo_fn.h headers back into pg_foo.h headers.
Tom Lane <tgl@sss.pgh.pa.us> — 2018-04-08T18:35:39Z
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. Discussion: https://postgr.es/m/23690.1523031777@sss.pgh.pa.us Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/cefa3871534d2c467a520820c0ae3f002a46d8e4 Modified Files -------------- contrib/sepgsql/dml.c | 2 +- src/backend/catalog/dependency.c | 3 - src/backend/catalog/heap.c | 2 - src/backend/catalog/index.c | 3 +- src/backend/catalog/namespace.c | 1 - src/backend/catalog/objectaddress.c | 1 - src/backend/catalog/partition.c | 1 - src/backend/catalog/pg_aggregate.c | 2 - src/backend/catalog/pg_collation.c | 1 - src/backend/catalog/pg_constraint.c | 1 - src/backend/catalog/pg_conversion.c | 1 - src/backend/catalog/pg_inherits.c | 1 - src/backend/catalog/pg_operator.c | 1 - src/backend/catalog/pg_proc.c | 3 +- src/backend/catalog/pg_type.c | 1 - src/backend/commands/aggregatecmds.c | 1 - src/backend/commands/analyze.c | 2 +- src/backend/commands/collationcmds.c | 1 - src/backend/commands/conversioncmds.c | 1 - src/backend/commands/functioncmds.c | 2 - src/backend/commands/indexcmds.c | 3 +- src/backend/commands/lockcmds.c | 2 +- src/backend/commands/operatorcmds.c | 1 - src/backend/commands/proclang.c | 1 - src/backend/commands/publicationcmds.c | 2 +- src/backend/commands/tablecmds.c | 3 - src/backend/commands/trigger.c | 3 +- src/backend/commands/typecmds.c | 3 - src/backend/commands/vacuum.c | 2 +- src/backend/executor/execPartition.c | 2 +- src/backend/optimizer/plan/planner.c | 2 +- src/backend/optimizer/prep/prepunion.c | 2 +- src/backend/parser/parse_agg.c | 2 +- src/backend/parser/parse_clause.c | 2 +- src/backend/parser/parse_coerce.c | 2 +- src/backend/parser/parse_utilcmd.c | 1 - src/backend/rewrite/rowsecurity.c | 2 +- src/backend/tcop/utility.c | 2 +- src/include/catalog/pg_aggregate.h | 36 ++++++++++ src/include/catalog/pg_aggregate_fn.h | 52 -------------- src/include/catalog/pg_collation.h | 11 +++ src/include/catalog/pg_collation_fn.h | 27 -------- src/include/catalog/pg_constraint.h | 88 +++++++++++++++++++++++ src/include/catalog/pg_constraint_fn.h | 104 ---------------------------- src/include/catalog/pg_conversion.h | 11 +++ src/include/catalog/pg_conversion_fn.h | 27 -------- src/include/catalog/pg_db_role_setting.h | 1 + src/include/catalog/pg_enum.h | 1 + src/include/catalog/pg_inherits.h | 14 ++++ src/include/catalog/pg_inherits_fn.h | 30 -------- src/include/catalog/pg_operator.h | 20 ++++++ src/include/catalog/pg_operator_fn.h | 36 ---------- src/include/catalog/pg_proc.h | 34 +++++++++ src/include/catalog/pg_proc_fn.h | 50 ------------- src/include/catalog/pg_publication.h | 1 + src/include/catalog/pg_replication_origin.h | 1 + src/include/catalog/pg_subscription.h | 1 + src/include/catalog/pg_subscription_rel.h | 1 + src/include/catalog/pg_type.h | 67 ++++++++++++++++++ src/include/catalog/pg_type_fn.h | 84 ---------------------- src/pl/plperl/plperl.c | 1 - src/pl/plpgsql/src/pl_comp.c | 1 - src/pl/plpython/plpy_procedure.c | 1 - 63 files changed, 304 insertions(+), 464 deletions(-)
-
Re: pgsql: Merge catalog/pg_foo_fn.h headers back into pg_foo.h headers.
Michael Paquier <michael@paquier.xyz> — 2018-04-09T01:00:31Z
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 -
Re: pgsql: Merge catalog/pg_foo_fn.h headers back into pg_foo.h headers.
Tom Lane <tgl@sss.pgh.pa.us> — 2018-04-09T03:05:09Z
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
-
Re: pgsql: Merge catalog/pg_foo_fn.h headers back into pg_foo.h headers.
Michael Paquier <michael@paquier.xyz> — 2018-04-09T06:13:54Z
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
-
Re: pgsql: Merge catalog/pg_foo_fn.h headers back into pg_foo.h headers.
Tom Lane <tgl@sss.pgh.pa.us> — 2018-04-09T17:21:49Z
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
-
Re: pgsql: Merge catalog/pg_foo_fn.h headers back into pg_foo.h headers.
Tom Lane <tgl@sss.pgh.pa.us> — 2018-04-09T20:46:34Z
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
-
Re: pgsql: Merge catalog/pg_foo_fn.h headers back into pg_foo.h headers.
Michael Paquier <michael@paquier.xyz> — 2018-04-10T02:59:43Z
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
-
Re: pgsql: Merge catalog/pg_foo_fn.h headers back into pg_foo.h headers.
Julien Rouhaud <rjuju123@gmail.com> — 2018-04-10T08:08:16Z
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.
-
Re: pgsql: Merge catalog/pg_foo_fn.h headers back into pg_foo.h headers.
Michael Paquier <michael@paquier.xyz> — 2018-04-10T09:03:39Z
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
-
Re: pgsql: Merge catalog/pg_foo_fn.h headers back into pg_foo.h headers.
Tom Lane <tgl@sss.pgh.pa.us> — 2018-04-10T13:46:32Z
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
-
Re: pgsql: Merge catalog/pg_foo_fn.h headers back into pg_foo.h headers.
Julien Rouhaud <rjuju123@gmail.com> — 2018-04-10T16:13:01Z
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.
-
Re: pgsql: Merge catalog/pg_foo_fn.h headers back into pg_foo.h headers.
Tom Lane <tgl@sss.pgh.pa.us> — 2018-04-10T16:58:10Z
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
-
Re: pgsql: Merge catalog/pg_foo_fn.h headers back into pg_foo.h headers.
Julien Rouhaud <rjuju123@gmail.com> — 2018-04-10T17:27:27Z
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!
-
Re: pgsql: Merge catalog/pg_foo_fn.h headers back into pg_foo.h headers.
Tom Lane <tgl@sss.pgh.pa.us> — 2018-04-10T17:47:43Z
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