Broken handling of lwlocknames.h

Re: Tom Lane 2016-06-27 <31398.1467036827@sss.pgh.pa.us>

Bjorn Munch reported off-list that this sequence:

unpack tarball, cd into it
./configure ...
cd src/test/regress
make

no longer works in 9.6beta2, where it did work in previous releases.
I have confirmed both statements. The failure looks like

gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -g -O1 -fpic -I../../../src/include -D_GNU_SOURCE -c -o regress.o regress.c
In file included from ../../../src/include/storage/lock.h:23,
from ../../../src/include/access/heapam.h:22,
from ../../../src/include/nodes/execnodes.h:18,
from ../../../src/include/commands/trigger.h:17,
from regress.c:29:
../../../src/include/storage/lwlock.h:129:33: error: storage/lwlocknames.h: No such file or directory
make: *** [regress.o] Error 1

So this is some sort of fallout from commit aa65de042f582896, which
invented that as a generated file.

Perhaps the solution is to extend src/test/regress/GNUmakefile to know
about this file explicitly (as it already does know about
src/port/pg_config_paths.h). But that seems rather brute-force; in
particular it seems like that does nothing to keep us from getting burnt
again the same way in future. I wonder if we should modify
src/backend/Makefile so that it exposes a phony target for "update all the
generated include files", and then have src/test/regress/GNUmakefile call
that. Or maybe there are other ways.

That would also fix the "build plpython3 only" problem I was aiming at
in /messages/by-id/20150916200959.GB32090@msg.df7cb.de

So another +1 from a packagers perspective.

Christoph

On Tue, Jun 28, 2016 at 3:22 AM, Christoph Berg <myon@debian.org> wrote:

Re: Tom Lane 2016-06-27 <31398.1467036827@sss.pgh.pa.us>

Bjorn Munch reported off-list that this sequence:

unpack tarball, cd into it
./configure ...
cd src/test/regress
make

no longer works in 9.6beta2, where it did work in previous releases.
I have confirmed both statements. The failure looks like

gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -g -O1 -fpic -I../../../src/include -D_GNU_SOURCE -c -o regress.o regress.c
In file included from ../../../src/include/storage/lock.h:23,
from ../../../src/include/access/heapam.h:22,
from ../../../src/include/nodes/execnodes.h:18,
from ../../../src/include/commands/trigger.h:17,
from regress.c:29:
../../../src/include/storage/lwlock.h:129:33: error: storage/lwlocknames.h: No such file or directory
make: *** [regress.o] Error 1

So this is some sort of fallout from commit aa65de042f582896, which
invented that as a generated file.

Perhaps the solution is to extend src/test/regress/GNUmakefile to know
about this file explicitly (as it already does know about
src/port/pg_config_paths.h). But that seems rather brute-force; in
particular it seems like that does nothing to keep us from getting burnt
again the same way in future. I wonder if we should modify
src/backend/Makefile so that it exposes a phony target for "update all the
generated include files", and then have src/test/regress/GNUmakefile call
that. Or maybe there are other ways.

That would also fix the "build plpython3 only" problem I was aiming at
in /messages/by-id/20150916200959.GB32090@msg.df7cb.de

So another +1 from a packagers perspective.

Yes that would be indeed cleaner this way. I have poked a bit at that
and finished with the attached that defines some rules to generate all
the files needed. But actually it does not seem to be enough, for
example on OSX this would fail to compile because it cannot find the
postgres binary in src/backend/postgres. Does somebody have an idea
what this is about? It seems that we have two problems here.
--
Michael

On Tue, Jun 28, 2016 at 12:26:00PM +0900, Michael Paquier wrote:

On Tue, Jun 28, 2016 at 3:22 AM, Christoph Berg <myon@debian.org> wrote:

Re: Tom Lane 2016-06-27 <31398.1467036827@sss.pgh.pa.us>

Bjorn Munch reported off-list that this sequence:

unpack tarball, cd into it
./configure ...
cd src/test/regress
make

no longer works in 9.6beta2, where it did work in previous releases.
I have confirmed both statements. The failure looks like

gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -g -O1 -fpic -I../../../src/include -D_GNU_SOURCE -c -o regress.o regress.c
In file included from ../../../src/include/storage/lock.h:23,
from ../../../src/include/access/heapam.h:22,
from ../../../src/include/nodes/execnodes.h:18,
from ../../../src/include/commands/trigger.h:17,
from regress.c:29:
../../../src/include/storage/lwlock.h:129:33: error: storage/lwlocknames.h: No such file or directory
make: *** [regress.o] Error 1

So this is some sort of fallout from commit aa65de042f582896, which
invented that as a generated file.

Perhaps the solution is to extend src/test/regress/GNUmakefile to know
about this file explicitly (as it already does know about
src/port/pg_config_paths.h). But that seems rather brute-force; in
particular it seems like that does nothing to keep us from getting burnt
again the same way in future. I wonder if we should modify
src/backend/Makefile so that it exposes a phony target for "update all the
generated include files", and then have src/test/regress/GNUmakefile call
that. Or maybe there are other ways.

That would also fix the "build plpython3 only" problem I was aiming at
in /messages/by-id/20150916200959.GB32090@msg.df7cb.de

So another +1 from a packagers perspective.

