From 351685da3cee865dfcb39fc1f23ead512714f9d8 Mon Sep 17 00:00:00 2001 From: Ants Aasma Date: Tue, 11 Jun 2024 17:01:48 +0300 Subject: [PATCH 1/7] Track on CPU events too To not count dead backends as still running on the CPU we need to detect that the backend is dead. Starting from PG17 proc->pid is reset in ProcKill, before this we can check if the process latch is disowned. Not nice to be poking around in latch internals like this, but all alternatives seem to involve scanning bestatus array and correlating pids. Also makes sense to exclude ourselves as we will always be on CPU while looking at wait events. --- collector.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/collector.c b/collector.c index dcb9695..cae7ec7 100644 --- a/collector.c +++ b/collector.c @@ -163,10 +163,7 @@ probe_waits(History *observations, HTAB *profile_hash, *observation; PGPROC *proc = &ProcGlobal->allProcs[i]; - if (proc->pid == 0) - continue; - - if (proc->wait_event_info == 0) + if (proc->pid == 0 || proc->procLatch.owner_pid == 0 || proc->pid == MyProcPid) continue; /* Collect next wait event sample */ From b156883013857b579e664a3e7d3e641820fe3cd5 Mon Sep 17 00:00:00 2001 From: Ants Aasma Date: Fri, 14 Jun 2024 18:12:23 +0300 Subject: [PATCH 2/7] Add a GUC for controlling whether on CPU events are counted Defaults to false meaning previous behavior is retained. Update pg_wait_sampling_current view to respect this flag. --- README.md | 19 ++++++++++++------- collector.c | 3 +++ pg_wait_sampling.c | 20 +++++++++++++++++--- pg_wait_sampling.h | 1 + 4 files changed, 33 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index d8b03dc..01dd020 100644 --- a/README.md +++ b/README.md @@ -133,13 +133,14 @@ in-memory hash table. The work of wait event statistics collector worker is controlled by following GUCs. -| Parameter name | Data type | Description | Default value | -| ----------------------------------- | --------- | ------------------------------------------- | ------------: | -| pg_wait_sampling.history_size | int4 | Size of history in-memory ring buffer | 5000 | -| pg_wait_sampling.history_period | int4 | Period for history sampling in milliseconds | 10 | -| pg_wait_sampling.profile_period | int4 | Period for profile sampling in milliseconds | 10 | -| pg_wait_sampling.profile_pid | bool | Whether profile should be per pid | true | -| pg_wait_sampling.profile_queries | bool | Whether profile should be per query | true | +| Parameter name | Data type | Description | Default value | +|----------------------------------| --------- |---------------------------------------------|--------------:| +| pg_wait_sampling.history_size | int4 | Size of history in-memory ring buffer | 5000 | +| pg_wait_sampling.history_period | int4 | Period for history sampling in milliseconds | 10 | +| pg_wait_sampling.profile_period | int4 | Period for profile sampling in milliseconds | 10 | +| pg_wait_sampling.profile_pid | bool | Whether profile should be per pid | true | +| pg_wait_sampling.profile_queries | bool | Whether profile should be per query | true | +| pg_wait_sampling.sample_cpu | bool | Whether on CPU backends should be sampled | false | If `pg_wait_sampling.profile_pid` is set to false, sampling profile wouldn't be collected in per-process manner. In this case the value of pid could would @@ -148,6 +149,10 @@ be always zero and corresponding row contain samples among all the processes. While `pg_wait_sampling.profile_queries` is set to false `queryid` field in views will be zero. +If `pg_wait_sampling.sample_cpu` is set to true hen processes that are not +waiting on anything are also sampled. The wait event columns for such processes +will be NULL. + These GUCs are allowed to be changed by superuser. Also, they are placed into shared memory. Thus, they could be changed from any backend and affects worker runtime. diff --git a/collector.c b/collector.c index cae7ec7..9970e67 100644 --- a/collector.c +++ b/collector.c @@ -163,6 +163,9 @@ probe_waits(History *observations, HTAB *profile_hash, *observation; PGPROC *proc = &ProcGlobal->allProcs[i]; + if (proc->wait_event_info == 0 && !pgws_collector_hdr->sampleCpu) + continue; + if (proc->pid == 0 || proc->procLatch.owner_pid == 0 || proc->pid == MyProcPid) continue; diff --git a/pg_wait_sampling.c b/pg_wait_sampling.c index 268b40e..602c03c 100644 --- a/pg_wait_sampling.c +++ b/pg_wait_sampling.c @@ -198,7 +198,8 @@ setup_gucs() history_period_found = false, profile_period_found = false, profile_pid_found = false, - profile_queries_found = false; + profile_queries_found = false, + sample_cpu_found = false; get_guc_variables_compat(&guc_vars, &numOpts); @@ -240,6 +241,12 @@ setup_gucs() var->_bool.variable = &pgws_collector_hdr->profileQueries; pgws_collector_hdr->profileQueries = true; } + else if (!strcmp(name, "pg_wait_sampling.sample_cpu")) + { + sample_cpu_found = true; + var->_bool.variable = &pgws_collector_hdr->sampleCpu; + pgws_collector_hdr->sampleCpu = false; + } } if (!history_size_found) @@ -272,11 +279,18 @@ setup_gucs() &pgws_collector_hdr->profileQueries, true, PGC_SUSET, 0, shmem_bool_guc_check_hook, NULL, NULL); + if (!sample_cpu_found) + DefineCustomBoolVariable("pg_wait_sampling.sample_cpu", + "Sets whether not waiting backends should be sampled.", NULL, + &pgws_collector_hdr->sampleCpu, false, + PGC_SUSET, 0, shmem_bool_guc_check_hook, NULL, NULL); + if (history_size_found || history_period_found || profile_period_found || profile_pid_found - || profile_queries_found) + || profile_queries_found + || sample_cpu_found) { ProcessConfigFile(PGC_SIGHUP); } @@ -501,7 +515,7 @@ pg_wait_sampling_get_current(PG_FUNCTION_ARGS) { PGPROC *proc = &ProcGlobal->allProcs[i]; - if (proc != NULL && proc->pid != 0 && proc->wait_event_info) + if (proc != NULL && proc->pid != 0 && (proc->wait_event_info || pgws_collector_hdr->sampleCpu)) { params->items[j].pid = proc->pid; params->items[j].wait_event_info = proc->wait_event_info; diff --git a/pg_wait_sampling.h b/pg_wait_sampling.h index 29425fc..de08e09 100644 --- a/pg_wait_sampling.h +++ b/pg_wait_sampling.h @@ -68,6 +68,7 @@ typedef struct int profilePeriod; bool profilePid; bool profileQueries; + bool sampleCpu; } CollectorShmqHeader; /* pg_wait_sampling.c */ From 0ce12dc79d0d35dea526c4917fc4994b9af95a90 Mon Sep 17 00:00:00 2001 From: Ants Aasma Date: Mon, 17 Jun 2024 14:54:27 +0300 Subject: [PATCH 3/7] Fix typo in README --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 01dd020..514bb15 100644 --- a/README.md +++ b/README.md @@ -149,7 +149,7 @@ be always zero and corresponding row contain samples among all the processes. While `pg_wait_sampling.profile_queries` is set to false `queryid` field in views will be zero. -If `pg_wait_sampling.sample_cpu` is set to true hen processes that are not +If `pg_wait_sampling.sample_cpu` is set to true then processes that are not waiting on anything are also sampled. The wait event columns for such processes will be NULL. From f22906ee1078273c271dd542c9e9280dc1a7e268 Mon Sep 17 00:00:00 2001 From: Ants Aasma Date: Mon, 17 Jun 2024 14:55:09 +0300 Subject: [PATCH 4/7] Add comment justifying peeking into procLatch --- collector.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/collector.c b/collector.c index 9970e67..895e8df 100644 --- a/collector.c +++ b/collector.c @@ -166,6 +166,12 @@ probe_waits(History *observations, HTAB *profile_hash, if (proc->wait_event_info == 0 && !pgws_collector_hdr->sampleCpu) continue; + /* + * On PostgreSQL versions < 17 the PGPROC->pid field is not reset on + * process exit. This would lead to such processes getting counted + * for null wait events. So instead we make use of DisownLatch() + * resetting owner_pid during ProcKill(). + */ if (proc->pid == 0 || proc->procLatch.owner_pid == 0 || proc->pid == MyProcPid) continue; From ceaafb860a6897c5ebc607de012fcc9454ddb32c Mon Sep 17 00:00:00 2001 From: Ants Aasma Date: Mon, 17 Jun 2024 14:55:43 +0300 Subject: [PATCH 5/7] Make pg_wait_sampling_get_current() mirror probe_waits() --- pg_wait_sampling.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/pg_wait_sampling.c b/pg_wait_sampling.c index 602c03c..fb6141d 100644 --- a/pg_wait_sampling.c +++ b/pg_wait_sampling.c @@ -515,13 +515,17 @@ pg_wait_sampling_get_current(PG_FUNCTION_ARGS) { PGPROC *proc = &ProcGlobal->allProcs[i]; - if (proc != NULL && proc->pid != 0 && (proc->wait_event_info || pgws_collector_hdr->sampleCpu)) - { - params->items[j].pid = proc->pid; - params->items[j].wait_event_info = proc->wait_event_info; - params->items[j].queryId = pgws_proc_queryids[i]; - j++; - } + if (proc->wait_event_info == 0 && !pgws_collector_hdr->sampleCpu) + continue; + + /* See corresponding comment in probe_waits() */ + if (proc->pid == 0 || proc->procLatch.owner_pid == 0 || proc->pid == MyProcPid) + continue; + + params->items[j].pid = proc->pid; + params->items[j].wait_event_info = proc->wait_event_info; + params->items[j].queryId = pgws_proc_queryids[i]; + j++; } funcctx->max_calls = j; } From 567d0c9ebd4981ab126bc1343c2af4db825f3f78 Mon Sep 17 00:00:00 2001 From: Ants Aasma Date: Mon, 17 Jun 2024 15:08:50 +0300 Subject: [PATCH 6/7] Factor decision to sample into a separate function --- collector.c | 11 +---------- pg_wait_sampling.c | 28 +++++++++++++++++++++++----- pg_wait_sampling.h | 1 + 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/collector.c b/collector.c index 895e8df..215bebc 100644 --- a/collector.c +++ b/collector.c @@ -163,16 +163,7 @@ probe_waits(History *observations, HTAB *profile_hash, *observation; PGPROC *proc = &ProcGlobal->allProcs[i]; - if (proc->wait_event_info == 0 && !pgws_collector_hdr->sampleCpu) - continue; - - /* - * On PostgreSQL versions < 17 the PGPROC->pid field is not reset on - * process exit. This would lead to such processes getting counted - * for null wait events. So instead we make use of DisownLatch() - * resetting owner_pid during ProcKill(). - */ - if (proc->pid == 0 || proc->procLatch.owner_pid == 0 || proc->pid == MyProcPid) + if (!pgws_should_sample_proc(proc)) continue; /* Collect next wait event sample */ diff --git a/pg_wait_sampling.c b/pg_wait_sampling.c index fb6141d..bc16786 100644 --- a/pg_wait_sampling.c +++ b/pg_wait_sampling.c @@ -450,6 +450,28 @@ search_proc(int pid) return NULL; } +/* + * Decide whether this PGPROC entry should be included in profiles and output + * views. + */ +bool +pgws_should_sample_proc(PGPROC *proc) +{ + if (proc->wait_event_info == 0 && !pgws_collector_hdr->sampleCpu) + return false; + + /* + * On PostgreSQL versions < 17 the PGPROC->pid field is not reset on + * process exit. This would lead to such processes getting counted for + * null wait events. So instead we make use of DisownLatch() resetting + * owner_pid during ProcKill(). + */ + if (proc->pid == 0 || proc->procLatch.owner_pid == 0 || proc->pid == MyProcPid) + return false; + + return true; +} + typedef struct { HistoryItem *items; @@ -515,11 +537,7 @@ pg_wait_sampling_get_current(PG_FUNCTION_ARGS) { PGPROC *proc = &ProcGlobal->allProcs[i]; - if (proc->wait_event_info == 0 && !pgws_collector_hdr->sampleCpu) - continue; - - /* See corresponding comment in probe_waits() */ - if (proc->pid == 0 || proc->procLatch.owner_pid == 0 || proc->pid == MyProcPid) + if (!pgws_should_sample_proc(proc)) continue; params->items[j].pid = proc->pid; diff --git a/pg_wait_sampling.h b/pg_wait_sampling.h index de08e09..8ff31f8 100644 --- a/pg_wait_sampling.h +++ b/pg_wait_sampling.h @@ -76,6 +76,7 @@ extern CollectorShmqHeader *pgws_collector_hdr; extern shm_mq *pgws_collector_mq; extern uint64 *pgws_proc_queryids; extern void pgws_init_lock_tag(LOCKTAG *tag, uint32 lock); +extern bool pgws_should_sample_proc(PGPROC *proc); /* collector.c */ extern void pgws_register_wait_collector(void); From fbe3e7f5bb29f23640b9b5b5589095a883b978c8 Mon Sep 17 00:00:00 2001 From: Ants Aasma Date: Mon, 17 Jun 2024 16:53:10 +0300 Subject: [PATCH 7/7] Turn on sampling of on CPU events by default --- README.md | 2 +- pg_wait_sampling.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 514bb15..ddced01 100644 --- a/README.md +++ b/README.md @@ -140,7 +140,7 @@ GUCs. | pg_wait_sampling.profile_period | int4 | Period for profile sampling in milliseconds | 10 | | pg_wait_sampling.profile_pid | bool | Whether profile should be per pid | true | | pg_wait_sampling.profile_queries | bool | Whether profile should be per query | true | -| pg_wait_sampling.sample_cpu | bool | Whether on CPU backends should be sampled | false | +| pg_wait_sampling.sample_cpu | bool | Whether on CPU backends should be sampled | true | If `pg_wait_sampling.profile_pid` is set to false, sampling profile wouldn't be collected in per-process manner. In this case the value of pid could would diff --git a/pg_wait_sampling.c b/pg_wait_sampling.c index bc16786..f226728 100644 --- a/pg_wait_sampling.c +++ b/pg_wait_sampling.c @@ -245,7 +245,7 @@ setup_gucs() { sample_cpu_found = true; var->_bool.variable = &pgws_collector_hdr->sampleCpu; - pgws_collector_hdr->sampleCpu = false; + pgws_collector_hdr->sampleCpu = true; } } @@ -282,7 +282,7 @@ setup_gucs() if (!sample_cpu_found) DefineCustomBoolVariable("pg_wait_sampling.sample_cpu", "Sets whether not waiting backends should be sampled.", NULL, - &pgws_collector_hdr->sampleCpu, false, + &pgws_collector_hdr->sampleCpu, true, PGC_SUSET, 0, shmem_bool_guc_check_hook, NULL, NULL); if (history_size_found