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 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 -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers