Обсуждение: 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
Re: Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.
От
Alvaro Herrera
Дата:
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
Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.
От
Peter Eisentraut
Дата:
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
Re: Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.
От
Andres Freund
Дата:
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
Re: Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.
От
Michael Paquier
Дата:
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
Re: Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.
От
Peter Eisentraut
Дата:
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
Re: Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.
От
Peter Eisentraut
Дата:
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