Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)

On Sun, Mar 29, 2020 at 9:52 AM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Sun, 29 Mar 2020 at 15:19, Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
> >
> > On Sun, 29 Mar 2020 at 14:23, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Sat, Mar 28, 2020 at 8:47 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > > >
> > > > On Sat, Mar 28, 2020 at 02:38:27PM +0100, Julien Rouhaud wrote:
> > > > > On Sat, Mar 28, 2020 at 04:14:04PM +0530, Amit Kapila wrote:
> > > > > >
> > > > > > I see some basic problems with the patch.  The way it tries to compute
> > > > > > WAL usage for parallel stuff doesn't seem right to me.  Can you share
> > > > > > or point me to any test done where we have computed WAL for parallel
> > > > > > operations like Parallel Vacuum or Parallel Create Index?
> > > > >
> > > > > Ah, that's indeed a good point and AFAICT WAL records from parallel utility
> > > > > workers won't be accounted for.  That being said, I think that an argument
> > > > > could be made that proper infrastructure should have been added in the original
> > > > > parallel utility patches, as pg_stat_statement is already broken wrt. buffer
> > > > > usage in parallel utility, unless I'm missing something.
> > > >
> > > > Just to be sure I did a quick test with pg_stat_statements behavior using
> > > > parallel/non-parallel CREATE INDEX and VACUUM, and unsurprisingly buffer usage
> > > > doesn't reflect parallel workers' activity.
> > > >
> > >
> > > Sawada-San would like to investigate this? If not, I will look into
> > > this next week.
> >
> > Sure, I'll investigate this issue today.

Thanks for looking at it!

> I've run vacuum with/without parallel workers on the table having 5
> indexes. The vacuum reads all blocks of table and indexes.
>
> * VACUUM command with no parallel workers
> =# select total_time, shared_blks_hit, shared_blks_read,
> shared_blks_hit + shared_blks_read as total_read_blks,
> shared_blks_dirtied, shared_blks_written from pg_stat_statements where
> query ~ 'vacuum';
>
>   total_time  | shared_blks_hit | shared_blks_read | total_read_blks |
> shared_blks_dirtied | shared_blks_written
> --------------+-----------------+------------------+-----------------+---------------------+---------------------
>  19857.217207 |           45238 |           226944 |          272182 |
>              225943 |              225894
> (1 row)
>
> * VACUUM command with 4 parallel workers
> =# select total_time, shared_blks_hit, shared_blks_read,
> shared_blks_hit + shared_blks_read as total_read_blks,
> shared_blks_dirtied, shared_blks_written from pg_stat_statements where
> query ~ 'vacuum';
>
>  total_time  | shared_blks_hit | shared_blks_read | total_read_blks |
> shared_blks_dirtied | shared_blks_written
> -------------+-----------------+------------------+-----------------+---------------------+---------------------
>  6932.117365 |           45205 |            73079 |          118284 |
>              72403 |               72365
> (1 row)
>
> The total number of blocks of table and indexes are about 182243
> blocks. As Julien reported, obviously the total number of read blocks
> during parallel vacuum is much less than single process vacuum's
> result.
>
> Parallel create index has the same issue but it doesn't exist in
> parallel queries for SELECTs.
>
> I think we need to change parallel maintenance commands so that they
> report buffer usage like what ParallelQueryMain() does; prepare to
> track buffer usage during query execution by
> InstrStartParallelQuery(), and report it by InstrEndParallelQuery()
> after parallel maintenance command. To report buffer usage of parallel
> maintenance command correctly, I'm thinking that we can (1) change
> parallel create index and parallel vacuum so that they prepare
> gathering buffer usage, or (2) have a common entry point for parallel
> maintenance commands that is responsible for gathering buffer usage
> and calling the entry functions for individual maintenance command.
> I'll investigate it more in depth.

As I just mentioned, (2) seems like a better design as it's quite
likely that the number of parallel-aware utilities will probably
continue to increase.  One problem also is that parallel CREATE INDEX
has been introduced in pg11, so (2) probably won't be packpatchable
(and (1) seems problematic too).