Re: Tracking wait event for latches

On Fri, Sep 30, 2016 at 5:47 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Fri, Sep 30, 2016 at 1:48 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> The way that we're constructing the wait event ID that ultimately gets
>> advertised in pg_stat_activity is a bit silly in this patch. We start
>> with an event ID (which is an integer) and we then do an array lookup
>> (via GetWaitEventClass) to get a second integer which is then XOR'd
>> back into the first integer (via pgstat_report_wait_start) before
>> storing it in the PGPROC. New idea: let's change
>> pgstat_report_wait_start() to take a single integer argument which it
>> stores without interpretation. Let's separate the construction of the
>> wait event into a separate macro, say make_wait_event(). Then
>> existing callers of pgstat_report_wait_start(a, b) will instead do
>> pgstat_report_wait_start(make_wait_event(a, b)), but callers that want
>> to construct the wait event and push it through a few levels of the
>> call stack before advertising it only need to pass one integer. If
>> done correctly, this should be cleaner and faster than what you've got
>> right now.
>
> Hm, OK.... So ProcSleep() and ProcWaitForSignal() need an additional
> argument in the shape of a uint32 wait_event. So we need to change the
> shape of WaitLatch&friends to have also just a uint32 as an extra
> argument. This has as result to force all the callers of WaitLatch to
> use the new routine pgstat_make_wait_event() (renamed it), so pgstat.h
> needs to be included where WaitLatch() is called.

Hmm. I like the use of pgstat in that name. It helps with the
confusion created by the overloading of the term 'wait event' in the
pg_stat_activity view and the WaitEventSet API, by putting it into a
different pseudo-namespace.

+ uint32 wait_event;

How about a typedef for that instead of using raw uint32 everywhere?
Something like pgstat_wait_descriptor? Then a variable name like
pgstat_desc, since this is most definitely not just a wait_event
anymore.

+ /* Define event to wait for */

It's not defining the event to wait for at all, it's building a
description for pgstat.

+ wait_event = pgstat_make_wait_event(WAIT_EXTENSION,
+ WAIT_EVENT_EXTENSION);

It's not making a wait event, it's combining a class and an event.
How about something like this:

pgstat_desc = pgstat_make_wait_descriptor(WAIT_CLASS_EXTENSION,
WAIT_EVENT_EXTENSION)?

/* Sleep until there's something to do */
wc = WaitLatchOrSocket(MyLatch,
WL_LATCH_SET | WL_SOCKET_READABLE,
PQsocket(conn),
- -1L);
+ -1L,
+ wait_event);

... then use 'pgstat_desc' here.

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