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,1000000) 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.com

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.com

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

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

Here is an updated patch fixing the aforementionned problems
with tests and vacuum stats.

--
Benoit Lobréau
Consultant
http://dalibo.com

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

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]/messages/by-id/d657df20-c4bf-63f6-e74c-cb85a81d0383@dalibo.com. We thought about several
places to do it for several use cases.

Guillaume Lelarge worked on pg_stat_statements [1]/messages/by-id/CAECtzeWtTGOK0UgKXdDGpfTVSa5bd_VbUt6K6xn8P7X+_dZqKw@mail.gmail.com and
pg_stat_user_[tables|indexes] [2]/messages/by-id/CAECtzeXXuMkw-RVGTWvHGOJsmFdsRY+jK0ndQa80sw46y2uvVQ@mail.gmail.com. I proposed a patch for the logs [3]/messages/by-id/8123423a-f041-4f4c-a771-bfd96ab235b0@dalibo.com.

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]: /messages/by-id/d657df20-c4bf-63f6-e74c-cb85a81d0383@dalibo.com
/messages/by-id/d657df20-c4bf-63f6-e74c-cb85a81d0383@dalibo.com
[1]: /messages/by-id/CAECtzeWtTGOK0UgKXdDGpfTVSa5bd_VbUt6K6xn8P7X+_dZqKw@mail.gmail.com
/messages/by-id/CAECtzeWtTGOK0UgKXdDGpfTVSa5bd_VbUt6K6xn8P7X+_dZqKw@mail.gmail.com
[2]: /messages/by-id/CAECtzeXXuMkw-RVGTWvHGOJsmFdsRY+jK0ndQa80sw46y2uvVQ@mail.gmail.com
/messages/by-id/CAECtzeXXuMkw-RVGTWvHGOJsmFdsRY+jK0ndQa80sw46y2uvVQ@mail.gmail.com
[3]: /messages/by-id/8123423a-f041-4f4c-a771-bfd96ab235b0@dalibo.com
/messages/by-id/8123423a-f041-4f4c-a771-bfd96ab235b0@dalibo.com

--
Benoit Lobréau
Consultant
http://dalibo.com

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:
/messages/by-id/CAECtzeWtTGOK0UgKXdDGpfTVSa5bd_VbUt6K6xn8P7X+_dZqKw@mail.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

Hey,

Le mer. 2 oct. 2024 à 11:12, Benoit Lobréau <benoit.lobreau@dalibo.com> 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.

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

Le mar. 8 oct. 2024 à 14:03, Benoit Lobréau <benoit.lobreau@dalibo.com> 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.

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.com

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

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

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

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

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

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

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