Yes that would be indeed cleaner this way. I have poked a bit at that
and finished with the attached that defines some rules to generate all
the files needed. But actually it does not seem to be enough, for
example on OSX this would fail to compile because it cannot find the
postgres binary in src/backend/postgres. Does somebody have an idea
what this is about? It seems that we have two problems here.

[Action required within 72 hours. This is a generic notification.]

The above-described topic is currently a PostgreSQL 9.6 open item. Robert,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
9.6 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1]/messages/by-id/20160527025039.GA447393@tornado.leadboat.com and send a status update within 72 hours of this
message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping 9.6rc1. Consequently, I will appreciate your
efforts toward speedy resolution. Thanks.

[1]: /messages/by-id/20160527025039.GA447393@tornado.leadboat.com

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

On Fri, Jul 1, 2016 at 12:09 AM, Noah Misch <noah@leadboat.com> wrote:

[Action required within 72 hours. This is a generic notification.]

The above-described topic is currently a PostgreSQL 9.6 open item. Robert,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
9.6 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1] and send a status update within 72 hours of this
message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping 9.6rc1. Consequently, I will appreciate your
efforts toward speedy resolution. Thanks.

[1] /messages/by-id/20160527025039.GA447393@tornado.leadboat.com

Since Tom proposed the approach which Michael's patch takes, I'm
hoping he will review and commit this. If it is left to me to fix it,
I may just adopt a minimal fix.

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

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

Robert Haas <robertmhaas@gmail.com> writes:

Since Tom proposed the approach which Michael's patch takes, I'm
hoping he will review and commit this. If it is left to me to fix it,
I may just adopt a minimal fix.

I'll take a look at it.

regards, tom lane

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

On Fri, Jul 1, 2016 at 10:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

Since Tom proposed the approach which Michael's patch takes, I'm
hoping he will review and commit this. If it is left to me to fix it,
I may just adopt a minimal fix.

I'll take a look at it.

Note: the patch is not complete yet. I found some bugs in it.
--
Michael

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

Michael Paquier <michael.paquier@gmail.com> writes:

Yes that would be indeed cleaner this way. I have poked a bit at that
and finished with the attached that defines some rules to generate all
the files needed. But actually it does not seem to be enough, for
example on OSX this would fail to compile because it cannot find the
postgres binary in src/backend/postgres. Does somebody have an idea
what this is about? It seems that we have two problems here.

Yeah, on OS X, building regress.so (or any other loadable extension)
will fail unless you built the backend first:

gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -Wno-unused-command-line-argument -g -O2 -bundle -multiply_defined suppress -Wl,-undefined,dynamic_lookup -o regress.so regress.o -L../../../src/port -L../../../src/common -Wl,-dead_strip_dylibs -bundle_loader ../../../src/backend/postgres
ld: file not found: ../../../src/backend/postgres

The reason is you need the "-bundle_loader postgres" option, because
OS X's linker is pickier about unresolved symbols than Linux's.

However, that is not a 9.6 regression: that's never worked on OS X, or at
least not since we started using -bundle_loader.

I do not think that we want the makefiles to enforce this build
dependency, as that would completely destroy any speed advantage of trying
to build just the particular .so. Maybe we could enforce the dependency
just on OS X, but since we haven't gotten complaints from people trying
to build like this on OS X, I doubt it's worth the trouble.

So my inclination is to fix the include-file issue and call it good.
In any case, if someone did want to deal with making the -bundle_loader
calls safer, it would be material for a separate patch IMO.

regards, tom lane

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

Michael Paquier <michael.paquier@gmail.com> writes:

Yes that would be indeed cleaner this way. I have poked a bit at that
and finished with the attached that defines some rules to generate all
the files needed.

I made some mostly-cosmetic changes to this and pushed it. One thing
to note is that it seemed to me you'd broken the rule for schemapg.h:
by removing the phony target, I think you removed the behavior that
we'd always go and recheck schemapg.h's dependencies. To do it correctly
without that target, we'd need src/backend/Makefile to know all of those
dependencies, duplicating the rather long list in catalog/Makefile.

regards, tom lane

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

On Sat, Jul 2, 2016 at 4:13 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Michael Paquier <michael.paquier@gmail.com> writes:

Yes that would be indeed cleaner this way. I have poked a bit at that
and finished with the attached that defines some rules to generate all
the files needed.

I made some mostly-cosmetic changes to this and pushed it. One thing
to note is that it seemed to me you'd broken the rule for schemapg.h:
by removing the phony target, I think you removed the behavior that
we'd always go and recheck schemapg.h's dependencies. To do it correctly
without that target, we'd need src/backend/Makefile to know all of those
dependencies, duplicating the rather long list in catalog/Makefile.

Thanks. Yes the bug that I was mentioning yesterday was that I missed
one place where the target of schemapg.h was referenced, and I didn't
take the time to fully cover this set of dependencies..
--
Michael

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

Re: Tom Lane 2016-07-01 <26357.1467400438@sss.pgh.pa.us>

I made some mostly-cosmetic changes to this and pushed it.

I confirm that Debian's out-of-tree python3 build works now when
invoked directly in the relevant plpython/hstore_plpython
subdirectories. Thanks!

Christoph

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