Re: Parallel workers stats in pg_stat_database
On 11/12/24 16:24, Michael Banck wrote: I am not sure "backend state" is a good reason (unless it is exposed somewhere to users?), but the point about utilities does make sense I guess. We only track parallel workers used by queries right now. Parallel index builds (btree & brin) and vacuum cleanup is not commited yet since it's not a common occurence. I implemented it in separate counters. -- Benoit Lobréau Consultant http://dalibo.com
Re: Parallel workers stats in pg_stat_database
Hi, On Tue, Nov 12, 2024 at 03:56:11PM +0100, Benoit Lobréau wrote: > On 11/12/24 15:05, Michael Banck wrote: > > I was wondering about the weird new column name workers_to_launch when I > > read the commit message - AFAICT this has been an internal term so far, > > and this is the first time we expose it to users? > > > > I personally find (parallel_)workers_planned/launched clearer from a > > user perspective, was it discussed that we need to follow the internal > > terms here? If so, I missed that discussion in this thread (and the > > other thread that lead to cf54a2c00). > > I initiallly called it like that but changed it to mirror the column > name added in pg_stat_statements for coherence sake. Ah, I mixed up the threads about adding parallel stats to pg_stat_all_tables and pg_stat_statements - I only reviewed the former, but in the latter, Michael writes: |- I've been struggling a bit on the "planned" vs "launched" terms used |in the names for the counters. It is inconsistent with the backend |state, where we talk about workers "to launch" and workers "launched". |"planned" does not really apply to utilities, as this may not be |planned per se. I am not sure "backend state" is a good reason (unless it is exposed somewhere to users?), but the point about utilities does make sense I guess. Michael
Re: Parallel workers stats in pg_stat_database
On 11/12/24 15:05, Michael Banck wrote: I was wondering about the weird new column name workers_to_launch when I read the commit message - AFAICT this has been an internal term so far, and this is the first time we expose it to users? I personally find (parallel_)workers_planned/launched clearer from a user perspective, was it discussed that we need to follow the internal terms here? If so, I missed that discussion in this thread (and the other thread that lead to cf54a2c00). Michael I initiallly called it like that but changed it to mirror the column name added in pg_stat_statements for coherence sake. I prefer "planned" but english is clearly not my strong suit and I assumed it meant that the number of worker planned could change before execution. I just checked in parallel.c and I don't think it's the case, could it be done elsewhere ? -- Benoit Lobréau Consultant http://dalibo.com
Re: Parallel workers stats in pg_stat_database
Hi,
On Fri, Oct 11, 2024 at 09:33:48AM +0200, Guillaume Lelarge wrote:
> FWIW, with the recent commits of the pg_stat_statements patch, you need a
> slight change in the patch I sent on this thread. You'll find a patch
> attached to do that. You need to apply it after a rebase to master.
>
> - if (estate->es_parallelized_workers_planned > 0) {
> + if (estate->es_parallel_workers_to_launch > 0) {
> pgstat_update_parallel_workers_stats(
> - (PgStat_Counter)
> estate->es_parallelized_workers_planned,
> - (PgStat_Counter)
> estate->es_parallelized_workers_launched);
> + (PgStat_Counter) estate->es_parallel_workers_to_launch,
> + (PgStat_Counter) estate->es_parallel_workers_launched);
I was wondering about the weird new column name workers_to_launch when I
read the commit message - AFAICT this has been an internal term so far,
and this is the first time we expose it to users?
I personally find (parallel_)workers_planned/launched clearer from a
user perspective, was it discussed that we need to follow the internal
terms here? If so, I missed that discussion in this thread (and the
other thread that lead to cf54a2c00).
Michael
Re: Parallel workers stats in pg_stat_database
On 11/11/24 02:51, Michael Paquier wrote: Okidoki, applied. If tweaks are necessary depending on the feedback, like column names, let's tackle things as required. We still have a good chunk of time for this release cycle. -- Michael Thanks ! -- Benoit Lobréau Consultant http://dalibo.com
Re: Parallel workers stats in pg_stat_database
On Fri, Nov 08, 2024 at 03:13:35PM +0100, Benoit Lobréau wrote: > I just reread the patch. > Thanks for the changes. It looks great. Okidoki, applied. If tweaks are necessary depending on the feedback, like column names, let's tackle things as required. We still have a good chunk of time for this release cycle. -- Michael signature.asc Description: PGP signature
Re: Parallel workers stats in pg_stat_database
On 11/8/24 05:08, Michael Paquier wrote: On Thu, Nov 07, 2024 at 02:36:58PM +0900, Michael Paquier wrote: Incorrect comment format, about which pgindent does not complain.. .. But pgindent complains in execMain.c and pgstat_database.c. These are only nits, the patch is fine. If anybody has objections or comments, feel free. Found a few more things, but overall it was fine. Here is what I have staged on my local branch. -- Michael Hi, I just reread the patch. Thanks for the changes. It looks great. -- Benoit Lobréau Consultant http://dalibo.com
Re: Parallel workers stats in pg_stat_database
On Thu, Nov 07, 2024 at 02:36:58PM +0900, Michael Paquier wrote:
> Incorrect comment format, about which pgindent does not complain..
>
> .. But pgindent complains in execMain.c and pgstat_database.c. These
> are only nits, the patch is fine. If anybody has objections or
> comments, feel free.
Found a few more things, but overall it was fine. Here is what I have
staged on my local branch.
--
Michael
From 960fbe663d4a6a4594b8121bbf299c72ef2a6ab8 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Fri, 8 Nov 2024 13:00:26 +0900
Subject: [PATCH v7] Add two attributes to pg_stat_database for parallel
workers activity
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Two attributes are added to pg_stat_database:
* parallel_workers_to_launch, counting the total number of parallel
workers that were planned to be launched.
* parallel_workers_launched, counting the total number of parallel
workers actually launched.
The ratio of both fields can provide hints that there are not enough
slots available when launching parallel workers.
This commit relies on de3a2ea3b264, that has added two fields to EState,
that get incremented when executing Gather or GatherMerge nodes. The
data now gets pushed to pg_stat_database, which is useful how much a
database faces contention when spawning parallel workers if
pg_stat_statements is not enabled on an instance.
Bump catalog version.
Author: Benoit Lobréau
Discussion: https://postgr.es/m/783bc7f7-659a-42fa-99dd-ee0565644...@dalibo.com
---
src/include/catalog/catversion.h | 2 +-
src/include/catalog/pg_proc.dat | 10 +++
src/include/pgstat.h | 4 +++
src/backend/catalog/system_views.sql | 2 ++
src/backend/executor/execMain.c | 5
src/backend/utils/activity/pgstat_database.c | 19 +
src/backend/utils/adt/pgstatfuncs.c | 6 +
src/test/regress/expected/rules.out | 2 ++
src/test/regress/expected/select_parallel.out | 27 +++
src/test/regress/sql/select_parallel.sql | 14 ++
doc/src/sgml/monitoring.sgml | 18 +
11 files changed, 108 insertions(+), 1 deletion(-)
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 2abc523f5c..86436e0356 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -57,6 +57,6 @@
*/
/* mmddN */
-#define CATALOG_VERSION_NO 202411071
+#define CATALOG_VERSION_NO 202411081
#endif
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index f23321a41f..cbbe8acd38 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5813,6 +5813,16 @@
proname => 'pg_stat_get_db_sessions_killed', provolatile => 's',
proparallel => 'r', prorettype => 'int8', proargtypes => 'oid',
prosrc => 'pg_stat_get_db_sessions_killed' },
+{ oid => '8403',
+ descr => 'statistics: number of parallel workers planned to be launched by queries',
+ proname => 'pg_stat_get_db_parallel_workers_to_launch', provolatile => 's',
+ proparallel => 'r', prorettype => 'int8', proargtypes => 'oid',
+ prosrc => 'pg_stat_get_db_parallel_workers_to_launch' },
+{ oid => '8404',
+ descr => 'statistics: number of parallel workers effectively launched by queries',
+ proname => 'pg_stat_get_db_parallel_workers_launched', provolatile => 's',
+ proparallel => 'r', prorettype => 'int8', proargtypes => 'oid',
+ prosrc => 'pg_stat_get_db_parallel_workers_launched' },
{ oid => '3195', descr => 'statistics: information about WAL archiver',
proname => 'pg_stat_get_archiver', proisstrict => 'f', provolatile => 's',
proparallel => 'r', prorettype => 'record', proargtypes => '',
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index df53fa2d4f..59c28b4aca 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -386,6 +386,8 @@ typedef struct PgStat_StatDBEntry
PgStat_Counter sessions_abandoned;
PgStat_Counter sessions_fatal;
PgStat_Counter sessions_killed;
+ PgStat_Counter parallel_workers_to_launch;
+ PgStat_Counter parallel_workers_launched;
TimestampTz stat_reset_timestamp;
} PgStat_StatDBEntry;
@@ -583,6 +585,8 @@ extern void pgstat_report_deadlock(void);
extern void pgstat_report_checksum_failures_in_db(Oid dboid, int failurecount);
extern void pgstat_report_checksum_failure(void);
extern void pgstat_report_connect(Oid dboid);
+extern void pgstat_update_parallel_workers_stats(PgStat_Counter workers_to_launch,
+ PgStat_Counter workers_launched);
#define pgstat_count_buffer_read_time(n) \
(pgStatBlockReadTime += (n))
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 3456b821bc..da9a8fe99f 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1073,6 +1073,8 @@ CREATE VIEW pg_stat_database AS
Re: Parallel workers stats in pg_stat_database
On Sat, Oct 12, 2024 at 01:14:54AM +0200, Benoit Lobréau wrote: > Here is an updated version, I modified it to: > > * have the same wording in the doc and code (planned => to_launch) > * split de declaration from the rest (and have the same code as the parallel > worker logging patch) Thanks for the updated patch set. I've been thinking about this proposal for the two counters with pg_stat_database in 0001, and I am going to side with the argument that it sucks to not have this information except if pg_stat_statements is enabled on an instance. It would be a different discussion if PGSS were to be in core, and if that were to happen we could perhaps remove these counters from pg_stat_database, but there is no way to be sure if this is going to happen, as well. And this information is useful for the GUC settings. +/* + * reports parallel_workers_to_launch and parallel_workers_launched into + * PgStat_StatDBEntry + */ Perhaps a reword with: "Notify the stats system about parallel worker information." +/* pg_stat_get_db_parallel_workers_to_launch*/ [...] +/* pg_stat_get_db_parallel_workers_launched*/ Incorrect comment format, about which pgindent does not complain.. .. But pgindent complains in execMain.c and pgstat_database.c. These are only nits, the patch is fine. If anybody has objections or comments, feel free. Now, I am not really on board with 0002 and 0003 about the tracking of the maintenance workers, which reflect operations that happen less often than what 0001 is covering. Perhaps this would have more value if autovacuum supported parallel operations, though. -- Michael signature.asc Description: PGP signature
Re: Parallel workers stats in pg_stat_database
On 10/11/24 09:33, Guillaume Lelarge wrote:
FWIW, with the recent commits of the pg_stat_statements patch, you need
a slight change in the patch I sent on this thread. You'll find a patch
attached to do that. You need to apply it after a rebase to master.
Thanks.
Here is an updated version, I modified it to:
* have the same wording in the doc and code (planned => to_launch)
* split de declaration from the rest (and have the same code as the
parallel worker logging patch)
--
Benoit Lobréau
Consultant
http://dalibo.comFrom ab9aa1344f974348638dd3898c944f3d5253374d Mon Sep 17 00:00:00 2001
From: benoit
Date: Tue, 8 Oct 2024 10:01:52 +0200
Subject: [PATCH 3/3] Adds two parallel workers stat columns for utilities to
pg_stat_database
* parallel_maint_workers_to_launch
* parallel_maint_workers_launched
---
doc/src/sgml/monitoring.sgml | 18 ++
src/backend/access/brin/brin.c| 4
src/backend/access/nbtree/nbtsort.c | 4
src/backend/catalog/system_views.sql | 2 ++
src/backend/commands/vacuumparallel.c | 6 ++
src/backend/utils/activity/pgstat_database.c | 18 ++
src/backend/utils/adt/pgstatfuncs.c | 6 ++
src/include/catalog/pg_proc.dat | 10 ++
src/include/pgstat.h | 3 +++
src/test/regress/expected/rules.out | 2 ++
src/test/regress/expected/vacuum_parallel.out | 19 +++
src/test/regress/sql/vacuum_parallel.sql | 11 +++
12 files changed, 103 insertions(+)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 840d7f8161..bd4e4b63c7 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3629,6 +3629,24 @@ description | Waiting for a newly initialized WAL file to reach durable storage
+
+
+ parallel_maint_workers_to_launch bigint
+
+
+ Number of parallel workers planned to be launched by utilities on this database
+
+
+
+
+
+ parallel_maint_workers_launched bigint
+
+
+ Number of parallel workers launched by utilities on this database
+
+
+
stats_reset timestamp with time zone
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index c0b978119a..4e83091d2c 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -2544,6 +2544,10 @@ _brin_end_parallel(BrinLeader *brinleader, BrinBuildState *state)
/* Shutdown worker processes */
WaitForParallelWorkersToFinish(brinleader->pcxt);
+ pgstat_update_parallel_maint_workers_stats(
+ (PgStat_Counter) brinleader->pcxt->nworkers_to_launch,
+ (PgStat_Counter) brinleader->pcxt->nworkers_launched);
+
/*
* Next, accumulate WAL usage. (This must wait for the workers to finish,
* or we might get incomplete data.)
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index 5cca0d4f52..8ee5fcf6d3 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -1615,6 +1615,10 @@ _bt_end_parallel(BTLeader *btleader)
/* Shutdown worker processes */
WaitForParallelWorkersToFinish(btleader->pcxt);
+ pgstat_update_parallel_maint_workers_stats(
+ (PgStat_Counter) btleader->pcxt->nworkers_to_launch,
+ (PgStat_Counter) btleader->pcxt->nworkers_launched);
+
/*
* Next, accumulate WAL usage. (This must wait for the workers to finish,
* or we might get incomplete data.)
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index da9a8fe99f..648166bb3b 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1075,6 +1075,8 @@ CREATE VIEW pg_stat_database AS
pg_stat_get_db_sessions_killed(D.oid) AS sessions_killed,
pg_stat_get_db_parallel_workers_to_launch(D.oid) as parallel_workers_to_launch,
pg_stat_get_db_parallel_workers_launched(D.oid) as parallel_workers_launched,
+pg_stat_get_db_parallel_maint_workers_to_launch(D.oid) as parallel_maint_workers_to_launch,
+pg_stat_get_db_parallel_maint_workers_launched(D.oid) as parallel_maint_workers_launched,
pg_stat_get_db_stat_reset_time(D.oid) AS stats_reset
FROM (
SELECT 0 AS oid, NULL::name AS datname
diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c
index 77679e8df6..edd0823353 100644
--- a/src/backend/commands/vacuumparallel.c
+++ b/src/backend/commands/vacuumparallel.c
@@ -443,6 +443,12 @@ parallel_vacuum_end(ParallelVacuumState *pvs, IndexBulkDeleteResult **istats)
{
Assert(!IsParallelWorker());
+ if (pvs->nworkers_to_launch > 0)
+ pgstat_update_parallel_maint_workers_stats(
+ (PgStat_Counter) pvs->pcxt->nworkers_to_launch,
+ (PgStat_Counter) pvs->pcxt->nworkers_launched
+ );
+
/* Cop
Re: Parallel workers stats in pg_stat_database
Le mar. 8 oct. 2024 à 14:03, Benoit Lobréau a
écrit :
> On 10/7/24 10:19, Guillaume Lelarge wrote:
> > I've done the split, but I didn't go any further than that.
>
> Thank you Guillaume. I have done the rest of the reformatting
> suggested by Michael but I decided to see If I have similar stuff
> in my logging patch and refactor accordingly if needed before posting
> the result here.
>
> I have hopes to finish it this week.
>
>
FWIW, with the recent commits of the pg_stat_statements patch, you need a
slight change in the patch I sent on this thread. You'll find a patch
attached to do that. You need to apply it after a rebase to master.
--
Guillaume.
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index e749cdaa17..60a643b08d 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -484,10 +484,10 @@ standard_ExecutorEnd(QueryDesc *queryDesc)
Assert(estate != NULL);
- if (estate->es_parallelized_workers_planned > 0) {
+ if (estate->es_parallel_workers_to_launch > 0) {
pgstat_update_parallel_workers_stats(
- (PgStat_Counter) estate->es_parallelized_workers_planned,
- (PgStat_Counter) estate->es_parallelized_workers_launched);
+ (PgStat_Counter) estate->es_parallel_workers_to_launch,
+ (PgStat_Counter) estate->es_parallel_workers_launched);
}
/*
Re: Parallel workers stats in pg_stat_database
On 10/7/24 10:19, Guillaume Lelarge wrote: I've done the split, but I didn't go any further than that. Thank you Guillaume. I have done the rest of the reformatting suggested by Michael but I decided to see If I have similar stuff in my logging patch and refactor accordingly if needed before posting the result here. I have hopes to finish it this week. -- Benoit Lobréau Consultant http://dalibo.com
Re: Parallel workers stats in pg_stat_database
Hey,
Le mer. 2 oct. 2024 à 11:12, Benoit Lobréau a
écrit :
> Hi,
>
> Thanks for your imput ! I will fix the doc as proposed and do the split
> as soon as I have time.
>
>
I've done the split, but I didn't go any further than that.
Two patches attached:
* v5-0001 adds the metrics (same patch than v3-0001 for pg_stat_statements)
* v5-0002 handles the metrics for pg_stat_database.
"make check" works, and I also did a few other tests without any issues.
Regards.
--
Guillaume.
From 9413829616bdc0806970647c15d7d6bbd96489d1 Mon Sep 17 00:00:00 2001
From: Guillaume Lelarge
Date: Mon, 7 Oct 2024 08:45:36 +0200
Subject: [PATCH v5 1/2] Introduce two new counters in EState
They will be used by two other patchs to populate new columns in
pg_stat_database and pg_statements.
---
src/backend/executor/execUtils.c | 3 +++
src/backend/executor/nodeGather.c | 3 +++
src/backend/executor/nodeGatherMerge.c | 3 +++
src/include/nodes/execnodes.h | 3 +++
4 files changed, 12 insertions(+)
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index 5737f9f4eb..1908481999 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -162,6 +162,9 @@ CreateExecutorState(void)
estate->es_jit_flags = 0;
estate->es_jit = NULL;
+ estate->es_parallelized_workers_launched = 0;
+ estate->es_parallelized_workers_planned = 0;
+
/*
* Return the executor state structure
*/
diff --git a/src/backend/executor/nodeGather.c b/src/backend/executor/nodeGather.c
index 5d4ffe989c..0fb915175a 100644
--- a/src/backend/executor/nodeGather.c
+++ b/src/backend/executor/nodeGather.c
@@ -182,6 +182,9 @@ ExecGather(PlanState *pstate)
/* We save # workers launched for the benefit of EXPLAIN */
node->nworkers_launched = pcxt->nworkers_launched;
+ estate->es_parallelized_workers_launched += pcxt->nworkers_launched;
+ estate->es_parallelized_workers_planned += pcxt->nworkers_to_launch;
+
/* Set up tuple queue readers to read the results. */
if (pcxt->nworkers_launched > 0)
{
diff --git a/src/backend/executor/nodeGatherMerge.c b/src/backend/executor/nodeGatherMerge.c
index 45f6017c29..149ab23d90 100644
--- a/src/backend/executor/nodeGatherMerge.c
+++ b/src/backend/executor/nodeGatherMerge.c
@@ -223,6 +223,9 @@ ExecGatherMerge(PlanState *pstate)
/* We save # workers launched for the benefit of EXPLAIN */
node->nworkers_launched = pcxt->nworkers_launched;
+ estate->es_parallelized_workers_launched += pcxt->nworkers_launched;
+ estate->es_parallelized_workers_planned += pcxt->nworkers_to_launch;
+
/* Set up tuple queue readers to read the results. */
if (pcxt->nworkers_launched > 0)
{
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index aab59d681c..f898590ece 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -708,6 +708,9 @@ typedef struct EState
bool es_use_parallel_mode; /* can we use parallel workers? */
+ int es_parallelized_workers_launched;
+ int es_parallelized_workers_planned;
+
/* The per-query shared memory area to use for parallel execution. */
struct dsa_area *es_query_dsa;
--
2.46.2
From 873c03b99e1ccc05bac4c71b5f070e0dbe0bd779 Mon Sep 17 00:00:00 2001
From: benoit
Date: Mon, 7 Oct 2024 10:12:46 +0200
Subject: [PATCH v5 2/2] Adds four parallel workers stat columns to
pg_stat_database
* parallel_workers_planned
* parallel_workers_launched
* parallel_maint_workers_planned
* parallel_maint_workers_launched
---
doc/src/sgml/monitoring.sgml | 36 +++
src/backend/access/brin/brin.c| 4 +++
src/backend/access/nbtree/nbtsort.c | 4 +++
src/backend/catalog/system_views.sql | 4 +++
src/backend/commands/vacuumparallel.c | 5 +++
src/backend/executor/execMain.c | 7
src/backend/utils/activity/pgstat_database.c | 36 +++
src/backend/utils/adt/pgstatfuncs.c | 12 +++
src/include/catalog/pg_proc.dat | 20 +++
src/include/pgstat.h | 7
src/test/regress/expected/rules.out | 4 +++
src/test/regress/expected/select_parallel.out | 11 ++
src/test/regress/expected/vacuum_parallel.out | 19 ++
src/test/regress/sql/select_parallel.sql | 8 +
src/test/regress/sql/vacuum_parallel.sql | 11 ++
15 files changed, 188 insertions(+)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 331315f8d3..9567ca5bd1 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3611,6 +3611,42 @@ description | Waiting for a newly initialized WAL file to reach durable storage
+
+
+ parallel_workers_planned bigint
+
+
+ Number of parallel workers planned by queries on this database
+
+
+
+
+
+ parallel_workers_launched
Re: Parallel workers stats in pg_stat_database
On Wed, Oct 02, 2024 at 11:12:37AM +0200, Benoit Lobréau wrote: > My collegues and I had a discussion about what could be done to improve > parallelism observability in PostgreSQL [0]. We thought about several > places to do it for several use cases. > > Guillaume Lelarge worked on pg_stat_statements [1]. Thanks, missed that. I will post a reply there. There is a good overlap with everything you are doing here, because each one of you wishes to track more data to the executor state and push it to different part of the system, system view or just an extension. Tracking the number of workers launched and planned in the executor state is the strict minimum for a lot of these things, as far as I can see. Once the nodes are able to push this data, then extensions can feed on it the way they want. So that's a good idea on its own, and covers two of the counters posted here: https://www.postgresql.org/message-id/CAECtzeWtTGOK0UgKXdDGpfTVSa5bd_VbUt6K6xn8P7X%2B_dZqKw%40mail.gmail.com Could you split the patch based on that? I'd recommend to move es_workers_launched and es_workers_planned closer to the top, say es_total_processed, and document what these counters are here for. After that comes the problem of where to push this data.. > Lastly the number would be more precise/easier to make sense of, since > pg_stat_statement has a limited size. Upper bound that can be configured. When looking for query-level patterns or specific SET tuning, using query-level data speaks more than this data pushed at database level. TBH, I am +-0 about pushing this data to pg_stat_database so as we would be able to tune database-level GUCs. That does not help with SET commands tweaking the number of workers to use. Well, perhaps few rely on SET and most rely on the system-level GUCs in their applications, meaning that I'm wrong, making your point about publishing this data at database-level better, but I'm not really sure. If others have an opinion, feel free. Anyway, what I am sure of is that publishing the same set of data everywhere leads to bloat, and I'd rather avoid that. Aggregating that from the queries also to get an impression of the whole database offers an equivalent of what would be stored in pg_stat_database assuming that the load is steady. Your point about pg_stat_statements not being set is also true, even if some cloud vendors enable it by default. Table/index-level data can be really interesting because we can cross-check what's happening for more complex queries if there are many gather nodes with complex JOINs. Utilities (vacuum, btree, brin) are straight-forward and best at query level, making pg_stat_statements their best match. And there is no need for four counters if pushed at this level while two are able to do the job as utility and non-utility statements are separated depending on their PlannedStmt leading to separate entries in PGSS. -- Michael signature.asc Description: PGP signature
Re: Parallel workers stats in pg_stat_database
Hi, Thanks for your imput ! I will fix the doc as proposed and do the split as soon as I have time. On 10/1/24 09:27, Michael Paquier wrote: I'm less a fan of the addition for utilities because these are less common operations. My thought process was that in order to size max_parallel_workers we need to have information on the maintenance parallel worker and "query" parallel workers. Actually, could we do better than what's proposed here? How about presenting an aggregate of this data in pg_stat_statements for each query instead? I think both features are useful. My collegues and I had a discussion about what could be done to improve parallelism observability in PostgreSQL [0]. We thought about several places to do it for several use cases. Guillaume Lelarge worked on pg_stat_statements [1] and pg_stat_user_[tables|indexes] [2]. I proposed a patch for the logs [3]. As a consultant, I frequently work on installation without pg_stat_statements and I cannot install it on the client's production in the timeframe of my intervention. pg_stat_database is available everywhere and can easily be sampled by collectors/supervision services (like check_pgactivity). Lastly the number would be more precise/easier to make sense of, since pg_stat_statement has a limited size. [0] https://www.postgresql.org/message-id/flat/d657df20-c4bf-63f6-e74c-cb85a81d0...@dalibo.com [1] https://www.postgresql.org/message-id/CAECtzeWtTGOK0UgKXdDGpfTVSa5bd_VbUt6K6xn8P7X%2B_dZqKw%40mail.gmail.com [2] https://www.postgresql.org/message-id/flat/CAECtzeXXuMkw-RVGTWvHGOJsmFdsRY%2BjK0ndQa80sw46y2uvVQ%40mail.gmail.com [3] https://www.postgresql.org/message-id/8123423a-f041-4f4c-a771-bfd96ab235b0%40dalibo.com -- Benoit Lobréau Consultant http://dalibo.com
Re: Parallel workers stats in pg_stat_database
On Tue, Sep 17, 2024 at 02:22:59PM +0200, Benoit Lobréau wrote: > Here is an updated patch fixing the aforementionned problems > with tests and vacuum stats. Your patch needs a rebase. + Number of parallel workers obtained by utilities on this database s/obtained/launched/ for consistency? I like the general idea of the patch because it is rather difficult now to know how to tune these parameters. If I were to put a priority on both ideas, the possibility of being able to look at the number of workers launched vs requested in the executor is higher, and I'm less a fan of the addition for utilities because these are less common operations. So I'd suggest to split the patch into two pieces, one for each, if we do that at database level, but.. Actually, could we do better than what's proposed here? How about presenting an aggregate of this data in pg_stat_statements for each query instead? The ExecutorEnd() hook has an access to the executor state, so the number of workers planned and launched could be given by the execution nodes to the estate, then fed back to pg_stat_statements. You are already doing most of the work with the introduction of es_workers_launched and es_workers_planned. If you want to get the data across a database, then just sum up the counters for all the queries, applying a filter with the number of calls, for example. -- Michael signature.asc Description: PGP signature
Re: Parallel workers stats in pg_stat_database
Here is an updated patch fixing the aforementionned problems with tests and vacuum stats. -- Benoit Lobréau Consultant http://dalibo.comFrom 1b52f5fb4e977599b8925c69193d31148042ca7d Mon Sep 17 00:00:00 2001 From: benoit Date: Wed, 28 Aug 2024 02:27:13 +0200 Subject: [PATCH] Adds four parallel workers stat columns to pg_stat_database * parallel_workers_planned * parallel_workers_launched * parallel_maint_workers_planned * parallel_maint_workers_launched --- doc/src/sgml/monitoring.sgml | 36 +++ src/backend/access/brin/brin.c| 4 +++ src/backend/access/nbtree/nbtsort.c | 4 +++ src/backend/catalog/system_views.sql | 4 +++ src/backend/commands/vacuumparallel.c | 5 +++ src/backend/executor/execMain.c | 7 src/backend/executor/execUtils.c | 3 ++ src/backend/executor/nodeGather.c | 3 ++ src/backend/executor/nodeGatherMerge.c| 3 ++ src/backend/utils/activity/pgstat_database.c | 36 +++ src/backend/utils/adt/pgstatfuncs.c | 12 +++ src/include/catalog/pg_proc.dat | 20 +++ src/include/nodes/execnodes.h | 3 ++ src/include/pgstat.h | 7 src/test/regress/expected/rules.out | 4 +++ src/test/regress/expected/select_parallel.out | 27 ++ src/test/regress/expected/vacuum_parallel.out | 19 ++ src/test/regress/sql/select_parallel.sql | 15 src/test/regress/sql/vacuum_parallel.sql | 11 ++ 19 files changed, 223 insertions(+) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 55417a6fa9..8c4b11c11d 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -3508,6 +3508,42 @@ description | Waiting for a newly initialized WAL file to reach durable storage + + + parallel_workers_planned bigint + + + Number of parallel workers planned by queries on this database + + + + + + parallel_workers_launched bigint + + + Number of parallel workers obtained by queries on this database + + + + + + parallel_maint_workers_planned bigint + + + Number of parallel workers planned by utilities on this database + + + + + + parallel_maint_workers_launched bigint + + + Number of parallel workers obtained by utilities on this database + + + stats_reset timestamp with time zone diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 6467bed604..9eceb87b52 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -2540,6 +2540,10 @@ _brin_end_parallel(BrinLeader *brinleader, BrinBuildState *state) /* Shutdown worker processes */ WaitForParallelWorkersToFinish(brinleader->pcxt); + pgstat_update_parallel_maint_workers_stats( + (PgStat_Counter) brinleader->pcxt->nworkers_to_launch, + (PgStat_Counter) brinleader->pcxt->nworkers_launched); + /* * Next, accumulate WAL usage. (This must wait for the workers to finish, * or we might get incomplete data.) diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c index f5d7b3b0c3..232e1a0942 100644 --- a/src/backend/access/nbtree/nbtsort.c +++ b/src/backend/access/nbtree/nbtsort.c @@ -1611,6 +1611,10 @@ _bt_end_parallel(BTLeader *btleader) /* Shutdown worker processes */ WaitForParallelWorkersToFinish(btleader->pcxt); + pgstat_update_parallel_maint_workers_stats( + (PgStat_Counter) btleader->pcxt->nworkers_to_launch, + (PgStat_Counter) btleader->pcxt->nworkers_launched); + /* * Next, accumulate WAL usage. (This must wait for the workers to finish, * or we might get incomplete data.) diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 19cabc9a47..48bf9e5535 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -1073,6 +1073,10 @@ CREATE VIEW pg_stat_database AS pg_stat_get_db_sessions_abandoned(D.oid) AS sessions_abandoned, pg_stat_get_db_sessions_fatal(D.oid) AS sessions_fatal, pg_stat_get_db_sessions_killed(D.oid) AS sessions_killed, +pg_stat_get_db_parallel_workers_planned(D.oid) as parallel_workers_planned, +pg_stat_get_db_parallel_workers_launched(D.oid) as parallel_workers_launched, +pg_stat_get_db_parallel_maint_workers_planned(D.oid) as parallel_maint_workers_planned, +pg_stat_get_db_parallel_maint_workers_launched(D.oid) as parallel_maint_workers_launched, pg_stat_get_db_stat_reset_time(D.oid) AS stats_re
Re: Parallel workers stats in pg_stat_database
On 9/4/24 08:46, Bertrand Drouvot wrote:> What about moving the tests to places where it's "guaranteed" to get parallel workers involved? For example, a "parallel_maint_workers" only test could be done in vacuum_parallel.sql. Thank you ! I was too focussed on the stat part and missed the obvious. It's indeed better with this file. ... Which led me to discover that the area I choose to gather my stats is wrong (parallel_vacuum_end), it only traps workers allocated for parallel_vacuum_cleanup_all_indexes() and not parallel_vacuum_bulkdel_all_indexes(). Back to the drawing board... -- Benoit Lobréau Consultant http://dalibo.com
Re: Parallel workers stats in pg_stat_database
Hi, On Tue, Sep 03, 2024 at 02:34:06PM +0200, Benoit Lobréau wrote: > I noticed that the tests are still not stable. I tried using tenk2 > but fail to have stable plans. I'd love to have pointers on that front. What about moving the tests to places where it's "guaranteed" to get parallel workers involved? For example, a "parallel_maint_workers" only test could be done in vacuum_parallel.sql. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Parallel workers stats in pg_stat_database
Hi, This new version avoids updating the stats for non parallel queries. I noticed that the tests are still not stable. I tried using tenk2 but fail to have stable plans. I'd love to have pointers on that front. -- Benoit Lobréau Consultant http://dalibo.comFrom 5e4401c865f77ed447b8b3f25aac0ffa9af0d700 Mon Sep 17 00:00:00 2001 From: benoit Date: Wed, 28 Aug 2024 02:27:13 +0200 Subject: [PATCH] Adds four parallel workers stat columns to pg_stat_database * parallel_workers_planned * parallel_workers_launched * parallel_maint_workers_planned * parallel_maint_workers_launched --- doc/src/sgml/monitoring.sgml | 36 src/backend/access/brin/brin.c | 4 +++ src/backend/access/nbtree/nbtsort.c | 4 +++ src/backend/catalog/system_views.sql | 4 +++ src/backend/commands/vacuumparallel.c| 5 +++ src/backend/executor/execMain.c | 7 src/backend/executor/execUtils.c | 3 ++ src/backend/executor/nodeGather.c| 3 ++ src/backend/executor/nodeGatherMerge.c | 3 ++ src/backend/utils/activity/pgstat_database.c | 36 src/backend/utils/adt/pgstatfuncs.c | 12 +++ src/include/catalog/pg_proc.dat | 20 +++ src/include/nodes/execnodes.h| 3 ++ src/include/pgstat.h | 7 src/test/regress/expected/rules.out | 4 +++ src/test/regress/expected/stats.out | 17 + src/test/regress/sql/stats.sql | 14 17 files changed, 182 insertions(+) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 55417a6fa9..8c4b11c11d 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -3508,6 +3508,42 @@ description | Waiting for a newly initialized WAL file to reach durable storage + + + parallel_workers_planned bigint + + + Number of parallel workers planned by queries on this database + + + + + + parallel_workers_launched bigint + + + Number of parallel workers obtained by queries on this database + + + + + + parallel_maint_workers_planned bigint + + + Number of parallel workers planned by utilities on this database + + + + + + parallel_maint_workers_launched bigint + + + Number of parallel workers obtained by utilities on this database + + + stats_reset timestamp with time zone diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 6467bed604..9eceb87b52 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -2540,6 +2540,10 @@ _brin_end_parallel(BrinLeader *brinleader, BrinBuildState *state) /* Shutdown worker processes */ WaitForParallelWorkersToFinish(brinleader->pcxt); + pgstat_update_parallel_maint_workers_stats( + (PgStat_Counter) brinleader->pcxt->nworkers_to_launch, + (PgStat_Counter) brinleader->pcxt->nworkers_launched); + /* * Next, accumulate WAL usage. (This must wait for the workers to finish, * or we might get incomplete data.) diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c index f5d7b3b0c3..232e1a0942 100644 --- a/src/backend/access/nbtree/nbtsort.c +++ b/src/backend/access/nbtree/nbtsort.c @@ -1611,6 +1611,10 @@ _bt_end_parallel(BTLeader *btleader) /* Shutdown worker processes */ WaitForParallelWorkersToFinish(btleader->pcxt); + pgstat_update_parallel_maint_workers_stats( + (PgStat_Counter) btleader->pcxt->nworkers_to_launch, + (PgStat_Counter) btleader->pcxt->nworkers_launched); + /* * Next, accumulate WAL usage. (This must wait for the workers to finish, * or we might get incomplete data.) diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 19cabc9a47..48bf9e5535 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -1073,6 +1073,10 @@ CREATE VIEW pg_stat_database AS pg_stat_get_db_sessions_abandoned(D.oid) AS sessions_abandoned, pg_stat_get_db_sessions_fatal(D.oid) AS sessions_fatal, pg_stat_get_db_sessions_killed(D.oid) AS sessions_killed, +pg_stat_get_db_parallel_workers_planned(D.oid) as parallel_workers_planned, +pg_stat_get_db_parallel_workers_launched(D.oid) as parallel_workers_launched, +pg_stat_get_db_parallel_maint_workers_planned(D.oid) as parallel_maint_workers_planned, +pg_stat_get_db_parallel_maint_workers_launched(D.oid) as parallel_maint_workers_launched, pg_stat_get_db_stat_reset_time(D.oid) AS stats_reset FROM (
Re: Parallel workers stats in pg_stat_database
Hi, This is a new patch which: * fixes some typos * changes the execgather / execgathermerge code so that the stats are accumulated in EState and inserted in pg_stat_database only once, during ExecutorEnd * adds tests (very ugly, but I could get the parallel plan to be stable across make check executions.) On 8/28/24 17:10, Benoit Lobréau wrote: Hi hackers, This patch introduces four new columns in pg_stat_database: * parallel_worker_planned * parallel_worker_launched * parallel_maint_worker_planned * parallel_maint_worker_launched The intent is to help administrators evaluate the usage of parallel workers in their databases and help sizing max_worker_processes, max_parallel_workers or max_parallel_maintenance_workers). Here is a test script: psql << _EOF_ -- Index creation DROP TABLE IF EXISTS test_pql; CREATE TABLE test_pql(i int, j int); INSERT INTO test_pql SELECT x,x FROM generate_series(1,100) as F(x); -- 0 planned / 0 launched EXPLAIN (ANALYZE) SELECT 1; -- 2 planned / 2 launched EXPLAIN (ANALYZE) SELECT i, avg(j) FROM test_pql GROUP BY i; SET max_parallel_workers TO 1; -- 4 planned / 1 launched EXPLAIN (ANALYZE) SELECT i, avg(j) FROM test_pql GROUP BY i UNION SELECT i, avg(j) FROM test_pql GROUP BY i; RESET max_parallel_workers; -- 1 planned / 1 launched CREATE INDEX ON test_pql(i); SET max_parallel_workers TO 0; -- 1 planned / 0 launched CREATE INDEX ON test_pql(j); -- 1 planned / 0 launched CREATE INDEX ON test_pql(i, j); SET maintenance_work_mem TO '96MB'; RESET max_parallel_workers; -- 2 planned / 2 launched VACUUM (VERBOSE) test_pql; SET max_parallel_workers TO 1; -- 2 planned / 1 launched VACUUM (VERBOSE) test_pql; -- TOTAL: parallel workers: 6 planned / 3 launched -- TOTAL: parallel maint workers: 7 planned / 4 launched _EOF_ And the output in pg_stat_database a fresh server without any configuration change except thoses in the script: [local]:5445 postgres@postgres=# SELECT datname, parallel_workers_planned, parallel_workers_launched, parallel_maint_workers_planned, parallel_maint_workers_launched FROM pg _stat_database WHERE datname = 'postgres' \gx -[ RECORD 1 ]---+- datname | postgres parallel_workers_planned | 6 parallel_workers_launched | 3 parallel_maint_workers_planned | 7 parallel_maint_workers_launched | 4 Thanks to: Jehan-Guillaume de Rorthais, Guillaume Lelarge and Franck Boudehen for the help and motivation boost. --- Benoit Lobréau Consultant http://dalibo.com -- Benoit Lobréau Consultant http://dalibo.comFrom 0338cfb11ab98594b2f16d143b505e269566bb6e Mon Sep 17 00:00:00 2001 From: benoit Date: Wed, 28 Aug 2024 02:27:13 +0200 Subject: [PATCH] Adds four parallel workers stat columns to pg_stat_database * parallel_workers_planned * parallel_workers_launched * parallel_maint_workers_planned * parallel_maint_workers_launched --- doc/src/sgml/monitoring.sgml | 36 src/backend/access/brin/brin.c | 4 +++ src/backend/access/nbtree/nbtsort.c | 4 +++ src/backend/catalog/system_views.sql | 4 +++ src/backend/commands/vacuumparallel.c| 5 +++ src/backend/executor/execMain.c | 5 +++ src/backend/executor/execUtils.c | 3 ++ src/backend/executor/nodeGather.c| 3 ++ src/backend/executor/nodeGatherMerge.c | 3 ++ src/backend/utils/activity/pgstat_database.c | 36 src/backend/utils/adt/pgstatfuncs.c | 12 +++ src/include/catalog/pg_proc.dat | 20 +++ src/include/nodes/execnodes.h| 3 ++ src/include/pgstat.h | 7 src/test/regress/expected/rules.out | 4 +++ src/test/regress/expected/stats.out | 17 + src/test/regress/sql/stats.sql | 14 17 files changed, 180 insertions(+) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 55417a6fa9..8c4b11c11d 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -3508,6 +3508,42 @@ description | Waiting for a newly initialized WAL file to reach durable storage + + + parallel_workers_planned bigint + + + Number of parallel workers planned by queries on this database + + + + + + parallel_workers_launched bigint + + + Number of parallel workers obtained by queries on this database + + + + + + parallel_maint_workers_planned bigint + + + Number of parallel workers planned by utilities on this database + + + + + + parallel_maint_workers_launched bigint + + + Number of parallel workers obtained by utilities on this database + + + stats_reset timestamp with time zone diff --git a
