pgsql: Fix inadequacies in recently added wait events

Fix inadequacies in recently added wait events

In commit 9915de6c1cb2, we introduced a new wait point for replication
slots and incorrectly labelled it as wait event PG_WAIT_LOCK. That's
wrong, so invent an appropriate new wait event instead, and document it
properly.

While at it, fix numerous other problems in the vicinity:
- two different walreceiver wait events were being mixed up in a single
wait event (which wasn't documented either); split it out so that they
can be distinguished, and document the new events properly.

- ParallelBitmapPopulate was documented but didn't exist.

- ParallelBitmapScan was not documented (I think this should be called
"ParallelBitmapScanInit" instead.)

- Logical replication wait events weren't documented

- various symbols had been added in dartboard order in various places.
Put them in alphabetical order instead, as was originally intended.

Discussion: https://postgr.es/m/[email protected]

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/030273b7ea468ed4b3073dfd1f2ad88e3129df6a

Modified Files
--------------
doc/src/sgml/monitoring.sgml | 32 +++++++++++++++++--
src/backend/postmaster/pgstat.c | 36 +++++++++++++---------
.../libpqwalreceiver/libpqwalreceiver.c | 4 +--
src/backend/replication/slot.c | 3 +-
src/include/pgstat.h | 16 +++++-----
5 files changed, 64 insertions(+), 27 deletions(-)

On Wed, Aug 9, 2017 at 7:39 AM, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> Fix inadequacies in recently added wait events
>
> In commit 9915de6c1cb2, we introduced a new wait point for replication
> slots and incorrectly labelled it as wait event PG_WAIT_LOCK. That's
> wrong, so invent an appropriate new wait event instead, and document it
> properly.
>
> While at it, fix numerous other problems in the vicinity:
> - two different walreceiver wait events were being mixed up in a single
> wait event (which wasn't documented either); split it out so that they
> can be distinguished, and document the new events properly.
>
> - ParallelBitmapPopulate was documented but didn't exist.
>
> - ParallelBitmapScan was not documented (I think this should be called
> "ParallelBitmapScanInit" instead.)
>
> - Logical replication wait events weren't documented
>
> - various symbols had been added in dartboard order in various places.
> Put them in alphabetical order instead, as was originally intended.

All of the above seem like good candidates for a checker script in
src/tools/check_XXX.pl, a bit like the others I've talked about
recently [1][2].

[1] https://www.postgresql.org/message-id/CAEepm=3da6aJYxiUk03pAKt1t7eGWAnn9CajC773Ct1rDPO-Hw@mail.gmail.com
[2] https://www.postgresql.org/message-id/CAEepm=0B7yM9MZSviq1d-hnt4KoaRVeJvSfAyVfykNV-pVDqug@mail.gmail.com

--
Thomas Munro
http://www.enterprisedb.com

Thread moved to -hackers.

Thomas Munro wrote:
> On Wed, Aug 9, 2017 at 7:39 AM, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:

> > While at it, fix numerous other problems in the vicinity:

> All of the above seem like good candidates for a checker script in
> src/tools/check_XXX.pl, a bit like the others I've talked about
> recently [1][2].

Yeah, that's one idea, but I think it'd be better to have all these
things be generated content from a single "wait_events.txt" file, like
src/backend/utils/errcodes.txt which gives birth to a whole host of
different files.

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

On Tue, Aug 8, 2017 at 11:35 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Thread moved to -hackers.
>
> Thomas Munro wrote:
>> On Wed, Aug 9, 2017 at 7:39 AM, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
>> > While at it, fix numerous other problems in the vicinity:
>
>> All of the above seem like good candidates for a checker script in
>> src/tools/check_XXX.pl, a bit like the others I've talked about
>> recently [1][2].
>
> Yeah, that's one idea, but I think it'd be better to have all these
> things be generated content from a single "wait_events.txt" file, like
> src/backend/utils/errcodes.txt which gives birth to a whole host of
> different files.

I would prefer that. Check scripts would tend to be never run. So is
there somebody willing to work on that? Or should I as I am
responsible to increasing the number of wait events?
--
Michael

On Wed, Aug 9, 2017 at 6:42 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Tue, Aug 8, 2017 at 11:35 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
>>> All of the above seem like good candidates for a checker script in
>>> src/tools/check_XXX.pl, a bit like the others I've talked about
>>> recently [1][2].
>>
>> Yeah, that's one idea, but I think it'd be better to have all these
>> things be generated content from a single "wait_events.txt" file, like
>> src/backend/utils/errcodes.txt which gives birth to a whole host of
>> different files.
>
> I would prefer that. Check scripts would tend to be never run. So is
> there somebody willing to work on that? Or should I as I am
> responsible to increasing the number of wait events?

Yeah, that may well be a better idea in this case. On the other hand
it wouldn't catch multiple users of the same wait event, so both
things could be useful.

As for whether hypothetical check scripts would ever be run, I was
thinking we should stick them under some make target that developers
run all the time anyway -- perhaps "check". Shouldn't we catch simple
mechanically detectable problems as early in the pipeline as possible?

--
Thomas Munro
http://www.enterprisedb.com

Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> writes:
> As for whether hypothetical check scripts would ever be run, I was
> thinking we should stick them under some make target that developers
> run all the time anyway -- perhaps "check". Shouldn't we catch simple
> mechanically detectable problems as early in the pipeline as possible?

Adding overhead to every developer's every test cycle doesn't sound
like a win. Possibly a reasonable compromise would be to have some
buildfarm members running this check.

regards, tom lane

On Wed, Aug 9, 2017 at 10:14 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> writes:
>> As for whether hypothetical check scripts would ever be run, I was
>> thinking we should stick them under some make target that developers
>> run all the time anyway -- perhaps "check". Shouldn't we catch simple
>> mechanically detectable problems as early in the pipeline as possible?
>
> Adding overhead to every developer's every test cycle doesn't sound
> like a win.

If it takes 100ms, nobody's gonna notice.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Aug 9, 2017 at 10:14 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> writes:
>>> As for whether hypothetical check scripts would ever be run, I was
>>> thinking we should stick them under some make target that developers
>>> run all the time anyway -- perhaps "check". Shouldn't we catch simple
>>> mechanically detectable problems as early in the pipeline as possible?

>> Adding overhead to every developer's every test cycle doesn't sound
>> like a win.

> If it takes 100ms, nobody's gonna notice.

I doubt running a perl script that analyzes the entire backend source
code is gonna take 100ms.

regards, tom lane

On Wed, Aug 09, 2017 at 10:56:50AM -0400, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Wed, Aug 9, 2017 at 10:14 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> writes:
> >>> As for whether hypothetical check scripts would ever be run, I
> >>> was thinking we should stick them under some make target that
> >>> developers run all the time anyway -- perhaps "check".
> >>> Shouldn't we catch simple mechanically detectable problems as
> >>> early in the pipeline as possible?
>
> >> Adding overhead to every developer's every test cycle doesn't
> >> sound like a win.
>
> > If it takes 100ms, nobody's gonna notice.
>
> I doubt running a perl script that analyzes the entire backend
> source code is gonna take 100ms.

What would be a reasonable maximum amount of time for such a check to take?

Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

This thread is surprising. If we generate the few lines of code being
in trouble, we don't need any checker script, so I don't see why we'd go
the route of the checker script instead.

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

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> This thread is surprising. If we generate the few lines of code being
> in trouble, we don't need any checker script, so I don't see why we'd go
> the route of the checker script instead.

I think generating whatever we can from a single authoritative file
is indeed a good idea. But I had the impression that people also wanted
to enforce a rule about "only one use of each wait event name", which'd
require a checker script, no? (I'm not really convinced that we need
such a rule, fwiw.)

regards, tom lane

Tom Lane wrote:

> I think generating whatever we can from a single authoritative file
> is indeed a good idea.

Yay.

> But I had the impression that people also wanted to enforce a rule
> about "only one use of each wait event name", which'd require a
> checker script, no? (I'm not really convinced that we need such a
> rule, fwiw.)

I'm not convinced of that, either. Of the possible problems in the
area, that seems the lesser one.

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

On Wed, Aug 9, 2017 at 9:25 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> Tom Lane wrote:
>> I think generating whatever we can from a single authoritative file
>> is indeed a good idea.
>
> Yay.

Indeed.

>> But I had the impression that people also wanted to enforce a rule
>> about "only one use of each wait event name", which'd require a
>> checker script, no? (I'm not really convinced that we need such a
>> rule, fwiw.)
>
> I'm not convinced of that, either. Of the possible problems in the
> area, that seems the lesser one.

With a minimal maintenance effort we can be careful enough. I think
that a comment for example in pgstat.c about the usage uniqueness
would be an adapted answer.
--
Michael

On Thu, Aug 10, 2017 at 2:52 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> With a minimal maintenance effort we can be careful enough. I think
> that a comment for example in pgstat.c about the usage uniqueness
> would be an adapted answer.

By the way, let's discuss that on a new thread. I'll try to come up
with a patch that can be used for a base discussion soon.
--
Michael