From 73d1ec2e41f221f8864bc7f9bfa0ffa81cee505b Mon Sep 17 00:00:00 2001 From: Andres Freund 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