From 73d1ec2e41f221f8864bc7f9bfa0ffa81cee505b Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Fri, 2 Apr 2021 12:25:21 -0700
Subject: [PATCH v2 3/3] Improve efficiency of wait event reporting.

---
 src/include/utils/wait_event.h          | 42 +++++++------------------
 src/backend/storage/lmgr/proc.c         | 24 +++++++++-----
 src/backend/utils/activity/wait_event.c | 41 ++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 38 deletions(-)

diff --git a/src/include/utils/wait_event.h b/src/include/utils/wait_event.h
index 2d6abfcb3f8..cff683a24eb 100644
--- a/src/include/utils/wait_event.h
+++ b/src/include/utils/wait_event.h
@@ -12,9 +12,6 @@
 #define WAIT_EVENT_H
 
 
-#include "storage/proc.h" /* for MyProc */
-
-
 /* ----------
  * Wait Classes
  * ----------
@@ -235,13 +232,10 @@ extern const char *pgstat_get_wait_event(uint32 wait_event_info);
 extern const char *pgstat_get_wait_event_type(uint32 wait_event_info);
 static inline void pgstat_report_wait_start(uint32 wait_event_info);
 static inline void pgstat_report_wait_end(void);
+extern void pgstat_set_wait_event_storage(uint32 *wait_event_info);
+extern void pgstat_reset_wait_event_storage(void);
 
-
-/*
- * Repeated here for the inline functions because it is declared in pgstat.h,
- * which includes this header.
- */
-extern PGDLLIMPORT bool pgstat_track_activities;
+extern PGDLLIMPORT uint32 *my_wait_event_info;
 
 
 /* ----------
@@ -255,47 +249,35 @@ extern PGDLLIMPORT bool pgstat_track_activities;
  *	for wait event which is sufficient for current usage, 1-byte is
  *	reserved for future usage.
  *
- * NB: this *must* be able to survive being called before MyProc has been
- * initialized.
+ *	Historically we used to make this reporting conditional on
+ *	pgstat_track_activities, but the check for that seems to add more cost
+ *	than it saves.
+ *
+ *	my_wait_event_info initially points to local memory, making it save to
+ *	call this before MyProc has been initialized.
  * ----------
  */
 static inline void
 pgstat_report_wait_start(uint32 wait_event_info)
 {
-	volatile PGPROC *proc = MyProc;
-
-	if (!pgstat_track_activities || !proc)
-		return;
-
 	/*
 	 * Since this is a four-byte field which is always read and written as
 	 * four-bytes, updates are atomic.
 	 */
-	proc->wait_event_info = wait_event_info;
+	*(volatile uint32 *) my_wait_event_info = wait_event_info;
 }
 
 /* ----------
  * pgstat_report_wait_end() -
  *
  *	Called to report end of a wait.
- *
- * NB: this *must* be able to survive being called before MyProc has been
- * initialized.
  * ----------
  */
 static inline void
 pgstat_report_wait_end(void)
 {
-	volatile PGPROC *proc = MyProc;
-
-	if (!pgstat_track_activities || !proc)
-		return;
-
-	/*
-	 * Since this is a four-byte field which is always read and written as
-	 * four-bytes, updates are atomic.
-	 */
-	proc->wait_event_info = 0;
+	/* see pgstat_report_wait_start() */
+	*(volatile uint32 *) my_wait_event_info = 0;
 }
 
 
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 897045ee272..692f21ef6a8 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -448,6 +448,9 @@ InitProcess(void)
 	OwnLatch(&MyProc->procLatch);
 	SwitchToSharedLatch();
 
