Thread: Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.

Robert Haas <robertmhaas@gmail.com> writes:
> Apparently, 'make world' does not build worker_spi.  I thought 'make
> world' was supposed to build everything?

You'd have thunk, yeah.  It looks like the issue is that src/Makefile
is selective about recursing into certain subdirectories of test/,
but mostly not test/ itself.  src/test/Makefile naively believes it's
in charge, though.  Probably that logic ought to get shoved down one
level, and then adjusted so that src/test/modules gets built by "all".
Or else teach top-level "make world" to do "make all" in src/test/,
but that seems like it's just doubling down on confusing interconnections.
        regards, tom lane

On Tue, Oct 4, 2016 at 11:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> Apparently, 'make world' does not build worker_spi.  I thought 'make
>> world' was supposed to build everything?
>
> You'd have thunk, yeah.  It looks like the issue is that src/Makefile
> is selective about recursing into certain subdirectories of test/,
> but mostly not test/ itself.  src/test/Makefile naively believes it's
> in charge, though.  Probably that logic ought to get shoved down one
> level, and then adjusted so that src/test/modules gets built by "all".
> Or else teach top-level "make world" to do "make all" in src/test/,
> but that seems like it's just doubling down on confusing interconnections.

I think this confusion probably resulted from the move of certain
things from contrib to src/test/modules, although that might be wrong.
At any rate, just as contrib isn't built by 'make all', one could
argue that src/test/modules shouldn't be built, either.  Then again,
if 'make check' depends on it, maybe it needs to be.  Either way,
'make world' should certainly build it, I think.

Interesting, 'make check-world' tried to compile test_shm_mq, so I
caught the WaitLatch call there before committing.  I guess it only
did that because src/test/modules/test_shm_mq has a regression test,
though.  work_spi does not, so even though they're both under
src/test/modules, one eventually got built and the other did not.
That's not so great.

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

Robert Haas wrote:
> On Tue, Oct 4, 2016 at 11:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Robert Haas <robertmhaas@gmail.com> writes:
> >> Apparently, 'make world' does not build worker_spi.  I thought 'make
> >> world' was supposed to build everything?
> >
> > You'd have thunk, yeah.  It looks like the issue is that src/Makefile
> > is selective about recursing into certain subdirectories of test/,
> > but mostly not test/ itself.  src/test/Makefile naively believes it's
> > in charge, though.  Probably that logic ought to get shoved down one
> > level, and then adjusted so that src/test/modules gets built by "all".
> > Or else teach top-level "make world" to do "make all" in src/test/,
> > but that seems like it's just doubling down on confusing interconnections.
> 
> I think this confusion probably resulted from the move of certain
> things from contrib to src/test/modules, although that might be wrong.

I think you're right -- I tried to avoid recursing into src/test/modules
as much as possible when doing the move, so that it'd not be a bother
when someone just wants to build/package the server.  But obviously for
the purposes of testing the overall build that is the wrong thing to do,
and I agree that we should revisit those decisions.

> At any rate, just as contrib isn't built by 'make all', one could
> argue that src/test/modules shouldn't be built, either.  Then again,
> if 'make check' depends on it, maybe it needs to be.

Hmm, does it?  AFAICS (toplevel GNUmakefile) only src/test/regress is
recursed onto for "make check".

> Either way, 'make world' should certainly build it, I think.

I can buy that.

> Interesting, 'make check-world' tried to compile test_shm_mq, so I
> caught the WaitLatch call there before committing.  I guess it only
> did that because src/test/modules/test_shm_mq has a regression test,
> though.  work_spi does not, so even though they're both under
> src/test/modules, one eventually got built and the other did not.
> That's not so great.

This sounds broken to me, actually.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 10/4/16 11:29 AM, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> Apparently, 'make world' does not build worker_spi.  I thought 'make
>> world' was supposed to build everything?
> 
> You'd have thunk, yeah.  It looks like the issue is that src/Makefile
> is selective about recursing into certain subdirectories of test/,
> but mostly not test/ itself.  src/test/Makefile naively believes it's
> in charge, though.  Probably that logic ought to get shoved down one
> level, and then adjusted so that src/test/modules gets built by "all".
> Or else teach top-level "make world" to do "make all" in src/test/,
> but that seems like it's just doubling down on confusing interconnections.

We generally don't build test code during make world.

The reason src/Makefile does that is probably because pg_regress is part
of the installation.

So this looks more or less correct to me.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 10/4/16 11:29 AM, Tom Lane wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> Apparently, 'make world' does not build worker_spi.  I thought 'make
>>> world' was supposed to build everything?

