Thread: 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
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
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-cb85a81d0383@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
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
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 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
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
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