+	/* now that we have a proc, report wait events to shared memory */
+	pgstat_set_wait_event_storage(&MyProc->wait_event_info);
+
 	/*
 	 * We might be reusing a semaphore that belonged to a failed process. So
 	 * be careful and reinitialize its value here.  (This is not strictly
@@ -601,6 +604,9 @@ InitAuxiliaryProcess(void)
 	OwnLatch(&MyProc->procLatch);
 	SwitchToSharedLatch();
 
+	/* now that we have a proc, report wait events to shared memory */
+	pgstat_set_wait_event_storage(&MyProc->wait_event_info);
+
 	/* Check that group locking fields are in a proper initial state. */
 	Assert(MyProc->lockGroupLeader == NULL);
 	Assert(dlist_is_empty(&MyProc->lockGroupMembers));
@@ -884,10 +890,15 @@ ProcKill(int code, Datum arg)
 	/*
 	 * Reset MyLatch to the process local one.  This is so that signal
 	 * handlers et al can continue using the latch after the shared latch
-	 * isn't ours anymore. After that clear MyProc and disown the shared
-	 * latch.
+	 * isn't ours anymore.
+	 *
+	 * Similarly, stop reporting wait events to MyProc->wait_event_info.
+	 *
+	 * After that clear MyProc and disown the shared latch.
 	 */
 	SwitchBackToLocalLatch();
+	pgstat_reset_wait_event_storage();
+
 	proc = MyProc;
 	MyProc = NULL;
 	DisownLatch(&proc->procLatch);
@@ -952,13 +963,10 @@ AuxiliaryProcKill(int code, Datum arg)
 	/* Cancel any pending condition variable sleep, too */
 	ConditionVariableCancelSleep();
 
-	/*
-	 * Reset MyLatch to the process local one.  This is so that signal
-	 * handlers et al can continue using the latch after the shared latch
-	 * isn't ours anymore. After that clear MyProc and disown the shared
-	 * latch.
-	 */
+	/* look at the equivalent ProcKill() code for comments */
 	SwitchBackToLocalLatch();
+	pgstat_reset_wait_event_storage();
+
 	proc = MyProc;
 	MyProc = NULL;
 	DisownLatch(&proc->procLatch);
diff --git a/src/backend/utils/activity/wait_event.c b/src/backend/utils/activity/wait_event.c
index 840ebef92ab..accc1eb5776 100644
--- a/src/backend/utils/activity/wait_event.c
+++ b/src/backend/utils/activity/wait_event.c
@@ -7,6 +7,17 @@
  *
  * IDENTIFICATION
  *	  src/backend/postmaster/wait_event.c
+ *
+ * NOTES
+ *
+ * To make pgstat_report_wait_start() and pgstat_report_wait_end() as
+ * lightweight as possible, they do not check if shared memory (MyProc
+ * specifically, where the wait event is stored) is already available. Instead
+ * we initially set my_wait_event_info to a process local variable, which then
+ * is redirected to shared memory using pgstat_set_wait_event_storage(). For
+ * the same reason pgstat_track_activities is not checked - the check adds
+ * more work than it saves.
+ *
  * ----------
  */
 #include "postgres.h"
@@ -23,6 +34,36 @@ static const char *pgstat_get_wait_timeout(WaitEventTimeout w);
 static const char *pgstat_get_wait_io(WaitEventIO w);
 
 
+static uint32 local_my_wait_event_info;
+uint32	   *my_wait_event_info = &local_my_wait_event_info;
+
+
+/*
+ * Configure wait event reporting to report wait events to *wait_event_info.
+ * *wait_event_info needs to be valid until pgstat_reset_wait_event_storage()
+ * is called.
+ *
+ * Expected to be called during backend startup, to point my_wait_event_info
+ * into shared memory.
+ */
+void
+pgstat_set_wait_event_storage(uint32 *wait_event_info)
+{
+	my_wait_event_info = wait_event_info;
+}
+
+/*
+ * Reset wait event storage location.
+ *
+ * Expected to be called during backend shutdown, before the location set up
+ * pgstat_set_wait_event_storage() becomes invalid.
+ */
+void
+pgstat_reset_wait_event_storage(void)
+{
+	my_wait_event_info = &local_my_wait_event_info;
+}
+
 /* ----------
  * pgstat_get_wait_event_type() -
  *
-- 
2.31.0.121.g9198c13e34

