On Fri, Sep 30, 2016 at 5:47 PM, Michael Paquier
<michael.paqu...@gmail.com> wrote:
> On Fri, Sep 30, 2016 at 1:48 AM, Robert Haas <robertmh...@gmail.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

+ /* 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,

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,

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

... then use 'pgstat_desc' here.

Thomas Munro

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

Reply via email to