pgsql: Extend framework from commit 53be0b1ad to report latch waits.

Started by Robert Haasover 9 years ago25 messages
#1Robert Haas
rhaas@postgresql.org

Extend framework from commit 53be0b1ad to report latch waits.

WaitLatch, WaitLatchOrSocket, and WaitEventSetWait now taken an
additional wait_event_info parameter; legal values are defined in
pgstat.h. This makes it possible to uniquely identify every point in
the core code where we are waiting for a latch; extensions can pass
WAIT_EXTENSION.

Because latches were the major wait primitive not previously covered
by this patch, it is now possible to see information in
pg_stat_activity on a large number of important wait events not
previously addressed, such as ClientRead, ClientWrite, and SyncRep.

Unfortunately, many of the wait events added by this patch will fail
to appear in pg_stat_activity because they're only used in background
processes which don't currently appear in pg_stat_activity. We should
fix this either by creating a separate view for such information, or
else by deciding to include them in pg_stat_activity after all.

Michael Paquier and Robert Haas, reviewed by Alexander Korotkov and
Thomas Munro.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/6f3bd98ebfc008cbd676da777bb0b2376c4c4bfa

Modified Files
--------------
contrib/postgres_fdw/connection.c | 3 +-
doc/src/sgml/monitoring.sgml | 169 ++++++++++++++++++++++++
src/backend/access/transam/parallel.c | 4 +-
src/backend/access/transam/xlog.c | 7 +-
src/backend/executor/nodeGather.c | 3 +-
src/backend/libpq/be-secure-openssl.c | 4 +-
src/backend/libpq/be-secure.c | 7 +-
src/backend/libpq/pqmq.c | 4 +-
src/backend/postmaster/autovacuum.c | 3 +-
src/backend/postmaster/bgworker.c | 7 +-
src/backend/postmaster/bgwriter.c | 5 +-
src/backend/postmaster/checkpointer.c | 3 +-
src/backend/postmaster/pgarch.c | 3 +-
src/backend/postmaster/pgstat.c | 236 ++++++++++++++++++++++++++++++++--
src/backend/postmaster/syslogger.c | 4 +-
src/backend/postmaster/walwriter.c | 3 +-
src/backend/replication/basebackup.c | 3 +-
src/backend/replication/syncrep.c | 4 +-
src/backend/replication/walreceiver.c | 7 +-
src/backend/replication/walsender.c | 9 +-
src/backend/storage/buffer/bufmgr.c | 7 +-
src/backend/storage/ipc/latch.c | 18 ++-
src/backend/storage/ipc/shm_mq.c | 7 +-
src/backend/storage/ipc/standby.c | 5 +-
src/backend/storage/lmgr/lock.c | 3 -
src/backend/storage/lmgr/lwlock.c | 6 +-
src/backend/storage/lmgr/predicate.c | 3 +-
src/backend/storage/lmgr/proc.c | 8 +-
src/backend/utils/adt/misc.c | 4 +-
src/include/pgstat.h | 99 ++++++++++++--
src/include/storage/latch.h | 9 +-
src/include/storage/lwlock.h | 2 +-
src/include/storage/proc.h | 2 +-
src/test/modules/test_shm_mq/setup.c | 3 +-
src/test/modules/test_shm_mq/test.c | 3 +-
35 files changed, 584 insertions(+), 83 deletions(-)

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

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#1)
Re: pgsql: Extend framework from commit 53be0b1ad to report latch waits.

Robert Haas <rhaas@postgresql.org> writes:

Extend framework from commit 53be0b1ad to report latch waits.

Buildfarm doesn't like this one bit.

regards, tom lane

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

#3Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#2)
Re: pgsql: Extend framework from commit 53be0b1ad to report latch waits.

On Tue, Oct 4, 2016 at 11:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <rhaas@postgresql.org> writes:

Extend framework from commit 53be0b1ad to report latch waits.

Buildfarm doesn't like this one bit.

Hmm, and for good reason. But I swear I ran 'make check-world' before
pushing. Argh.

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

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

