Re: Tracking wait event for latches - Mailing list pgsql-hackers

On Fri, Sep 23, 2016 at 10:32 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Sep 22, 2016 at 7:10 PM, Thomas Munro
> <thomas.munro@enterprisedb.com> wrote:
>> I was thinking about suggesting a category "Replication" to cover the
>> waits for client IO relating to replication, as opposed to client IO
>> waits relating to regular user connections.  Then you could put sync
>> rep into that category instead of IPC, even though technically it is
>> waiting for IPC from walsender process(es), on the basis that it's
>> more newsworthy to a DBA that it's really waiting for a remote replica
>> to respond.  But it's probably pretty clear what's going on from the
>> the wait point names, so maybe it's not worth a category.  Thoughts?
>
> I thought about a replication category but either it will only have
> SyncRep in it, which is odd, or it will pull in other things that
> otherwise fit nicely into the Activity category, and then that
> boundaries of all the categories become mushy: is the subsystem that
> causes the wait that we are trying to document, or the kind of thing
> for which we are waiting?

Using category IPC for SyncRep looks fine to me.

>> I do suspect that the set of wait points will grow quite a bit as we
>> develop more parallel stuff though.  For example, I have been working
>> on a patch that adds several more wait points, indirectly via
>> condition variables (using your patch).  Actually in my case it's
>> BarrierWait -> ConditionVariableWait -> WaitEventSetWait.  I propose
>> that these higher level wait primitives should support passing a wait
>> point identifier through to WaitEventSetWait.
>
> +1.

As much as I suspect that inclusion of pgstat.h will become more and
more usual to allow more code paths to access to a given WaitClass.

After digesting all the comments given, I have produced the patch
attached that uses the following categories:
+const char *const WaitEventNames[] = {
+   /* activity */
+   "ArchiverMain",
+   "AutoVacuumMain",
+   "BgWriterHibernate",
+   "BgWriterMain",
+   "CheckpointerMain",
+   "PgStatMain",
+   "RecoveryWalAll",
+   "RecoveryWalStream",
+   "SysLoggerMain",
+   "WalReceiverMain",
+   "WalSenderMain",
+   "WalWriterMain",
+   /* client */
+   "SecureRead",
+   "SecureWrite",
+   "SSLOpenServer",
+   "WalReceiverWaitStart",
+   "WalSenderWaitForWAL",
+   "WalSenderWriteData",
+   /* Extension */
+   "Extension",
+   /* IPC */
+   "BgWorkerShutdown",
+   "BgWorkerStartup",
+   "ExecuteGather",
+   "MessageQueueInternal",
+   "MessageQueuePutMessage",
+   "MessageQueueReceive",
+   "MessageQueueSend",
+   "ParallelFinish",
+   "ProcSignal",
+   "ProcSleep",
+   "SyncRep",
+   /* timeout */
+   "BaseBackupThrottle",
+   "PgSleep",
+   "RecoveryApplyDelay",
+};
I have moved WalSenderMain as it tracks a main loop, so it was strange
to not have it in Activity. I also kept SecureRead and SecureWrite
because this is referring to the functions of the same name. For the
rest, I got convinced by what has been discussed and it makes things
clearer. My head is not spinning anymore when I try to keep in sync
the list of names and its associated enum, which is good. I have as
well rewritten the docs to follow that.
--
Michael