Re: Tracking wait event for latches

On Fri, Sep 23, 2016 at 10:32 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Sep 22, 2016 at 7:10 PM, Thomas Munro
> <thomas(dot)munro(at)enterprisedb(dot)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