>> You'd have thunk, yeah.  It looks like the issue is that src/Makefile
>> is selective about recursing into certain subdirectories of test/,
>> but mostly not test/ itself.  src/test/Makefile naively believes it's
>> in charge, though.  Probably that logic ought to get shoved down one
>> level, and then adjusted so that src/test/modules gets built by "all".
>> Or else teach top-level "make world" to do "make all" in src/test/,
>> but that seems like it's just doubling down on confusing interconnections.

> We generally don't build test code during make world.

> The reason src/Makefile does that is probably because pg_regress is part
> of the installation.

> So this looks more or less correct to me.

Even if you think the behavior is correct (I'm not convinced), the
implementation is certainly confusing.  I don't like the way that
src/test/Makefile gets bypassed for decisions about which of its
subdirectories get built for what.  Any normal person would expect
that src/test/Makefile is what determines that.
        regards, tom lane

On Wed, Oct 12, 2016 at 8:18 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 10/4/16 11:29 AM, Tom Lane wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> Apparently, 'make world' does not build worker_spi.  I thought 'make
>>> world' was supposed to build everything?
>>
>> You'd have thunk, yeah.  It looks like the issue is that src/Makefile
>> is selective about recursing into certain subdirectories of test/,
>> but mostly not test/ itself.  src/test/Makefile naively believes it's
>> in charge, though.  Probably that logic ought to get shoved down one
>> level, and then adjusted so that src/test/modules gets built by "all".
>> Or else teach top-level "make world" to do "make all" in src/test/,
>> but that seems like it's just doubling down on confusing interconnections.
>
> We generally don't build test code during make world.
>
> The reason src/Makefile does that is probably because pg_regress is part
> of the installation.
>
> So this looks more or less correct to me.

But it's annoying to push code and have it break the buildfarm when
you already did 'make world' and 'make check-world'.  What target
should I be using?

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

On 2016-10-12 11:18:57 -0400, Peter Eisentraut wrote:
> On 10/4/16 11:29 AM, Tom Lane wrote:
> > Robert Haas <robertmhaas@gmail.com> writes:
> >> Apparently, 'make world' does not build worker_spi.  I thought 'make
> >> world' was supposed to build everything?
> > 
> > You'd have thunk, yeah.  It looks like the issue is that src/Makefile
> > is selective about recursing into certain subdirectories of test/,
> > but mostly not test/ itself.  src/test/Makefile naively believes it's
> > in charge, though.  Probably that logic ought to get shoved down one
> > level, and then adjusted so that src/test/modules gets built by "all".
> > Or else teach top-level "make world" to do "make all" in src/test/,
> > but that seems like it's just doubling down on confusing interconnections.
> 
> We generally don't build test code during make world.

FWIW, I find that quite annoying. I want to run make world with parallelism so
I can run make world afterwards with as little unnecessary
unparallelized work. And since the latter takes just about forever, any
errors visible earlier are good.

What are we gaining by this behaviour?

Andres

On Thu, Oct 13, 2016 at 8:38 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2016-10-12 11:18:57 -0400, Peter Eisentraut wrote:
>> On 10/4/16 11:29 AM, Tom Lane wrote:
>> > Robert Haas <robertmhaas@gmail.com> writes:
>> >> Apparently, 'make world' does not build worker_spi.  I thought 'make
>> >> world' was supposed to build everything?
>> >
>> > You'd have thunk, yeah.  It looks like the issue is that src/Makefile
>> > is selective about recursing into certain subdirectories of test/,
>> > but mostly not test/ itself.  src/test/Makefile naively believes it's
>> > in charge, though.  Probably that logic ought to get shoved down one
>> > level, and then adjusted so that src/test/modules gets built by "all".
>> > Or else teach top-level "make world" to do "make all" in src/test/,
>> > but that seems like it's just doubling down on confusing interconnections.
>>
>> We generally don't build test code during make world.
>
> FWIW, I find that quite annoying. I want to run make world with parallelism so
> I can run make world afterwards with as little unnecessary
> unparallelized work. And since the latter takes just about forever, any
> errors visible earlier are good.
>
> What are we gaining by this behaviour?

Personally, I have been trapped by the fact that worker_spi does not
get built when doing a simple check-world recently... So +1 for just
building it when running check-world. We gain nothing with the current
behavior except alarms on the buildfarm.
-- 
Michael

On 10/12/16 12:04 PM, Robert Haas wrote:
> But it's annoying to push code and have it break the buildfarm when
> you already did 'make world' and 'make check-world'.  What target
> should I be using?

I don't know why worker_spi is not tested by check-world.  It should
clearly do that.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 10/12/16 7:38 PM, Andres Freund wrote:
>> We generally don't build test code during make world.
> FWIW, I find that quite annoying. I want to run make world with parallelism so
> I can run make world afterwards with as little unnecessary
> unparallelized work. And since the latter takes just about forever, any
> errors visible earlier are good.
> 
> What are we gaining by this behaviour?

Well, the purpose of make all or make world is to build the things that
are to be installed by make install or make install-world, which is the
stuff that users want to use.  The purpose is not necessarily to build
stuff for the amusement of developers.  Remember that we used to have
some dusty corners under src/test/, so "build everything no matter what"
is/was not a clearly better strategy.  Also, some test code used to take
a relatively long time to build, which led to some level of annoyance.

It might be worth reviewing this approach, but that's what it is.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Oct 13, 2016 at 9:23 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 10/12/16 7:38 PM, Andres Freund wrote:
>>> We generally don't build test code during make world.
>> FWIW, I find that quite annoying. I want to run make world with parallelism so
>> I can run make world afterwards with as little unnecessary
>> unparallelized work. And since the latter takes just about forever, any
>> errors visible earlier are good.
>>
>> What are we gaining by this behaviour?
>
> Well, the purpose of make all or make world is to build the things that
> are to be installed by make install or make install-world, which is the
> stuff that users want to use.  The purpose is not necessarily to build
> stuff for the amusement of developers.  Remember that we used to have
> some dusty corners under src/test/, so "build everything no matter what"
> is/was not a clearly better strategy.  Also, some test code used to take
> a relatively long time to build, which led to some level of annoyance.
>
> It might be worth reviewing this approach, but that's what it is.

Well, I think what Tom, Andres, Michael, and I are saying is precisely
that we should review this approach and revise it so that 'make world'
builds everything.  Or else add 'make universe' to really build
everything, but personally I think that's an unnecessary complication.
The difference between 'make world' and a full build is probably not
enough that many people are going to care much about the difference in
compilation time.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Oct 13, 2016 at 9:23 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> Well, the purpose of make all or make world is to build the things that
>> are to be installed by make install or make install-world, which is the
>> stuff that users want to use.

> Well, I think what Tom, Andres, Michael, and I are saying is precisely
> that we should review this approach and revise it so that 'make world'
> builds everything.  Or else add 'make universe' to really build
> everything, but personally I think that's an unnecessary complication.

Not sure.  There's something to be said for the equivalence Peter
proposes above.  What you actually wanted, as I understood it, was
that "make world" plus "make check-world" should test absolutely
everything.  I don't have a problem with the idea that some bits
of test scaffolding don't get built until you do "make check-world".
The real problem here is that "make check-world" missed some tests,
which Peter agrees is a bug.

Andres complained about insufficient parallelism in the "check"
target, but that seems to me to be something to address separately;
redefining the set of actions to be taken is not the way to fix that.
        regards, tom lane

On Fri, Oct 14, 2016 at 8:33 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Thu, Oct 13, 2016 at 9:23 PM, Peter Eisentraut
>> <peter.eisentraut@2ndquadrant.com> wrote:
>>> Well, the purpose of make all or make world is to build the things that
>>> are to be installed by make install or make install-world, which is the
>>> stuff that users want to use.
>
>> Well, I think what Tom, Andres, Michael, and I are saying is precisely
>> that we should review this approach and revise it so that 'make world'
>> builds everything.  Or else add 'make universe' to really build
>> everything, but personally I think that's an unnecessary complication.
>
> Not sure.  There's something to be said for the equivalence Peter
> proposes above.  What you actually wanted, as I understood it, was
> that "make world" plus "make check-world" should test absolutely
> everything.  I don't have a problem with the idea that some bits
> of test scaffolding don't get built until you do "make check-world".
> The real problem here is that "make check-world" missed some tests,
> which Peter agrees is a bug.

No, the problem is that worker_spi has no tests, so 'make check-world'
never tries to build it at all.  Something in the buildfarm does cause
it to get built, though.

> Andres complained about insufficient parallelism in the "check"
> target, but that seems to me to be something to address separately;
> redefining the set of actions to be taken is not the way to fix that.

Sure, that's another issue.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Oct 14, 2016 at 8:33 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Not sure.  There's something to be said for the equivalence Peter
>> proposes above.  What you actually wanted, as I understood it, was
>> that "make world" plus "make check-world" should test absolutely
>> everything.  I don't have a problem with the idea that some bits
>> of test scaffolding don't get built until you do "make check-world".
>> The real problem here is that "make check-world" missed some tests,
>> which Peter agrees is a bug.

> No, the problem is that worker_spi has no tests, so 'make check-world'
> never tries to build it at all.  Something in the buildfarm does cause
> it to get built, though.

Well, if it has no tests *and* it's not getting installed, what's
the point of having it at all?
        regards, tom lane

On Fri, Oct 14, 2016 at 9:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Fri, Oct 14, 2016 at 8:33 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Not sure.  There's something to be said for the equivalence Peter
>>> proposes above.  What you actually wanted, as I understood it, was
>>> that "make world" plus "make check-world" should test absolutely
>>> everything.  I don't have a problem with the idea that some bits
>>> of test scaffolding don't get built until you do "make check-world".
>>> The real problem here is that "make check-world" missed some tests,
>>> which Peter agrees is a bug.
>
>> No, the problem is that worker_spi has no tests, so 'make check-world'
>> never tries to build it at all.  Something in the buildfarm does cause
>> it to get built, though.
>
> Well, if it has no tests *and* it's not getting installed, what's
> the point of having it at all?

It's intended as a demonstration of stuff you could do with background
workers.  Perhaps that begs the question of why Alvaro included it in
the set of things that got moved from contrib to src/test/modules, but
I'm still of the opinion that we should build everything in
src/test/modules when the user does 'make world', whether it has tests
defined or not.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Oct 14, 2016 at 9:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Well, if it has no tests *and* it's not getting installed, what's
>> the point of having it at all?

> It's intended as a demonstration of stuff you could do with background
> workers.  Perhaps that begs the question of why Alvaro included it in
> the set of things that got moved from contrib to src/test/modules, but
> I'm still of the opinion that we should build everything in
> src/test/modules when the user does 'make world', whether it has tests
> defined or not.

TBH, I can't muster much sympathy for that position.  Make a test case
for it, and the problem goes away, not to mention that confidence in
whether it actually works (not just compiles) goes up a lot.
        regards, tom lane

On Fri, Oct 14, 2016 at 9:43 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Fri, Oct 14, 2016 at 9:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Well, if it has no tests *and* it's not getting installed, what's
>>> the point of having it at all?
>
>> It's intended as a demonstration of stuff you could do with background
>> workers.  Perhaps that begs the question of why Alvaro included it in
>> the set of things that got moved from contrib to src/test/modules, but
>> I'm still of the opinion that we should build everything in
>> src/test/modules when the user does 'make world', whether it has tests
>> defined or not.
>
> TBH, I can't muster much sympathy for that position.  Make a test case
> for it, and the problem goes away, not to mention that confidence in
> whether it actually works (not just compiles) goes up a lot.

I'm not sure there's an easy way to test it via pg_regress, but if
somebody can come up with something, sure.  But why stick to a rule
that is inconvenient for no real benefit?  Compiling everything in
src/test/modules when someone runs 'make check-world' would take a
handful of seconds and prevent developer errors like the one that
started this thread.  That seems like a slam-dunk from here,
regardless of anything else.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Oct 14, 2016 at 9:43 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> TBH, I can't muster much sympathy for that position.  Make a test case
>> for it, and the problem goes away, not to mention that confidence in
>> whether it actually works (not just compiles) goes up a lot.

> I'm not sure there's an easy way to test it via pg_regress, but if
> somebody can come up with something, sure.  But why stick to a rule
> that is inconvenient for no real benefit?  Compiling everything in
> src/test/modules when someone runs 'make check-world' would take a
> handful of seconds and prevent developer errors like the one that
> started this thread.  That seems like a slam-dunk from here,
> regardless of anything else.

I guess what I'm having a problem with is something that lives under
src/test/ and is not in fact intended as a test.  If you're not interested
in making it into a live test, it's in the wrong place.  You might as
well complain that you put C code under doc/src/sgml/ and it didn't get
compiled.

One idea is to put "check: all" into its makefile, if there's no prospect
of check doing something more than that.
        regards, tom lane

On Fri, Oct 14, 2016 at 11:00 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Fri, Oct 14, 2016 at 9:43 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> TBH, I can't muster much sympathy for that position.  Make a test case
>>> for it, and the problem goes away, not to mention that confidence in
>>> whether it actually works (not just compiles) goes up a lot.
>
>> I'm not sure there's an easy way to test it via pg_regress, but if
>> somebody can come up with something, sure.  But why stick to a rule
>> that is inconvenient for no real benefit?  Compiling everything in
>> src/test/modules when someone runs 'make check-world' would take a
>> handful of seconds and prevent developer errors like the one that
>> started this thread.  That seems like a slam-dunk from here,
>> regardless of anything else.
>
> I guess what I'm having a problem with is something that lives under
> src/test/ and is not in fact intended as a test.  If you're not interested
> in making it into a live test, it's in the wrong place.  You might as
> well complain that you put C code under doc/src/sgml/ and it didn't get
> compiled.

Well, we could move worker_spi back to contrib.

> One idea is to put "check: all" into its makefile, if there's no prospect
> of check doing something more than that.

That'd certainly be better than doing nothing.

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