Re: Tracking wait event for latches

On Thu, Jun 2, 2016 at 12:25 PM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> This patch allows identifiers to be specified by the WaitLatch and
> WaitLatchOrSocket calls, but not for WaitEventSetWait, which is the
> more general waiting primitive. I think it should be done by
> WaitEventSetWait, and merely passed down by those wrappers functions.

The advantage of having WaitEventSetWait track is that we could track
the events of secure_read and secure_write. One potential problem by
doing so is if those routines are called during authentication. I
don't recall that's the case, but this needs a double-check.

> These are actually names for *wait points*, not names for latches.

OK.

> Some of the language in this patch makes it sound like they are latch
> names/latch identifiers, which is inaccurate (the latches themselves
> wouldn't be very interesting eg MyLatch). In some cases the main
> thing of interest is actually a socket or timer anyway, not a latch,
> so maybe it should appear as wait_event_type = WaitEventSet?

Hm. A latch could wait for multiple types things it is waiting for, so
don't you think we'd need to add more details in what is reported to
pg_stat_activity? There are two fields now, and in the context of this
patch:
- wait_event_type, which I'd like to think is associated to a latch,
so I named it so.
- wait_event, which is the code path that we are waiting at, like
SyncRep, the WAL writer main routine, etc.

Now if you would like to get a list of all the things that are being
waited for, shouldn't we add a third column to the set that has text[]
as return type? Let's name it wait_event_details, and for a latch we
have the following:
- WL_LATCH_SET
- WL_POSTMASTER_DEATH
- WL_SOCKET_READABLE
- WL_SOCKET_WRITEABLE
Compared to all the other existing wait types, that's a bit new and
that's exclusive to latches because we need a higher level of details.
Don't you think so? But actually I don't think that's necessary to go
into this level of details. We already know what a latch is waiting
for by looking at the code...

> Is there a reason you chose names like 'WALWriterMain'? That
> particular wait point is in the function WalWriterMain (note different
> case). It seems odd to use the containing function names to guide
> naming, but not use them exactly. Since this namespace needs to be
> able to name wait points anywhere in the postgres source tree (and
> maybe outside it too, for extensions), maybe it should be made
> hierarchical, like 'walwriter.WalWriterMain' (or some other
> organisational scheme).

Yeah, possibly this could be improved. I have put some thoughts in
having clear names for each one of them, so I'd rather keep them
simple.

> I personally think it would be very useful for extensions to be able
> to name their wait points. For example, I'd rather see
> 'postgres_fdw.pgfdw_get_result' or similar than a vague 'Extension'
> string which appears not only for all wait points in an extension but
> also for all extensions. I hope we can figure out a good way to do
> that. Clearly that would involve some shmem registry machinery to
> make the names visible across backends (a similar problem exists with
> lock tranche names).

This patch is shaped this way intentionally based on the feedback I
received at PGCon (Robert and others). We could provide a routine that
extensions call in _PG_init to register a new latch event name in
shared memory, but I didn't see much use in doing so, take for example
the case of background worker, it is possible to register a custom
string for pg_stat_activity via pgstat_report_activity. One could take
advantage of having custom latch wait names in shared memory if an
extension has wait points with latches though... But I am still not
sure if that's worth the complexity.
--
Michael