Re: [HACKERS] Broken handling of lwlocknames.h

[Christoph Berg]

Re: Tom Lane 2016-07-01 <26357.1467400...@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

Re: [HACKERS] Broken handling of lwlocknames.h

[Michael Paquier]

On Sat, Jul 2, 2016 at 4:13 AM, Tom Lane  wrote:
> Michael Paquier  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: [HACKERS] Broken handling of lwlocknames.h

[Tom Lane]

Michael Paquier  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

Re: [HACKERS] Broken handling of lwlocknames.h

[Tom Lane]

Michael Paquier  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

Re: [HACKERS] Broken handling of lwlocknames.h

[Michael Paquier]

On Fri, Jul 1, 2016 at 10:29 PM, Tom Lane  wrote:
> Robert Haas  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

Re: [HACKERS] Broken handling of lwlocknames.h

[Tom Lane]

Robert Haas  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

Re: [HACKERS] Broken handling of lwlocknames.h

[Robert Haas]

On Fri, Jul 1, 2016 at 12:09 AM, Noah Misch  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] 
> http://www.postgresql.org/message-id/20160527025039.ga447...@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

Re: [HACKERS] Broken handling of lwlocknames.h

[Noah Misch]

On Tue, Jun 28, 2016 at 12:26:00PM +0900, Michael Paquier wrote:
> On Tue, Jun 28, 2016 at 3:22 AM, Christoph Berg  wrote:
> > Re: Tom Lane 2016-06-27 <31398.1467036...@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 https://www.postgresql.org/message-id/20150916200959.gb32...@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] 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] 
http://www.postgresql.org/message-id/20160527025039.ga447...@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

Re: [HACKERS] Broken handling of lwlocknames.h

[Michael Paquier]

On Tue, Jun 28, 2016 at 3:22 AM, Christoph Berg  wrote:
> Re: Tom Lane 2016-06-27 <31398.1467036...@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 https://www.postgresql.org/message-id/20150916200959.gb32...@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


makefile-generate.patch
Description: invalid/octet-stream

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

Re: [HACKERS] Broken handling of lwlocknames.h

[Christoph Berg]

Re: Tom Lane 2016-06-27 <31398.1467036...@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 https://www.postgresql.org/message-id/20150916200959.gb32...@msg.df7cb.de

So another +1 from a packagers perspective.

Christoph


signature.asc
Description: PGP signature