#4Robert Haas
robertmhaas@gmail.com
In reply to: Robert Haas (#3)
Re: pgsql: Extend framework from commit 53be0b1ad to report latch waits.

On Tue, Oct 4, 2016 at 11:17 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Oct 4, 2016 at 11:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <rhaas@postgresql.org> writes:

Extend framework from commit 53be0b1ad to report latch waits.

Buildfarm doesn't like this one bit.

Hmm, and for good reason. But I swear I ran 'make check-world' before
pushing. Argh.

Apparently, 'make world' does not build worker_spi. I thought 'make
world' was supposed to build everything?

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

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

#5Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#4)
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

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

#6Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#5)
Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.

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

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

#7Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Robert Haas (#6)
Re: Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.

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

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

#8Amit Kapila
amit.kapila16@gmail.com
In reply to: Robert Haas (#1)
Re: pgsql: Extend framework from commit 53be0b1ad to report latch waits.

On Tue, Oct 4, 2016 at 8:32 PM, Robert Haas <rhaas@postgresql.org> wrote:

Extend framework from commit 53be0b1ad to report latch waits.

I am facing windows below compilation error:

1>pgstat.h(726): warning C4005: 'WAIT_TIMEOUT' : macro redefinition
1>c:\Program Files (x86)\Microsoft
SDKs\Windows\v7.0A\include\winerror.h(1797) : see previous definition
of 'WAIT_TIMEOUT'

It seems windows already have a define for WAIT_TIMEOUT. Do we want
to rename it to WAIT_TIMEEXPIRED or WAIT_EXPIRETIME?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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

#9Amit Kapila
amit.kapila16@gmail.com
In reply to: Amit Kapila (#8)
Re: pgsql: Extend framework from commit 53be0b1ad to report latch waits.

On Wed, Oct 5, 2016 at 5:53 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Oct 4, 2016 at 8:32 PM, Robert Haas <rhaas@postgresql.org> wrote:

Extend framework from commit 53be0b1ad to report latch waits.

I am facing windows below compilation error:

1>pgstat.h(726): warning C4005: 'WAIT_TIMEOUT' : macro redefinition
1>c:\Program Files (x86)\Microsoft
SDKs\Windows\v7.0A\include\winerror.h(1797) : see previous definition
of 'WAIT_TIMEOUT'

It seems windows already have a define for WAIT_TIMEOUT. Do we want
to rename it to WAIT_TIMEEXPIRED or WAIT_EXPIRETIME?

Recent commit d2ce38e204583bc444eebffd800d492cf56e3b38 should fix this problem.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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

#10Peter Eisentraut
peter.eisentraut@2ndquadrant.com
In reply to: Tom Lane (#5)
Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.

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

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

#11Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Eisentraut (#10)
Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.

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

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

#12Robert Haas
robertmhaas@gmail.com
In reply to: Peter Eisentraut (#10)
Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.

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

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

#13Andres Freund
andres@anarazel.de
In reply to: Peter Eisentraut (#10)
Re: Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.

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

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

#14Michael Paquier
michael.paquier@gmail.com
In reply to: Andres Freund (#13)
Re: Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.

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

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

#15Peter Eisentraut
peter.eisentraut@2ndquadrant.com
In reply to: Robert Haas (#12)
Re: Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.

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

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

#16Peter Eisentraut
peter.eisentraut@2ndquadrant.com
In reply to: Andres Freund (#13)
Re: Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.

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

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

#17Robert Haas
robertmhaas@gmail.com
In reply to: Peter Eisentraut (#16)
Re: Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.

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

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

#18Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#17)
Re: Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.

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

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

#19Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#18)
Re: Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.

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

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

#20Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#19)
Re: Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.

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

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

#21Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#20)
Re: Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.

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

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

#22Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#21)
Re: Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.

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

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

#23Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#22)
Re: Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.

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

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

#24Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#23)
Re: Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.

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

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

#25Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#24)
Re: Re: [COMMITTERS] pgsql: Extend framework from commit 53be0b1ad to report latch waits.

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

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