pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedpg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
43+ messages / 6 participants
[nested] [flat]
Commits from this thread:
2020-04-09 13823ad3b0 f179e9f01b 5c71362174 Allow parallel create index to accumulate buffer usage stats.
2020-04-02 3a5e22138a Allow parallel vacuum to accumulate buffer usage.
* pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-03-28 15:17 Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 2 replies; 43+ messages in thread
From: Julien Rouhaud @ 2020-03-28 15:17 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: pgsql-hackers
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.
I added an open for that, and adding Robert in Cc as 9da0cc352 is the first
commit adding parallel maintenance.
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-03-29 05:23 Amit Kapila <amit.kapila16@gmail.com>
parent: Julien Rouhaud <rjuju123@gmail.com>
1 sibling, 1 reply; 43+ messages in thread
From: Amit Kapila @ 2020-03-29 05:23 UTC (permalink / raw)
To: Julien Rouhaud <rjuju123@gmail.com>; Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; Dilip Kumar <dilipbalaut@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
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.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-03-29 06:19 Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 43+ messages in thread
From: Masahiko Sawada @ 2020-03-29 06:19 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Julien Rouhaud <rjuju123@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
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.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-03-29 07:51 Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
parent: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
0 siblings, 1 reply; 43+ messages in thread
From: Masahiko Sawada @ 2020-03-29 07:51 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Julien Rouhaud <rjuju123@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
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.
>
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.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-03-29 08:13 Julien Rouhaud <rjuju123@gmail.com>
parent: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
0 siblings, 1 reply; 43+ messages in thread
From: Julien Rouhaud @ 2020-03-29 08:13 UTC (permalink / raw)
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
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).
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-03-29 11:15 Amit Kapila <amit.kapila16@gmail.com>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 43+ messages in thread
From: Amit Kapila @ 2020-03-29 11:15 UTC (permalink / raw)
To: Julien Rouhaud <rjuju123@gmail.com>; +Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Sun, Mar 29, 2020 at 1:44 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Sun, Mar 29, 2020 at 9:52 AM Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
> >
> > 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).
>
I am not sure if we can decide at this stage whether it is
back-patchable or not. Let's first see the patch and if it turns out
to be complex, then we can try to do some straight-forward fix for
back-branches. In general, I don't see why the fix here should be
complex?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-03-29 11:44 Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 43+ messages in thread
From: Masahiko Sawada @ 2020-03-29 11:44 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Julien Rouhaud <rjuju123@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Sun, 29 Mar 2020 at 20:15, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sun, Mar 29, 2020 at 1:44 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> >
> > On Sun, Mar 29, 2020 at 9:52 AM Masahiko Sawada
> > <masahiko.sawada@2ndquadrant.com> wrote:
> > >
> > > 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).
> >
>
> I am not sure if we can decide at this stage whether it is
> back-patchable or not. Let's first see the patch and if it turns out
> to be complex, then we can try to do some straight-forward fix for
> back-branches.
Agreed.
> In general, I don't see why the fix here should be
> complex?
Yeah, particularly the approach (1) will not be complex. I'll write a
patch tomorrow.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-03-30 06:46 Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
parent: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
0 siblings, 1 reply; 43+ messages in thread
From: Masahiko Sawada @ 2020-03-30 06:46 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Julien Rouhaud <rjuju123@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Sun, 29 Mar 2020 at 20:44, Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Sun, 29 Mar 2020 at 20:15, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Sun, Mar 29, 2020 at 1:44 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > >
> > > On Sun, Mar 29, 2020 at 9:52 AM Masahiko Sawada
> > > <masahiko.sawada@2ndquadrant.com> wrote:
> > > >
> > > > 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).
> > >
> >
> > I am not sure if we can decide at this stage whether it is
> > back-patchable or not. Let's first see the patch and if it turns out
> > to be complex, then we can try to do some straight-forward fix for
> > back-branches.
>
> Agreed.
>
> > In general, I don't see why the fix here should be
> > complex?
>
> Yeah, particularly the approach (1) will not be complex. I'll write a
> patch tomorrow.
>
I've attached two patches fixing this issue for parallel index
creation and parallel vacuum. These approaches take the same approach;
we allocate DSM to share buffer usage and the leader gathers them,
described as approach (1) above. I think this is a straightforward
approach for this issue. We can create a common entry point for
parallel maintenance command that is responsible for gathering buffer
usage as well as sharing query text etc. But it will accompany
relatively big change and it might be overkill at this stage. We can
discuss that and it will become an item for PG14.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments: download all (2 files, .tar.gz)
bufferusage_vacuum.patch (application/octet-stream, 4.4K) download
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 03c43efc32..bc08651df1 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -65,6 +65,7 @@
#include "commands/dbcommands.h"
#include "commands/progress.h"
#include "commands/vacuum.h"
+#include "executor/instrument.h"
#include "miscadmin.h"
#include "optimizer/paths.h"
#include "pgstat.h"
@@ -137,6 +138,7 @@
#define PARALLEL_VACUUM_KEY_SHARED 1
#define PARALLEL_VACUUM_KEY_DEAD_TUPLES 2
#define PARALLEL_VACUUM_KEY_QUERY_TEXT 3
+#define PARALLEL_VACUUM_KEY_BUFFER_USAGE 4
/*
* Macro to check if we are in a parallel vacuum. If true, we are in the
@@ -259,6 +261,9 @@ typedef struct LVParallelState
/* Shared information among parallel vacuum workers */
LVShared *lvshared;
+ /* Points to buffer usage are in DSM */
+ BufferUsage *buffer_usage;
+
/*
* The number of indexes that support parallel index bulk-deletion and
* parallel index cleanup respectively.
@@ -1991,6 +1996,7 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
int nindexes)
{
int nworkers;
+ int i;
Assert(!IsParallelWorker());
Assert(ParallelVacuumIsActive(lps));
@@ -2088,6 +2094,13 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
/* Wait for all vacuum workers to finish */
WaitForParallelWorkersToFinish(lps->pcxt);
+ /*
+ * Next, accumulate buffer usage. (This must wait for the workers to
+ * finish, or we might get incomplete data.)
+ */
+ for (i = 0; i < nworkers; i++)
+ InstrAccumParallelQuery(&lps->buffer_usage[i]);
+
/*
* Carry the shared balance value to heap scan and disable shared costing
*/
@@ -3071,6 +3084,7 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats,
ParallelContext *pcxt;
LVShared *shared;
LVDeadTuples *dead_tuples;
+ BufferUsage *buffer_usage;
bool *can_parallel_vacuum;
long maxtuples;
char *sharedquery;
@@ -3154,6 +3168,18 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats,
shm_toc_estimate_chunk(&pcxt->estimator, est_deadtuples);
shm_toc_estimate_keys(&pcxt->estimator, 1);
+ /*
+ * Estimate space for BufferUsage -- PARALLEL_VACUUM_KEY_BUFFER_USAGE.
+ *
+ * BufferUsage during executing maintenance command can be used by an
+ * extension that reports the buffer usage, such as pg_stat_statements.
+ * We have no way of knowing whether anyone's looking at pgBufferUsage,
+ * so do it unconditionally.
+ */
+ shm_toc_estimate_chunk(&pcxt->estimator,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_estimate_keys(&pcxt->estimator, 1);
+
/* Finally, estimate PARALLEL_VACUUM_KEY_QUERY_TEXT space */
querylen = strlen(debug_query_string);
shm_toc_estimate_chunk(&pcxt->estimator, querylen + 1);
@@ -3188,6 +3214,12 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats,
shm_toc_insert(pcxt->toc, PARALLEL_VACUUM_KEY_DEAD_TUPLES, dead_tuples);
vacrelstats->dead_tuples = dead_tuples;
+ /* Allocate space for each worker's BufferUsage; no need to initialize */
+ buffer_usage = shm_toc_allocate(pcxt->toc,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_insert(pcxt->toc, PARALLEL_VACUUM_KEY_BUFFER_USAGE, buffer_usage);
+ lps->buffer_usage = buffer_usage;
+
/* Store query string for workers */
sharedquery = (char *) shm_toc_allocate(pcxt->toc, querylen + 1);
memcpy(sharedquery, debug_query_string, querylen + 1);
@@ -3317,6 +3349,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
Relation *indrels;
LVShared *lvshared;
LVDeadTuples *dead_tuples;
+ BufferUsage *buffer_usage;
int nindexes;
char *sharedquery;
IndexBulkDeleteResult **stats;
@@ -3369,10 +3402,18 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
if (lvshared->maintenance_work_mem_worker > 0)
maintenance_work_mem = lvshared->maintenance_work_mem_worker;
+ /* Prepare to track buffer usage during parallel execution */
+ InstrStartParallelQuery();
+
/* Process indexes to perform vacuum/cleanup */
parallel_vacuum_index(indrels, stats, lvshared, dead_tuples, nindexes);
+ /* Report buffer usage during parallel execution */
+ buffer_usage = shm_toc_lookup(toc, PARALLEL_VACUUM_KEY_BUFFER_USAGE, false);
+ InstrEndParallelQuery(&buffer_usage[ParallelWorkerNumber]);
+
vac_close_indexes(nindexes, indrels, RowExclusiveLock);
table_close(onerel, ShareUpdateExclusiveLock);
+
pfree(stats);
}
bufferusage_create_index.patch (application/octet-stream, 4.5K) download
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index e66cd36dfa..13d6eb76c3 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -67,6 +67,7 @@
#include "access/xloginsert.h"
#include "catalog/index.h"
#include "commands/progress.h"
+#include "executor/instrument.h"
#include "miscadmin.h"
#include "pgstat.h"
#include "storage/smgr.h"
@@ -81,6 +82,7 @@
#define PARALLEL_KEY_TUPLESORT UINT64CONST(0xA000000000000002)
#define PARALLEL_KEY_TUPLESORT_SPOOL2 UINT64CONST(0xA000000000000003)
#define PARALLEL_KEY_QUERY_TEXT UINT64CONST(0xA000000000000004)
+#define PARALLEL_KEY_BUFFER_USAGE UINT64CONST(0xA000000000000005)
/*
* DISABLE_LEADER_PARTICIPATION disables the leader's participation in
@@ -203,6 +205,7 @@ typedef struct BTLeader
Sharedsort *sharedsort;
Sharedsort *sharedsort2;
Snapshot snapshot;
+ BufferUsage *bufferusage;
} BTLeader;
/*
@@ -1474,6 +1477,7 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
Sharedsort *sharedsort2;
BTSpool *btspool = buildstate->spool;
BTLeader *btleader = (BTLeader *) palloc0(sizeof(BTLeader));
+ BufferUsage *bufferusage;
bool leaderparticipates = true;
char *sharedquery;
int querylen;
@@ -1526,6 +1530,18 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
shm_toc_estimate_keys(&pcxt->estimator, 3);
}
+ /*
+ * Estimate space for BufferUsage -- PARALLEL_KEY_BUFFER_USAGE
+ *
+ * BufferUsage during executing maintenance command can be used by an
+ * extension that reports the buffer usage, such as pg_stat_statements.
+ * We have no way of knowing whether anyone's looking at pgBufferUsage,
+ * so do it unconditionally.
+ */
+ shm_toc_estimate_chunk(&pcxt->estimator,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_estimate_keys(&pcxt->estimator, 1);
+
/* Finally, estimate PARALLEL_KEY_QUERY_TEXT space */
querylen = strlen(debug_query_string);
shm_toc_estimate_chunk(&pcxt->estimator, querylen + 1);
@@ -1597,6 +1613,11 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
memcpy(sharedquery, debug_query_string, querylen + 1);
shm_toc_insert(pcxt->toc, PARALLEL_KEY_QUERY_TEXT, sharedquery);
+ /* Allocate space for each worker's BufferUsage; no need to initialize */
+ bufferusage = shm_toc_allocate(pcxt->toc,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_insert(pcxt->toc, PARALLEL_KEY_BUFFER_USAGE, bufferusage);
+
/* Launch workers, saving status for leader/caller */
LaunchParallelWorkers(pcxt);
btleader->pcxt = pcxt;
@@ -1607,6 +1628,7 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
btleader->sharedsort = sharedsort;
btleader->sharedsort2 = sharedsort2;
btleader->snapshot = snapshot;
+ btleader->bufferusage = bufferusage;
/* If no workers were successfully launched, back out (do serial build) */
if (pcxt->nworkers_launched == 0)
@@ -1635,8 +1657,18 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
static void
_bt_end_parallel(BTLeader *btleader)
{
+ int i;
+
/* Shutdown worker processes */
WaitForParallelWorkersToFinish(btleader->pcxt);
+
+ /*
+ * Next, accumulate buffer usage. (This must wait for the workers to
+ * finish, or we might get incomplete data.)
+ */
+ for (i = 0; i < btleader->pcxt->nworkers; i++)
+ InstrAccumParallelQuery(&btleader->bufferusage[i]);
+
/* Free last reference to MVCC snapshot, if one was used */
if (IsMVCCSnapshot(btleader->snapshot))
UnregisterSnapshot(btleader->snapshot);
@@ -1767,6 +1799,7 @@ _bt_parallel_build_main(dsm_segment *seg, shm_toc *toc)
Relation indexRel;
LOCKMODE heapLockmode;
LOCKMODE indexLockmode;
+ BufferUsage *bufferusage;
int sortmem;
#ifdef BTREE_BUILD_STATS
@@ -1828,11 +1861,18 @@ _bt_parallel_build_main(dsm_segment *seg, shm_toc *toc)
tuplesort_attach_shared(sharedsort2, seg);
}
+ /* Prepare to track buffer usage during parallel execution */
+ InstrStartParallelQuery();
+
/* Perform sorting of spool, and possibly a spool2 */
sortmem = maintenance_work_mem / btshared->scantuplesortstates;
_bt_parallel_scan_and_sort(btspool, btspool2, btshared, sharedsort,
sharedsort2, sortmem, false);
+ /* Report buffer usage during parallel execution */
+ bufferusage = shm_toc_lookup(toc, PARALLEL_KEY_BUFFER_USAGE, false);
+ InstrEndParallelQuery(&bufferusage[ParallelWorkerNumber]);
+
#ifdef BTREE_BUILD_STATS
if (log_btree_build_stats)
{
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-03-30 07:01 Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
parent: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
0 siblings, 2 replies; 43+ messages in thread
From: Masahiko Sawada @ 2020-03-30 07:01 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Julien Rouhaud <rjuju123@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Mon, 30 Mar 2020 at 15:46, Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Sun, 29 Mar 2020 at 20:44, Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
> >
> > On Sun, 29 Mar 2020 at 20:15, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Sun, Mar 29, 2020 at 1:44 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > > >
> > > > On Sun, Mar 29, 2020 at 9:52 AM Masahiko Sawada
> > > > <masahiko.sawada@2ndquadrant.com> wrote:
> > > > >
> > > > > 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).
> > > >
> > >
> > > I am not sure if we can decide at this stage whether it is
> > > back-patchable or not. Let's first see the patch and if it turns out
> > > to be complex, then we can try to do some straight-forward fix for
> > > back-branches.
> >
> > Agreed.
> >
> > > In general, I don't see why the fix here should be
> > > complex?
> >
> > Yeah, particularly the approach (1) will not be complex. I'll write a
> > patch tomorrow.
> >
>
> I've attached two patches fixing this issue for parallel index
> creation and parallel vacuum. These approaches take the same approach;
> we allocate DSM to share buffer usage and the leader gathers them,
> described as approach (1) above. I think this is a straightforward
> approach for this issue. We can create a common entry point for
> parallel maintenance command that is responsible for gathering buffer
> usage as well as sharing query text etc. But it will accompany
> relatively big change and it might be overkill at this stage. We can
> discuss that and it will become an item for PG14.
>
The patch for vacuum conflicts with recent changes in vacuum. So I've
attached rebased one.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
bufferusage_vacuum_v2.patch (application/octet-stream, 4.4K) download
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 9726f69629..e8e3be0cba 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -65,6 +65,7 @@
#include "commands/dbcommands.h"
#include "commands/progress.h"
#include "commands/vacuum.h"
+#include "executor/instrument.h"
#include "miscadmin.h"
#include "optimizer/paths.h"
#include "pgstat.h"
@@ -137,6 +138,7 @@
#define PARALLEL_VACUUM_KEY_SHARED 1
#define PARALLEL_VACUUM_KEY_DEAD_TUPLES 2
#define PARALLEL_VACUUM_KEY_QUERY_TEXT 3
+#define PARALLEL_VACUUM_KEY_BUFFER_USAGE 4
/*
* Macro to check if we are in a parallel vacuum. If true, we are in the
@@ -270,6 +272,9 @@ typedef struct LVParallelState
/* Shared information among parallel vacuum workers */
LVShared *lvshared;
+ /* Points to buffer usage are in DSM */
+ BufferUsage *buffer_usage;
+
/*
* The number of indexes that support parallel index bulk-deletion and
* parallel index cleanup respectively.
@@ -2069,6 +2074,7 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
int nindexes)
{
int nworkers;
+ int i;
Assert(!IsParallelWorker());
Assert(ParallelVacuumIsActive(lps));
@@ -2166,6 +2172,13 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
/* Wait for all vacuum workers to finish */
WaitForParallelWorkersToFinish(lps->pcxt);
+ /*
+ * Next, accumulate buffer usage. (This must wait for the workers to
+ * finish, or we might get incomplete data.)
+ */
+ for (i = 0; i < nworkers; i++)
+ InstrAccumParallelQuery(&lps->buffer_usage[i]);
+
/*
* Carry the shared balance value to heap scan and disable shared costing
*/
@@ -3179,6 +3192,7 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats,
ParallelContext *pcxt;
LVShared *shared;
LVDeadTuples *dead_tuples;
+ BufferUsage *buffer_usage;
bool *can_parallel_vacuum;
long maxtuples;
char *sharedquery;
@@ -3262,6 +3276,18 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats,
shm_toc_estimate_chunk(&pcxt->estimator, est_deadtuples);
shm_toc_estimate_keys(&pcxt->estimator, 1);
+ /*
+ * Estimate space for BufferUsage -- PARALLEL_VACUUM_KEY_BUFFER_USAGE.
+ *
+ * BufferUsage during executing maintenance command can be used by an
+ * extension that reports the buffer usage, such as pg_stat_statements.
+ * We have no way of knowing whether anyone's looking at pgBufferUsage,
+ * so do it unconditionally.
+ */
+ shm_toc_estimate_chunk(&pcxt->estimator,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_estimate_keys(&pcxt->estimator, 1);
+
/* Finally, estimate PARALLEL_VACUUM_KEY_QUERY_TEXT space */
querylen = strlen(debug_query_string);
shm_toc_estimate_chunk(&pcxt->estimator, querylen + 1);
@@ -3296,6 +3322,12 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats,
shm_toc_insert(pcxt->toc, PARALLEL_VACUUM_KEY_DEAD_TUPLES, dead_tuples);
vacrelstats->dead_tuples = dead_tuples;
+ /* Allocate space for each worker's BufferUsage; no need to initialize */
+ buffer_usage = shm_toc_allocate(pcxt->toc,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_insert(pcxt->toc, PARALLEL_VACUUM_KEY_BUFFER_USAGE, buffer_usage);
+ lps->buffer_usage = buffer_usage;
+
/* Store query string for workers */
sharedquery = (char *) shm_toc_allocate(pcxt->toc, querylen + 1);
memcpy(sharedquery, debug_query_string, querylen + 1);
@@ -3425,6 +3457,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
Relation *indrels;
LVShared *lvshared;
LVDeadTuples *dead_tuples;
+ BufferUsage *buffer_usage;
int nindexes;
char *sharedquery;
IndexBulkDeleteResult **stats;
@@ -3494,10 +3527,17 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
errcallback.previous = error_context_stack;
error_context_stack = &errcallback;
+ /* Prepare to track buffer usage during parallel execution */
+ InstrStartParallelQuery();
+
/* Process indexes to perform vacuum/cleanup */
parallel_vacuum_index(indrels, stats, lvshared, dead_tuples, nindexes,
&vacrelstats);
+ /* Report buffer usage during parallel execution */
+ buffer_usage = shm_toc_lookup(toc, PARALLEL_VACUUM_KEY_BUFFER_USAGE, false);
+ InstrEndParallelQuery(&buffer_usage[ParallelWorkerNumber]);
+
/* Pop the error context stack */
error_context_stack = errcallback.previous;
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-03-30 08:00 Julien Rouhaud <rjuju123@gmail.com>
parent: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
1 sibling, 0 replies; 43+ messages in thread
From: Julien Rouhaud @ 2020-03-30 08:00 UTC (permalink / raw)
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Mon, Mar 30, 2020 at 04:01:18PM +0900, Masahiko Sawada wrote:
> On Mon, 30 Mar 2020 at 15:46, Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
> >
> > On Sun, 29 Mar 2020 at 20:44, Masahiko Sawada
> > <masahiko.sawada@2ndquadrant.com> wrote:
> > >
> > > > > > 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.
> > > > >
> > > [...]
> >
> > I've attached two patches fixing this issue for parallel index
> > creation and parallel vacuum. These approaches take the same approach;
> > we allocate DSM to share buffer usage and the leader gathers them,
> > described as approach (1) above. I think this is a straightforward
> > approach for this issue. We can create a common entry point for
> > parallel maintenance command that is responsible for gathering buffer
> > usage as well as sharing query text etc. But it will accompany
> > relatively big change and it might be overkill at this stage. We can
> > discuss that and it will become an item for PG14.
> >
>
> The patch for vacuum conflicts with recent changes in vacuum. So I've
> attached rebased one.
Thanks Sawada-san!
Just minor nitpicking:
+ int i;
Assert(!IsParallelWorker());
Assert(ParallelVacuumIsActive(lps));
@@ -2166,6 +2172,13 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
/* Wait for all vacuum workers to finish */
WaitForParallelWorkersToFinish(lps->pcxt);
+ /*
+ * Next, accumulate buffer usage. (This must wait for the workers to
+ * finish, or we might get incomplete data.)
+ */
+ for (i = 0; i < nworkers; i++)
+ InstrAccumParallelQuery(&lps->buffer_usage[i]);
We now allow declaring a variable in those loops, so it may be better to avoid
declaring i outside the for scope?
Other than that both patch looks good to me and a good fit for packpatching. I
also did some testing on VACUUM and CREATE INDEX and it works as expected.
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-03-31 03:57 Amit Kapila <amit.kapila16@gmail.com>
parent: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
1 sibling, 1 reply; 43+ messages in thread
From: Amit Kapila @ 2020-03-31 03:57 UTC (permalink / raw)
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; +Cc: Julien Rouhaud <rjuju123@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Mon, Mar 30, 2020 at 12:31 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> The patch for vacuum conflicts with recent changes in vacuum. So I've
> attached rebased one.
>
+ /*
+ * Next, accumulate buffer usage. (This must wait for the workers to
+ * finish, or we might get incomplete data.)
+ */
+ for (i = 0; i < nworkers; i++)
+ InstrAccumParallelQuery(&lps->buffer_usage[i]);
+
This should be done for launched workers aka
lps->pcxt->nworkers_launched. I think a similar problem exists in
create index related patch.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-03-31 05:13 Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 2 replies; 43+ messages in thread
From: Masahiko Sawada @ 2020-03-31 05:13 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Julien Rouhaud <rjuju123@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Tue, 31 Mar 2020 at 12:58, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Mar 30, 2020 at 12:31 PM Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
> >
> > The patch for vacuum conflicts with recent changes in vacuum. So I've
> > attached rebased one.
> >
>
> + /*
> + * Next, accumulate buffer usage. (This must wait for the workers to
> + * finish, or we might get incomplete data.)
> + */
> + for (i = 0; i < nworkers; i++)
> + InstrAccumParallelQuery(&lps->buffer_usage[i]);
> +
>
> This should be done for launched workers aka
> lps->pcxt->nworkers_launched. I think a similar problem exists in
> create index related patch.
You're right. Fixed in the new patches.
On Mon, 30 Mar 2020 at 17:00, Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> Just minor nitpicking:
>
> + int i;
>
> Assert(!IsParallelWorker());
> Assert(ParallelVacuumIsActive(lps));
> @@ -2166,6 +2172,13 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
> /* Wait for all vacuum workers to finish */
> WaitForParallelWorkersToFinish(lps->pcxt);
>
> + /*
> + * Next, accumulate buffer usage. (This must wait for the workers to
> + * finish, or we might get incomplete data.)
> + */
> + for (i = 0; i < nworkers; i++)
> + InstrAccumParallelQuery(&lps->buffer_usage[i]);
>
> We now allow declaring a variable in those loops, so it may be better to avoid
> declaring i outside the for scope?
We can do that but I was not sure if it's good since other codes
around there don't use that. So I'd like to leave it for committers.
It's a trivial change.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments: download all (2 files, .tar.gz)
bufferusage_vacuum_v3.patch (application/octet-stream, 4.5K) download
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 03c43efc32..777c7c9e70 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -65,6 +65,7 @@
#include "commands/dbcommands.h"
#include "commands/progress.h"
#include "commands/vacuum.h"
+#include "executor/instrument.h"
#include "miscadmin.h"
#include "optimizer/paths.h"
#include "pgstat.h"
@@ -137,6 +138,7 @@
#define PARALLEL_VACUUM_KEY_SHARED 1
#define PARALLEL_VACUUM_KEY_DEAD_TUPLES 2
#define PARALLEL_VACUUM_KEY_QUERY_TEXT 3
+#define PARALLEL_VACUUM_KEY_BUFFER_USAGE 4
/*
* Macro to check if we are in a parallel vacuum. If true, we are in the
@@ -259,6 +261,9 @@ typedef struct LVParallelState
/* Shared information among parallel vacuum workers */
LVShared *lvshared;
+ /* Points to buffer usage are in DSM */
+ BufferUsage *buffer_usage;
+
/*
* The number of indexes that support parallel index bulk-deletion and
* parallel index cleanup respectively.
@@ -1991,6 +1996,7 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
int nindexes)
{
int nworkers;
+ int i;
Assert(!IsParallelWorker());
Assert(ParallelVacuumIsActive(lps));
@@ -2088,6 +2094,13 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
/* Wait for all vacuum workers to finish */
WaitForParallelWorkersToFinish(lps->pcxt);
+ /*
+ * Next, accumulate buffer usage. (This must wait for the workers to
+ * finish, or we might get incomplete data.)
+ */
+ for (i = 0; i < lps->pcxt->nworkers_launched; i++)
+ InstrAccumParallelQuery(&lps->buffer_usage[i]);
+
/*
* Carry the shared balance value to heap scan and disable shared costing
*/
@@ -3071,6 +3084,7 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats,
ParallelContext *pcxt;
LVShared *shared;
LVDeadTuples *dead_tuples;
+ BufferUsage *buffer_usage;
bool *can_parallel_vacuum;
long maxtuples;
char *sharedquery;
@@ -3154,6 +3168,18 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats,
shm_toc_estimate_chunk(&pcxt->estimator, est_deadtuples);
shm_toc_estimate_keys(&pcxt->estimator, 1);
+ /*
+ * Estimate space for BufferUsage -- PARALLEL_VACUUM_KEY_BUFFER_USAGE.
+ *
+ * BufferUsage during executing maintenance command can be used by an
+ * extension that reports the buffer usage, such as pg_stat_statements.
+ * We have no way of knowing whether anyone's looking at pgBufferUsage,
+ * so do it unconditionally.
+ */
+ shm_toc_estimate_chunk(&pcxt->estimator,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_estimate_keys(&pcxt->estimator, 1);
+
/* Finally, estimate PARALLEL_VACUUM_KEY_QUERY_TEXT space */
querylen = strlen(debug_query_string);
shm_toc_estimate_chunk(&pcxt->estimator, querylen + 1);
@@ -3188,6 +3214,12 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats,
shm_toc_insert(pcxt->toc, PARALLEL_VACUUM_KEY_DEAD_TUPLES, dead_tuples);
vacrelstats->dead_tuples = dead_tuples;
+ /* Allocate space for each worker's BufferUsage; no need to initialize */
+ buffer_usage = shm_toc_allocate(pcxt->toc,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_insert(pcxt->toc, PARALLEL_VACUUM_KEY_BUFFER_USAGE, buffer_usage);
+ lps->buffer_usage = buffer_usage;
+
/* Store query string for workers */
sharedquery = (char *) shm_toc_allocate(pcxt->toc, querylen + 1);
memcpy(sharedquery, debug_query_string, querylen + 1);
@@ -3317,6 +3349,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
Relation *indrels;
LVShared *lvshared;
LVDeadTuples *dead_tuples;
+ BufferUsage *buffer_usage;
int nindexes;
char *sharedquery;
IndexBulkDeleteResult **stats;
@@ -3369,9 +3402,16 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
if (lvshared->maintenance_work_mem_worker > 0)
maintenance_work_mem = lvshared->maintenance_work_mem_worker;
+ /* Prepare to track buffer usage during parallel execution */
+ InstrStartParallelQuery();
+
/* Process indexes to perform vacuum/cleanup */
parallel_vacuum_index(indrels, stats, lvshared, dead_tuples, nindexes);
+ /* Report buffer usage during parallel execution */
+ buffer_usage = shm_toc_lookup(toc, PARALLEL_VACUUM_KEY_BUFFER_USAGE, false);
+ InstrEndParallelQuery(&buffer_usage[ParallelWorkerNumber]);
+
vac_close_indexes(nindexes, indrels, RowExclusiveLock);
table_close(onerel, ShareUpdateExclusiveLock);
pfree(stats);
bufferusage_create_index_v2.patch (application/octet-stream, 4.5K) download
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index e66cd36dfa..a08a7df5cc 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -67,6 +67,7 @@
#include "access/xloginsert.h"
#include "catalog/index.h"
#include "commands/progress.h"
+#include "executor/instrument.h"
#include "miscadmin.h"
#include "pgstat.h"
#include "storage/smgr.h"
@@ -81,6 +82,7 @@
#define PARALLEL_KEY_TUPLESORT UINT64CONST(0xA000000000000002)
#define PARALLEL_KEY_TUPLESORT_SPOOL2 UINT64CONST(0xA000000000000003)
#define PARALLEL_KEY_QUERY_TEXT UINT64CONST(0xA000000000000004)
+#define PARALLEL_KEY_BUFFER_USAGE UINT64CONST(0xA000000000000005)
/*
* DISABLE_LEADER_PARTICIPATION disables the leader's participation in
@@ -203,6 +205,7 @@ typedef struct BTLeader
Sharedsort *sharedsort;
Sharedsort *sharedsort2;
Snapshot snapshot;
+ BufferUsage *bufferusage;
} BTLeader;
/*
@@ -1474,6 +1477,7 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
Sharedsort *sharedsort2;
BTSpool *btspool = buildstate->spool;
BTLeader *btleader = (BTLeader *) palloc0(sizeof(BTLeader));
+ BufferUsage *bufferusage;
bool leaderparticipates = true;
char *sharedquery;
int querylen;
@@ -1526,6 +1530,18 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
shm_toc_estimate_keys(&pcxt->estimator, 3);
}
+ /*
+ * Estimate space for BufferUsage -- PARALLEL_KEY_BUFFER_USAGE
+ *
+ * BufferUsage during executing maintenance command can be used by an
+ * extension that reports the buffer usage, such as pg_stat_statements.
+ * We have no way of knowing whether anyone's looking at pgBufferUsage,
+ * so do it unconditionally.
+ */
+ shm_toc_estimate_chunk(&pcxt->estimator,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_estimate_keys(&pcxt->estimator, 1);
+
/* Finally, estimate PARALLEL_KEY_QUERY_TEXT space */
querylen = strlen(debug_query_string);
shm_toc_estimate_chunk(&pcxt->estimator, querylen + 1);
@@ -1597,6 +1613,11 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
memcpy(sharedquery, debug_query_string, querylen + 1);
shm_toc_insert(pcxt->toc, PARALLEL_KEY_QUERY_TEXT, sharedquery);
+ /* Allocate space for each worker's BufferUsage; no need to initialize */
+ bufferusage = shm_toc_allocate(pcxt->toc,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_insert(pcxt->toc, PARALLEL_KEY_BUFFER_USAGE, bufferusage);
+
/* Launch workers, saving status for leader/caller */
LaunchParallelWorkers(pcxt);
btleader->pcxt = pcxt;
@@ -1607,6 +1628,7 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
btleader->sharedsort = sharedsort;
btleader->sharedsort2 = sharedsort2;
btleader->snapshot = snapshot;
+ btleader->bufferusage = bufferusage;
/* If no workers were successfully launched, back out (do serial build) */
if (pcxt->nworkers_launched == 0)
@@ -1635,8 +1657,18 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
static void
_bt_end_parallel(BTLeader *btleader)
{
+ int i;
+
/* Shutdown worker processes */
WaitForParallelWorkersToFinish(btleader->pcxt);
+
+ /*
+ * Next, accumulate buffer usage. (This must wait for the workers to
+ * finish, or we might get incomplete data.)
+ */
+ for (i = 0; i < btleader->pcxt->nworkers_launched; i++)
+ InstrAccumParallelQuery(&btleader->bufferusage[i]);
+
/* Free last reference to MVCC snapshot, if one was used */
if (IsMVCCSnapshot(btleader->snapshot))
UnregisterSnapshot(btleader->snapshot);
@@ -1767,6 +1799,7 @@ _bt_parallel_build_main(dsm_segment *seg, shm_toc *toc)
Relation indexRel;
LOCKMODE heapLockmode;
LOCKMODE indexLockmode;
+ BufferUsage *bufferusage;
int sortmem;
#ifdef BTREE_BUILD_STATS
@@ -1828,11 +1861,18 @@ _bt_parallel_build_main(dsm_segment *seg, shm_toc *toc)
tuplesort_attach_shared(sharedsort2, seg);
}
+ /* Prepare to track buffer usage during parallel execution */
+ InstrStartParallelQuery();
+
/* Perform sorting of spool, and possibly a spool2 */
sortmem = maintenance_work_mem / btshared->scantuplesortstates;
_bt_parallel_scan_and_sort(btspool, btspool2, btshared, sharedsort,
sharedsort2, sortmem, false);
+ /* Report buffer usage during parallel execution */
+ bufferusage = shm_toc_lookup(toc, PARALLEL_KEY_BUFFER_USAGE, false);
+ InstrEndParallelQuery(&bufferusage[ParallelWorkerNumber]);
+
#ifdef BTREE_BUILD_STATS
if (log_btree_build_stats)
{
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-03-31 06:50 Dilip Kumar <dilipbalaut@gmail.com>
parent: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
1 sibling, 1 reply; 43+ messages in thread
From: Dilip Kumar @ 2020-03-31 06:50 UTC (permalink / raw)
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Julien Rouhaud <rjuju123@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Tue, Mar 31, 2020 at 10:44 AM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Tue, 31 Mar 2020 at 12:58, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Mar 30, 2020 at 12:31 PM Masahiko Sawada
> > <masahiko.sawada@2ndquadrant.com> wrote:
> > >
> > > The patch for vacuum conflicts with recent changes in vacuum. So I've
> > > attached rebased one.
> > >
> >
> > + /*
> > + * Next, accumulate buffer usage. (This must wait for the workers to
> > + * finish, or we might get incomplete data.)
> > + */
> > + for (i = 0; i < nworkers; i++)
> > + InstrAccumParallelQuery(&lps->buffer_usage[i]);
> > +
> >
> > This should be done for launched workers aka
> > lps->pcxt->nworkers_launched. I think a similar problem exists in
> > create index related patch.
>
> You're right. Fixed in the new patches.
>
> On Mon, 30 Mar 2020 at 17:00, Julien Rouhaud <rjuju123@gmail.com> wrote:
> >
> > Just minor nitpicking:
> >
> > + int i;
> >
> > Assert(!IsParallelWorker());
> > Assert(ParallelVacuumIsActive(lps));
> > @@ -2166,6 +2172,13 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
> > /* Wait for all vacuum workers to finish */
> > WaitForParallelWorkersToFinish(lps->pcxt);
> >
> > + /*
> > + * Next, accumulate buffer usage. (This must wait for the workers to
> > + * finish, or we might get incomplete data.)
> > + */
> > + for (i = 0; i < nworkers; i++)
> > + InstrAccumParallelQuery(&lps->buffer_usage[i]);
> >
> > We now allow declaring a variable in those loops, so it may be better to avoid
> > declaring i outside the for scope?
>
> We can do that but I was not sure if it's good since other codes
> around there don't use that. So I'd like to leave it for committers.
> It's a trivial change.
I have reviewed the patch and the patch looks fine to me.
One minor comment
/+ /* Points to buffer usage are in DSM */
+ BufferUsage *buffer_usage;
+
/buffer usage are in DSM / buffer usage area in DSM
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-03-31 14:02 Dilip Kumar <dilipbalaut@gmail.com>
parent: Dilip Kumar <dilipbalaut@gmail.com>
0 siblings, 1 reply; 43+ messages in thread
From: Dilip Kumar @ 2020-03-31 14:02 UTC (permalink / raw)
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Julien Rouhaud <rjuju123@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Tue, Mar 31, 2020 at 12:20 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Tue, Mar 31, 2020 at 10:44 AM Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
> >
> > On Tue, 31 Mar 2020 at 12:58, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Mon, Mar 30, 2020 at 12:31 PM Masahiko Sawada
> > > <masahiko.sawada@2ndquadrant.com> wrote:
> > > >
> > > > The patch for vacuum conflicts with recent changes in vacuum. So I've
> > > > attached rebased one.
> > > >
> > >
> > > + /*
> > > + * Next, accumulate buffer usage. (This must wait for the workers to
> > > + * finish, or we might get incomplete data.)
> > > + */
> > > + for (i = 0; i < nworkers; i++)
> > > + InstrAccumParallelQuery(&lps->buffer_usage[i]);
> > > +
> > >
> > > This should be done for launched workers aka
> > > lps->pcxt->nworkers_launched. I think a similar problem exists in
> > > create index related patch.
> >
> > You're right. Fixed in the new patches.
> >
> > On Mon, 30 Mar 2020 at 17:00, Julien Rouhaud <rjuju123@gmail.com> wrote:
> > >
> > > Just minor nitpicking:
> > >
> > > + int i;
> > >
> > > Assert(!IsParallelWorker());
> > > Assert(ParallelVacuumIsActive(lps));
> > > @@ -2166,6 +2172,13 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
> > > /* Wait for all vacuum workers to finish */
> > > WaitForParallelWorkersToFinish(lps->pcxt);
> > >
> > > + /*
> > > + * Next, accumulate buffer usage. (This must wait for the workers to
> > > + * finish, or we might get incomplete data.)
> > > + */
> > > + for (i = 0; i < nworkers; i++)
> > > + InstrAccumParallelQuery(&lps->buffer_usage[i]);
> > >
> > > We now allow declaring a variable in those loops, so it may be better to avoid
> > > declaring i outside the for scope?
> >
> > We can do that but I was not sure if it's good since other codes
> > around there don't use that. So I'd like to leave it for committers.
> > It's a trivial change.
>
> I have reviewed the patch and the patch looks fine to me.
>
> One minor comment
> /+ /* Points to buffer usage are in DSM */
> + BufferUsage *buffer_usage;
> +
> /buffer usage are in DSM / buffer usage area in DSM
>
While testing I have found one issue. Basically, during a parallel
vacuum, it was showing more number of
shared_blk_hits+shared_blks_read. After, some investigation, I found
that during the cleanup phase nworkers are -1, and because of this we
didn't try to launch worker but "lps->pcxt->nworkers_launched" had the
old launched worker count and shared memory also had old buffer read
data which was never updated as we did not try to launch the worker.
diff --git a/src/backend/access/heap/vacuumlazy.c
b/src/backend/access/heap/vacuumlazy.c
index b97b678..5dfaf4d 100644
inline patch 1: src/backend/access/heap/vacuumlazy.c (+0/-0 lines) download inline-1.patch
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -2150,7 +2150,8 @@ lazy_parallel_vacuum_indexes(Relation *Irel,
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-01 02:46 Amit Kapila <amit.kapila16@gmail.com>
parent: Dilip Kumar <dilipbalaut@gmail.com>
0 siblings, 2 replies; 43+ messages in thread
From: Amit Kapila @ 2020-04-01 02:46 UTC (permalink / raw)
To: Dilip Kumar <dilipbalaut@gmail.com>; +Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; Julien Rouhaud <rjuju123@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Tue, Mar 31, 2020 at 7:32 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> While testing I have found one issue. Basically, during a parallel
> vacuum, it was showing more number of
> shared_blk_hits+shared_blks_read. After, some investigation, I found
> that during the cleanup phase nworkers are -1, and because of this we
> didn't try to launch worker but "lps->pcxt->nworkers_launched" had the
> old launched worker count and shared memory also had old buffer read
> data which was never updated as we did not try to launch the worker.
>
> diff --git a/src/backend/access/heap/vacuumlazy.c
> b/src/backend/access/heap/vacuumlazy.c
> index b97b678..5dfaf4d 100644
> --- a/src/backend/access/heap/vacuumlazy.c
> +++ b/src/backend/access/heap/vacuumlazy.c
> @@ -2150,7 +2150,8 @@ lazy_parallel_vacuum_indexes(Relation *Irel,
> IndexBulkDeleteResult **stats,
> * Next, accumulate buffer usage. (This must wait for the workers to
> * finish, or we might get incomplete data.)
> */
> - for (i = 0; i < lps->pcxt->nworkers_launched; i++)
> + nworkers = Min(nworkers, lps->pcxt->nworkers_launched);
> + for (i = 0; i < nworkers; i++)
> InstrAccumParallelQuery(&lps->buffer_usage[i]);
>
> It worked after the above fix.
>
Good catch. I think we should not even call
WaitForParallelWorkersToFinish for such a case. So, I guess the fix
could be,
if (workers > 0)
{
WaitForParallelWorkersToFinish();
for (i = 0; i < lps->pcxt->nworkers_launched; i++)
InstrAccumParallelQuery(&lps->buffer_usage[i]);
}
or something along those lines.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-01 02:50 Dilip Kumar <dilipbalaut@gmail.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
1 sibling, 0 replies; 43+ messages in thread
From: Dilip Kumar @ 2020-04-01 02:50 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; Julien Rouhaud <rjuju123@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Wed, Apr 1, 2020 at 8:16 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Mar 31, 2020 at 7:32 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > While testing I have found one issue. Basically, during a parallel
> > vacuum, it was showing more number of
> > shared_blk_hits+shared_blks_read. After, some investigation, I found
> > that during the cleanup phase nworkers are -1, and because of this we
> > didn't try to launch worker but "lps->pcxt->nworkers_launched" had the
> > old launched worker count and shared memory also had old buffer read
> > data which was never updated as we did not try to launch the worker.
> >
> > diff --git a/src/backend/access/heap/vacuumlazy.c
> > b/src/backend/access/heap/vacuumlazy.c
> > index b97b678..5dfaf4d 100644
> > --- a/src/backend/access/heap/vacuumlazy.c
> > +++ b/src/backend/access/heap/vacuumlazy.c
> > @@ -2150,7 +2150,8 @@ lazy_parallel_vacuum_indexes(Relation *Irel,
> > IndexBulkDeleteResult **stats,
> > * Next, accumulate buffer usage. (This must wait for the workers to
> > * finish, or we might get incomplete data.)
> > */
> > - for (i = 0; i < lps->pcxt->nworkers_launched; i++)
> > + nworkers = Min(nworkers, lps->pcxt->nworkers_launched);
> > + for (i = 0; i < nworkers; i++)
> > InstrAccumParallelQuery(&lps->buffer_usage[i]);
> >
> > It worked after the above fix.
> >
>
> Good catch. I think we should not even call
> WaitForParallelWorkersToFinish for such a case. So, I guess the fix
> could be,
>
> if (workers > 0)
> {
> WaitForParallelWorkersToFinish();
> for (i = 0; i < lps->pcxt->nworkers_launched; i++)
> InstrAccumParallelQuery(&lps->buffer_usage[i]);
> }
>
> or something along those lines.
Hmm, Right!
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-01 02:56 Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
1 sibling, 1 reply; 43+ messages in thread
From: Masahiko Sawada @ 2020-04-01 02:56 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Dilip Kumar <dilipbalaut@gmail.com>; Julien Rouhaud <rjuju123@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Wed, 1 Apr 2020 at 11:46, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Mar 31, 2020 at 7:32 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > While testing I have found one issue. Basically, during a parallel
> > vacuum, it was showing more number of
> > shared_blk_hits+shared_blks_read. After, some investigation, I found
> > that during the cleanup phase nworkers are -1, and because of this we
> > didn't try to launch worker but "lps->pcxt->nworkers_launched" had the
> > old launched worker count and shared memory also had old buffer read
> > data which was never updated as we did not try to launch the worker.
> >
> > diff --git a/src/backend/access/heap/vacuumlazy.c
> > b/src/backend/access/heap/vacuumlazy.c
> > index b97b678..5dfaf4d 100644
> > --- a/src/backend/access/heap/vacuumlazy.c
> > +++ b/src/backend/access/heap/vacuumlazy.c
> > @@ -2150,7 +2150,8 @@ lazy_parallel_vacuum_indexes(Relation *Irel,
> > IndexBulkDeleteResult **stats,
> > * Next, accumulate buffer usage. (This must wait for the workers to
> > * finish, or we might get incomplete data.)
> > */
> > - for (i = 0; i < lps->pcxt->nworkers_launched; i++)
> > + nworkers = Min(nworkers, lps->pcxt->nworkers_launched);
> > + for (i = 0; i < nworkers; i++)
> > InstrAccumParallelQuery(&lps->buffer_usage[i]);
> >
> > It worked after the above fix.
> >
>
> Good catch. I think we should not even call
> WaitForParallelWorkersToFinish for such a case. So, I guess the fix
> could be,
>
> if (workers > 0)
> {
> WaitForParallelWorkersToFinish();
> for (i = 0; i < lps->pcxt->nworkers_launched; i++)
> InstrAccumParallelQuery(&lps->buffer_usage[i]);
> }
>
Agreed. I've attached the updated patch.
Thank you for testing, Dilip!
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
bufferusage_vacuum_v4.patch (application/octet-stream, 4.7K) download
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 03c43efc32..3a188574aa 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -65,6 +65,7 @@
#include "commands/dbcommands.h"
#include "commands/progress.h"
#include "commands/vacuum.h"
+#include "executor/instrument.h"
#include "miscadmin.h"
#include "optimizer/paths.h"
#include "pgstat.h"
@@ -137,6 +138,7 @@
#define PARALLEL_VACUUM_KEY_SHARED 1
#define PARALLEL_VACUUM_KEY_DEAD_TUPLES 2
#define PARALLEL_VACUUM_KEY_QUERY_TEXT 3
+#define PARALLEL_VACUUM_KEY_BUFFER_USAGE 4
/*
* Macro to check if we are in a parallel vacuum. If true, we are in the
@@ -259,6 +261,9 @@ typedef struct LVParallelState
/* Shared information among parallel vacuum workers */
LVShared *lvshared;
+ /* Points to buffer usage area in DSM */
+ BufferUsage *buffer_usage;
+
/*
* The number of indexes that support parallel index bulk-deletion and
* parallel index cleanup respectively.
@@ -1991,6 +1996,7 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
int nindexes)
{
int nworkers;
+ int i;
Assert(!IsParallelWorker());
Assert(ParallelVacuumIsActive(lps));
@@ -2085,8 +2091,18 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
parallel_vacuum_index(Irel, stats, lps->lvshared,
vacrelstats->dead_tuples, nindexes);
- /* Wait for all vacuum workers to finish */
- WaitForParallelWorkersToFinish(lps->pcxt);
+ /*
+ * Next, accumulate buffer usage. (This must wait for the workers to
+ * finish, or we might get incomplete data.)
+ */
+ if (nworkers > 0)
+ {
+ /* Wait for all vacuum workers to finish */
+ WaitForParallelWorkersToFinish(lps->pcxt);
+
+ for (i = 0; i < lps->pcxt->nworkers_launched; i++)
+ InstrAccumParallelQuery(&lps->buffer_usage[i]);
+ }
/*
* Carry the shared balance value to heap scan and disable shared costing
@@ -3071,6 +3087,7 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats,
ParallelContext *pcxt;
LVShared *shared;
LVDeadTuples *dead_tuples;
+ BufferUsage *buffer_usage;
bool *can_parallel_vacuum;
long maxtuples;
char *sharedquery;
@@ -3154,6 +3171,18 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats,
shm_toc_estimate_chunk(&pcxt->estimator, est_deadtuples);
shm_toc_estimate_keys(&pcxt->estimator, 1);
+ /*
+ * Estimate space for BufferUsage -- PARALLEL_VACUUM_KEY_BUFFER_USAGE.
+ *
+ * BufferUsage during executing maintenance command can be used by an
+ * extension that reports the buffer usage, such as pg_stat_statements.
+ * We have no way of knowing whether anyone's looking at pgBufferUsage,
+ * so do it unconditionally.
+ */
+ shm_toc_estimate_chunk(&pcxt->estimator,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_estimate_keys(&pcxt->estimator, 1);
+
/* Finally, estimate PARALLEL_VACUUM_KEY_QUERY_TEXT space */
querylen = strlen(debug_query_string);
shm_toc_estimate_chunk(&pcxt->estimator, querylen + 1);
@@ -3188,6 +3217,12 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats,
shm_toc_insert(pcxt->toc, PARALLEL_VACUUM_KEY_DEAD_TUPLES, dead_tuples);
vacrelstats->dead_tuples = dead_tuples;
+ /* Allocate space for each worker's BufferUsage; no need to initialize */
+ buffer_usage = shm_toc_allocate(pcxt->toc,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_insert(pcxt->toc, PARALLEL_VACUUM_KEY_BUFFER_USAGE, buffer_usage);
+ lps->buffer_usage = buffer_usage;
+
/* Store query string for workers */
sharedquery = (char *) shm_toc_allocate(pcxt->toc, querylen + 1);
memcpy(sharedquery, debug_query_string, querylen + 1);
@@ -3317,6 +3352,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
Relation *indrels;
LVShared *lvshared;
LVDeadTuples *dead_tuples;
+ BufferUsage *buffer_usage;
int nindexes;
char *sharedquery;
IndexBulkDeleteResult **stats;
@@ -3369,9 +3405,16 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
if (lvshared->maintenance_work_mem_worker > 0)
maintenance_work_mem = lvshared->maintenance_work_mem_worker;
+ /* Prepare to track buffer usage during parallel execution */
+ InstrStartParallelQuery();
+
/* Process indexes to perform vacuum/cleanup */
parallel_vacuum_index(indrels, stats, lvshared, dead_tuples, nindexes);
+ /* Report buffer usage during parallel execution */
+ buffer_usage = shm_toc_lookup(toc, PARALLEL_VACUUM_KEY_BUFFER_USAGE, false);
+ InstrEndParallelQuery(&buffer_usage[ParallelWorkerNumber]);
+
vac_close_indexes(nindexes, indrels, RowExclusiveLock);
table_close(onerel, ShareUpdateExclusiveLock);
pfree(stats);
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-01 03:21 Dilip Kumar <dilipbalaut@gmail.com>
parent: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
0 siblings, 2 replies; 43+ messages in thread
From: Dilip Kumar @ 2020-04-01 03:21 UTC (permalink / raw)
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Julien Rouhaud <rjuju123@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Wed, Apr 1, 2020 at 8:26 AM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Wed, 1 Apr 2020 at 11:46, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Mar 31, 2020 at 7:32 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > While testing I have found one issue. Basically, during a parallel
> > > vacuum, it was showing more number of
> > > shared_blk_hits+shared_blks_read. After, some investigation, I found
> > > that during the cleanup phase nworkers are -1, and because of this we
> > > didn't try to launch worker but "lps->pcxt->nworkers_launched" had the
> > > old launched worker count and shared memory also had old buffer read
> > > data which was never updated as we did not try to launch the worker.
> > >
> > > diff --git a/src/backend/access/heap/vacuumlazy.c
> > > b/src/backend/access/heap/vacuumlazy.c
> > > index b97b678..5dfaf4d 100644
> > > --- a/src/backend/access/heap/vacuumlazy.c
> > > +++ b/src/backend/access/heap/vacuumlazy.c
> > > @@ -2150,7 +2150,8 @@ lazy_parallel_vacuum_indexes(Relation *Irel,
> > > IndexBulkDeleteResult **stats,
> > > * Next, accumulate buffer usage. (This must wait for the workers to
> > > * finish, or we might get incomplete data.)
> > > */
> > > - for (i = 0; i < lps->pcxt->nworkers_launched; i++)
> > > + nworkers = Min(nworkers, lps->pcxt->nworkers_launched);
> > > + for (i = 0; i < nworkers; i++)
> > > InstrAccumParallelQuery(&lps->buffer_usage[i]);
> > >
> > > It worked after the above fix.
> > >
> >
> > Good catch. I think we should not even call
> > WaitForParallelWorkersToFinish for such a case. So, I guess the fix
> > could be,
> >
> > if (workers > 0)
> > {
> > WaitForParallelWorkersToFinish();
> > for (i = 0; i < lps->pcxt->nworkers_launched; i++)
> > InstrAccumParallelQuery(&lps->buffer_usage[i]);
> > }
> >
>
> Agreed. I've attached the updated patch.
>
> Thank you for testing, Dilip!
Thanks! One hunk is failing on the latest head. And, I have rebased
the patch for my testing so posting the same. I have done some more
testing to test multi-pass vacuum.
postgres[114321]=# show maintenance_work_mem ;
maintenance_work_mem
----------------------
1MB
(1 row)
--Test case
select pg_stat_statements_reset();
drop table test;
CREATE TABLE test (a int, b int);
CREATE INDEX idx1 on test(a);
CREATE INDEX idx2 on test(b);
INSERT INTO test SELECT i, i FROM GENERATE_SERIES(1,2000000) as i;
DELETE FROM test where a%2=0;
VACUUM (PARALLEL n) test;
select query, 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 like 'VACUUM%';
query | total_time | shared_blks_hit |
shared_blks_read | total_read_blks | shared_blks_dirtied |
shared_blks_written
--------------------------+-------------+-----------------+------------------+-----------------+---------------------+---------------------
VACUUM (PARALLEL 0) test | 5964.282408 | 92447 |
6 | 92453 | 19789 | 0
query | total_time | shared_blks_hit |
shared_blks_read | total_read_blks | shared_blks_dirtied |
shared_blks_written
--------------------------+--------------------+-----------------+------------------+-----------------+---------------------+---------------------
VACUUM (PARALLEL 1) test | 3957.7658810000003 | 92447 |
6 | 92453 | 19789 |
0
(1 row)
So I am getting correct results with the multi-pass vacuum.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Attachments:
bufferusage_vacuum_v5.patch (application/octet-stream, 4.6K) download
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 04b1234..ec1a291 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -65,6 +65,7 @@
#include "commands/dbcommands.h"
#include "commands/progress.h"
#include "commands/vacuum.h"
+#include "executor/instrument.h"
#include "miscadmin.h"
#include "optimizer/paths.h"
#include "pgstat.h"
@@ -137,6 +138,7 @@
#define PARALLEL_VACUUM_KEY_SHARED 1
#define PARALLEL_VACUUM_KEY_DEAD_TUPLES 2
#define PARALLEL_VACUUM_KEY_QUERY_TEXT 3
+#define PARALLEL_VACUUM_KEY_BUFFER_USAGE 4
/*
* Macro to check if we are in a parallel vacuum. If true, we are in the
@@ -270,6 +272,9 @@ typedef struct LVParallelState
/* Shared information among parallel vacuum workers */
LVShared *lvshared;
+ /* Points to buffer usage area in DSM */
+ BufferUsage *buffer_usage;
+
/*
* The number of indexes that support parallel index bulk-deletion and
* parallel index cleanup respectively.
@@ -2043,6 +2048,7 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
int nindexes)
{
int nworkers;
+ int i;
Assert(!IsParallelWorker());
Assert(ParallelVacuumIsActive(lps));
@@ -2137,8 +2143,18 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
parallel_vacuum_index(Irel, stats, lps->lvshared,
vacrelstats->dead_tuples, nindexes, vacrelstats);
- /* Wait for all vacuum workers to finish */
- WaitForParallelWorkersToFinish(lps->pcxt);
+ /*
+ * Next, accumulate buffer usage. (This must wait for the workers to
+ * finish, or we might get incomplete data.)
+ */
+ if (nworkers > 0)
+ {
+ /* Wait for all vacuum workers to finish */
+ WaitForParallelWorkersToFinish(lps->pcxt);
+
+ for (i = 0; i < lps->pcxt->nworkers_launched; i++)
+ InstrAccumParallelQuery(&lps->buffer_usage[i]);
+ }
/*
* Carry the shared balance value to heap scan and disable shared costing
@@ -3153,6 +3169,7 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats,
ParallelContext *pcxt;
LVShared *shared;
LVDeadTuples *dead_tuples;
+ BufferUsage *buffer_usage;
bool *can_parallel_vacuum;
long maxtuples;
char *sharedquery;
@@ -3236,6 +3253,18 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats,
shm_toc_estimate_chunk(&pcxt->estimator, est_deadtuples);
shm_toc_estimate_keys(&pcxt->estimator, 1);
+ /*
+ * Estimate space for BufferUsage -- PARALLEL_VACUUM_KEY_BUFFER_USAGE.
+ *
+ * BufferUsage during executing maintenance command can be used by an
+ * extension that reports the buffer usage, such as pg_stat_statements.
+ * We have no way of knowing whether anyone's looking at pgBufferUsage,
+ * so do it unconditionally.
+ */
+ shm_toc_estimate_chunk(&pcxt->estimator,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_estimate_keys(&pcxt->estimator, 1);
+
/* Finally, estimate PARALLEL_VACUUM_KEY_QUERY_TEXT space */
querylen = strlen(debug_query_string);
shm_toc_estimate_chunk(&pcxt->estimator, querylen + 1);
@@ -3270,6 +3299,12 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats,
shm_toc_insert(pcxt->toc, PARALLEL_VACUUM_KEY_DEAD_TUPLES, dead_tuples);
vacrelstats->dead_tuples = dead_tuples;
+ /* Allocate space for each worker's BufferUsage; no need to initialize */
+ buffer_usage = shm_toc_allocate(pcxt->toc,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_insert(pcxt->toc, PARALLEL_VACUUM_KEY_BUFFER_USAGE, buffer_usage);
+ lps->buffer_usage = buffer_usage;
+
/* Store query string for workers */
sharedquery = (char *) shm_toc_allocate(pcxt->toc, querylen + 1);
memcpy(sharedquery, debug_query_string, querylen + 1);
@@ -3399,6 +3434,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
Relation *indrels;
LVShared *lvshared;
LVDeadTuples *dead_tuples;
+ BufferUsage *buffer_usage;
int nindexes;
char *sharedquery;
IndexBulkDeleteResult **stats;
@@ -3468,10 +3504,17 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
errcallback.previous = error_context_stack;
error_context_stack = &errcallback;
+ /* Prepare to track buffer usage during parallel execution */
+ InstrStartParallelQuery();
+
/* Process indexes to perform vacuum/cleanup */
parallel_vacuum_index(indrels, stats, lvshared, dead_tuples, nindexes,
&vacrelstats);
+ /* Report buffer usage during parallel execution */
+ buffer_usage = shm_toc_lookup(toc, PARALLEL_VACUUM_KEY_BUFFER_USAGE, false);
+ InstrEndParallelQuery(&buffer_usage[ParallelWorkerNumber]);
+
/* Pop the error context stack */
error_context_stack = errcallback.previous;
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-01 06:31 Amit Kapila <amit.kapila16@gmail.com>
parent: Dilip Kumar <dilipbalaut@gmail.com>
1 sibling, 1 reply; 43+ messages in thread
From: Amit Kapila @ 2020-04-01 06:31 UTC (permalink / raw)
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; Dilip Kumar <dilipbalaut@gmail.com>; +Cc: Julien Rouhaud <rjuju123@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Wed, Apr 1, 2020 at 8:51 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> > Agreed. I've attached the updated patch.
> >
> > Thank you for testing, Dilip!
>
> Thanks! One hunk is failing on the latest head. And, I have rebased
> the patch for my testing so posting the same. I have done some more
> testing to test multi-pass vacuum.
>
The patch looks good to me. I have done a few minor modifications (a)
moved the declaration of variable closer to where it is used, (b)
changed a comment, (c) ran pgindent. I have also done some additional
testing with more number of indexes and found that vacuum and parallel
vacuum used the same number of total_read_blks and that is what is
expected here.
Let me know what you think of the attached?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
bufferusage_vacuum_v6.patch (application/octet-stream, 5.1K) download
From c2edea15f18e56ee6f26e3498d691ce22cc27676 Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Wed, 1 Apr 2020 11:50:57 +0530
Subject: [PATCH v6] Allow parallel vacuum to accumulate buffer usage.
Commit 40d964ec99 allowed vacuum command to process indexes in parallel but
forgot to accumulate the buffer usage stats of parallel workers. This
allows leader backend to accumulate buffer usage stats of all the parallel
workers.
Reported-by: Julien Rouhaud
Author: Sawada Masahiko
Reviewed-by: Dilip Kumar, Amit Kapila and Julien Rouhaud
Discussion: https://postgr.es/m/20200328151721.GB12854@nol
---
src/backend/access/heap/vacuumlazy.c | 47 ++++++++++++++++++++++++++++++++++--
1 file changed, 45 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 04b1234..9f9596c 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -65,6 +65,7 @@
#include "commands/dbcommands.h"
#include "commands/progress.h"
#include "commands/vacuum.h"
+#include "executor/instrument.h"
#include "miscadmin.h"
#include "optimizer/paths.h"
#include "pgstat.h"
@@ -137,6 +138,7 @@
#define PARALLEL_VACUUM_KEY_SHARED 1
#define PARALLEL_VACUUM_KEY_DEAD_TUPLES 2
#define PARALLEL_VACUUM_KEY_QUERY_TEXT 3
+#define PARALLEL_VACUUM_KEY_BUFFER_USAGE 4
/*
* Macro to check if we are in a parallel vacuum. If true, we are in the
@@ -270,6 +272,9 @@ typedef struct LVParallelState
/* Shared information among parallel vacuum workers */
LVShared *lvshared;
+ /* Points to buffer usage area in DSM */
+ BufferUsage *buffer_usage;
+
/*
* The number of indexes that support parallel index bulk-deletion and
* parallel index cleanup respectively.
@@ -2137,8 +2142,20 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
parallel_vacuum_index(Irel, stats, lps->lvshared,
vacrelstats->dead_tuples, nindexes, vacrelstats);
- /* Wait for all vacuum workers to finish */
- WaitForParallelWorkersToFinish(lps->pcxt);
+ /*
+ * Next, accumulate buffer usage. (This must wait for the workers to
+ * finish, or we might get incomplete data.)
+ */
+ if (nworkers > 0)
+ {
+ int i;
+
+ /* Wait for all vacuum workers to finish */
+ WaitForParallelWorkersToFinish(lps->pcxt);
+
+ for (i = 0; i < lps->pcxt->nworkers_launched; i++)
+ InstrAccumParallelQuery(&lps->buffer_usage[i]);
+ }
/*
* Carry the shared balance value to heap scan and disable shared costing
@@ -3153,6 +3170,7 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats,
ParallelContext *pcxt;
LVShared *shared;
LVDeadTuples *dead_tuples;
+ BufferUsage *buffer_usage;
bool *can_parallel_vacuum;
long maxtuples;
char *sharedquery;
@@ -3236,6 +3254,17 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats,
shm_toc_estimate_chunk(&pcxt->estimator, est_deadtuples);
shm_toc_estimate_keys(&pcxt->estimator, 1);
+ /*
+ * Estimate space for BufferUsage -- PARALLEL_VACUUM_KEY_BUFFER_USAGE.
+ *
+ * If there are no extensions loaded that care, we could skip this. We
+ * have no way of knowing whether anyone's looking at pgBufferUsage, so do
+ * it unconditionally.
+ */
+ shm_toc_estimate_chunk(&pcxt->estimator,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_estimate_keys(&pcxt->estimator, 1);
+
/* Finally, estimate PARALLEL_VACUUM_KEY_QUERY_TEXT space */
querylen = strlen(debug_query_string);
shm_toc_estimate_chunk(&pcxt->estimator, querylen + 1);
@@ -3270,6 +3299,12 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats,
shm_toc_insert(pcxt->toc, PARALLEL_VACUUM_KEY_DEAD_TUPLES, dead_tuples);
vacrelstats->dead_tuples = dead_tuples;
+ /* Allocate space for each worker's BufferUsage; no need to initialize */
+ buffer_usage = shm_toc_allocate(pcxt->toc,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_insert(pcxt->toc, PARALLEL_VACUUM_KEY_BUFFER_USAGE, buffer_usage);
+ lps->buffer_usage = buffer_usage;
+
/* Store query string for workers */
sharedquery = (char *) shm_toc_allocate(pcxt->toc, querylen + 1);
memcpy(sharedquery, debug_query_string, querylen + 1);
@@ -3399,6 +3434,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
Relation *indrels;
LVShared *lvshared;
LVDeadTuples *dead_tuples;
+ BufferUsage *buffer_usage;
int nindexes;
char *sharedquery;
IndexBulkDeleteResult **stats;
@@ -3468,10 +3504,17 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
errcallback.previous = error_context_stack;
error_context_stack = &errcallback;
+ /* Prepare to track buffer usage during parallel execution */
+ InstrStartParallelQuery();
+
/* Process indexes to perform vacuum/cleanup */
parallel_vacuum_index(indrels, stats, lvshared, dead_tuples, nindexes,
&vacrelstats);
+ /* Report buffer usage during parallel execution */
+ buffer_usage = shm_toc_lookup(toc, PARALLEL_VACUUM_KEY_BUFFER_USAGE, false);
+ InstrEndParallelQuery(&buffer_usage[ParallelWorkerNumber]);
+
/* Pop the error context stack */
error_context_stack = errcallback.previous;
--
1.8.3.1
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-01 07:11 Dilip Kumar <dilipbalaut@gmail.com>
parent: Dilip Kumar <dilipbalaut@gmail.com>
1 sibling, 1 reply; 43+ messages in thread
From: Dilip Kumar @ 2020-04-01 07:11 UTC (permalink / raw)
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Julien Rouhaud <rjuju123@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Wed, Apr 1, 2020 at 8:51 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Wed, Apr 1, 2020 at 8:26 AM Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
> >
> > On Wed, 1 Apr 2020 at 11:46, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Tue, Mar 31, 2020 at 7:32 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > >
> > > > While testing I have found one issue. Basically, during a parallel
> > > > vacuum, it was showing more number of
> > > > shared_blk_hits+shared_blks_read. After, some investigation, I found
> > > > that during the cleanup phase nworkers are -1, and because of this we
> > > > didn't try to launch worker but "lps->pcxt->nworkers_launched" had the
> > > > old launched worker count and shared memory also had old buffer read
> > > > data which was never updated as we did not try to launch the worker.
> > > >
> > > > diff --git a/src/backend/access/heap/vacuumlazy.c
> > > > b/src/backend/access/heap/vacuumlazy.c
> > > > index b97b678..5dfaf4d 100644
> > > > --- a/src/backend/access/heap/vacuumlazy.c
> > > > +++ b/src/backend/access/heap/vacuumlazy.c
> > > > @@ -2150,7 +2150,8 @@ lazy_parallel_vacuum_indexes(Relation *Irel,
> > > > IndexBulkDeleteResult **stats,
> > > > * Next, accumulate buffer usage. (This must wait for the workers to
> > > > * finish, or we might get incomplete data.)
> > > > */
> > > > - for (i = 0; i < lps->pcxt->nworkers_launched; i++)
> > > > + nworkers = Min(nworkers, lps->pcxt->nworkers_launched);
> > > > + for (i = 0; i < nworkers; i++)
> > > > InstrAccumParallelQuery(&lps->buffer_usage[i]);
> > > >
> > > > It worked after the above fix.
> > > >
> > >
> > > Good catch. I think we should not even call
> > > WaitForParallelWorkersToFinish for such a case. So, I guess the fix
> > > could be,
> > >
> > > if (workers > 0)
> > > {
> > > WaitForParallelWorkersToFinish();
> > > for (i = 0; i < lps->pcxt->nworkers_launched; i++)
> > > InstrAccumParallelQuery(&lps->buffer_usage[i]);
> > > }
> > >
> >
> > Agreed. I've attached the updated patch.
> >
> > Thank you for testing, Dilip!
>
> Thanks! One hunk is failing on the latest head. And, I have rebased
> the patch for my testing so posting the same. I have done some more
> testing to test multi-pass vacuum.
>
> postgres[114321]=# show maintenance_work_mem ;
> maintenance_work_mem
> ----------------------
> 1MB
> (1 row)
>
> --Test case
> select pg_stat_statements_reset();
> drop table test;
> CREATE TABLE test (a int, b int);
> CREATE INDEX idx1 on test(a);
> CREATE INDEX idx2 on test(b);
> INSERT INTO test SELECT i, i FROM GENERATE_SERIES(1,2000000) as i;
> DELETE FROM test where a%2=0;
> VACUUM (PARALLEL n) test;
> select query, 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 like 'VACUUM%';
>
> query | total_time | shared_blks_hit |
> shared_blks_read | total_read_blks | shared_blks_dirtied |
> shared_blks_written
> --------------------------+-------------+-----------------+------------------+-----------------+---------------------+---------------------
> VACUUM (PARALLEL 0) test | 5964.282408 | 92447 |
> 6 | 92453 | 19789 | 0
>
>
> query | total_time | shared_blks_hit |
> shared_blks_read | total_read_blks | shared_blks_dirtied |
> shared_blks_written
> --------------------------+--------------------+-----------------+------------------+-----------------+---------------------+---------------------
> VACUUM (PARALLEL 1) test | 3957.7658810000003 | 92447 |
> 6 | 92453 | 19789 |
> 0
> (1 row)
>
> So I am getting correct results with the multi-pass vacuum.
I have done some testing for the parallel "create index".
postgres[99536]=# show maintenance_work_mem ;
maintenance_work_mem
----------------------
1MB
(1 row)
CREATE TABLE test (a int, b int);
INSERT INTO test SELECT i, i FROM GENERATE_SERIES(1,2000000) as i;
CREATE INDEX idx1 on test(a);
select query, 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 like 'CREATE INDEX%';
SET max_parallel_maintenance_workers TO 0;
query | total_time | shared_blks_hit |
shared_blks_read | total_read_blks | shared_blks_dirtied |
shared_blks_written
------------------------------+--------------------+-----------------+------------------+-----------------+---------------------+---------------------
CREATE INDEX idx1 on test(a) | 1947.4959979999999 | 8947 |
11 | 8958 | 5 |
0
SET max_parallel_maintenance_workers TO 2;
query | total_time | shared_blks_hit |
shared_blks_read | total_read_blks | shared_blks_dirtied |
shared_blks_written
------------------------------+--------------------+-----------------+------------------+-----------------+---------------------+---------------------
CREATE INDEX idx1 on test(a) | 1942.1426040000001 | 8960 |
14 | 8974 | 5 |
0
(1 row)
I have noticed that the total_read_blks, with the parallel, create
index is more compared to non-parallel one. I have created a fresh
database before each run. I am not much aware of the internal code of
parallel create an index so I am not sure whether it is expected to
read extra blocks with the parallel create an index. I guess maybe
because multiple workers are inserting int the btree they might need
to visit some btree nodes multiple times while traversing the tree
down. But, it's better if someone who have more idea with this code
can confirm this.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-01 12:25 Dilip Kumar <dilipbalaut@gmail.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 0 replies; 43+ messages in thread
From: Dilip Kumar @ 2020-04-01 12:25 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; Julien Rouhaud <rjuju123@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Wed, Apr 1, 2020 at 12:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Apr 1, 2020 at 8:51 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > > Agreed. I've attached the updated patch.
> > >
> > > Thank you for testing, Dilip!
> >
> > Thanks! One hunk is failing on the latest head. And, I have rebased
> > the patch for my testing so posting the same. I have done some more
> > testing to test multi-pass vacuum.
> >
>
> The patch looks good to me. I have done a few minor modifications (a)
> moved the declaration of variable closer to where it is used, (b)
> changed a comment, (c) ran pgindent. I have also done some additional
> testing with more number of indexes and found that vacuum and parallel
> vacuum used the same number of total_read_blks and that is what is
> expected here.
>
> Let me know what you think of the attached?
The patch looks fine to me.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-02 02:52 Amit Kapila <amit.kapila16@gmail.com>
parent: Dilip Kumar <dilipbalaut@gmail.com>
0 siblings, 1 reply; 43+ messages in thread
From: Amit Kapila @ 2020-04-02 02:52 UTC (permalink / raw)
To: Dilip Kumar <dilipbalaut@gmail.com>; Peter Geoghegan <pg@bowt.ie>; +Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; Julien Rouhaud <rjuju123@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
Adding Peter G.
On Wed, Apr 1, 2020 at 12:41 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> I have done some testing for the parallel "create index".
>
> postgres[99536]=# show maintenance_work_mem ;
> maintenance_work_mem
> ----------------------
> 1MB
> (1 row)
>
> CREATE TABLE test (a int, b int);
> INSERT INTO test SELECT i, i FROM GENERATE_SERIES(1,2000000) as i;
> CREATE INDEX idx1 on test(a);
> select query, 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 like 'CREATE INDEX%';
>
>
> SET max_parallel_maintenance_workers TO 0;
> query | total_time | shared_blks_hit |
> shared_blks_read | total_read_blks | shared_blks_dirtied |
> shared_blks_written
> ------------------------------+--------------------+-----------------+------------------+-----------------+---------------------+---------------------
> CREATE INDEX idx1 on test(a) | 1947.4959979999999 | 8947 |
> 11 | 8958 | 5 |
> 0
>
> SET max_parallel_maintenance_workers TO 2;
>
> query | total_time | shared_blks_hit |
> shared_blks_read | total_read_blks | shared_blks_dirtied |
> shared_blks_written
> ------------------------------+--------------------+-----------------+------------------+-----------------+---------------------+---------------------
> CREATE INDEX idx1 on test(a) | 1942.1426040000001 | 8960 |
> 14 | 8974 | 5 |
> 0
> (1 row)
>
> I have noticed that the total_read_blks, with the parallel, create
> index is more compared to non-parallel one. I have created a fresh
> database before each run. I am not much aware of the internal code of
> parallel create an index so I am not sure whether it is expected to
> read extra blocks with the parallel create an index. I guess maybe
> because multiple workers are inserting int the btree they might need
> to visit some btree nodes multiple times while traversing the tree
> down. But, it's better if someone who have more idea with this code
> can confirm this.
>
Peter, Is this behavior expected?
Let me summarize the situation so that it would be easier for Peter to
comment. Julien has noticed that parallel vacuum and parallel create
index doesn't seem to report correct values for buffer usage stats.
Sawada-San wrote a patch to fix the problem for both the cases. We
expect that 'total_read_blks' as reported in pg_stat_statements should
give the same value for parallel and non-parallel operations. We see
that is true for parallel vacuum and previously we have the same
observation for the parallel query. Now, for parallel create index
this doesn't seem to be true as test results by Dilip show that. We
have two possibilities here (a) there is some bug in Sawada-San's
patch or (b) this is expected behavior for parallel create index.
What do you think?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-02 03:04 Peter Geoghegan <pg@bowt.ie>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 43+ messages in thread
From: Peter Geoghegan @ 2020-04-02 03:04 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Dilip Kumar <dilipbalaut@gmail.com>; Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; Julien Rouhaud <rjuju123@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Wed, Apr 1, 2020 at 7:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> Peter, Is this behavior expected?
>
> Let me summarize the situation so that it would be easier for Peter to
> comment. Julien has noticed that parallel vacuum and parallel create
> index doesn't seem to report correct values for buffer usage stats.
> Sawada-San wrote a patch to fix the problem for both the cases. We
> expect that 'total_read_blks' as reported in pg_stat_statements should
> give the same value for parallel and non-parallel operations. We see
> that is true for parallel vacuum and previously we have the same
> observation for the parallel query. Now, for parallel create index
> this doesn't seem to be true as test results by Dilip show that. We
> have two possibilities here (a) there is some bug in Sawada-San's
> patch or (b) this is expected behavior for parallel create index.
> What do you think?
nbtree CREATE INDEX doesn't even go through the buffer manager. The
difference that Dilip showed is probably due to extra catalog accesses
in the two parallel workers -- pg_amproc lookups, and the like. Those
are rather small differences, overall.
Can Dilip demonstrate the the "extra" buffer accesses are
proportionate to the number of workers launched in some constant,
predictable way?
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-02 03:43 Dilip Kumar <dilipbalaut@gmail.com>
parent: Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 43+ messages in thread
From: Dilip Kumar @ 2020-04-02 03:43 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; Julien Rouhaud <rjuju123@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Thu, Apr 2, 2020 at 8:34 AM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Wed, Apr 1, 2020 at 7:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > Peter, Is this behavior expected?
> >
> > Let me summarize the situation so that it would be easier for Peter to
> > comment. Julien has noticed that parallel vacuum and parallel create
> > index doesn't seem to report correct values for buffer usage stats.
> > Sawada-San wrote a patch to fix the problem for both the cases. We
> > expect that 'total_read_blks' as reported in pg_stat_statements should
> > give the same value for parallel and non-parallel operations. We see
> > that is true for parallel vacuum and previously we have the same
> > observation for the parallel query. Now, for parallel create index
> > this doesn't seem to be true as test results by Dilip show that. We
> > have two possibilities here (a) there is some bug in Sawada-San's
> > patch or (b) this is expected behavior for parallel create index.
> > What do you think?
>
> nbtree CREATE INDEX doesn't even go through the buffer manager.
Thanks for clarifying. So IIUC, it will not go through the buffer
manager for the index pages, but for the heap pages, it will still go
through the buffer manager.
> The
> difference that Dilip showed is probably due to extra catalog accesses
> in the two parallel workers -- pg_amproc lookups, and the like. Those
> are rather small differences, overall.
> Can Dilip demonstrate the the "extra" buffer accesses are
> proportionate to the number of workers launched in some constant,
> predictable way?
Okay, I will test this.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-02 04:43 Dilip Kumar <dilipbalaut@gmail.com>
parent: Dilip Kumar <dilipbalaut@gmail.com>
0 siblings, 0 replies; 43+ messages in thread
From: Dilip Kumar @ 2020-04-02 04:43 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; Julien Rouhaud <rjuju123@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Thu, Apr 2, 2020 at 9:13 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, Apr 2, 2020 at 8:34 AM Peter Geoghegan <pg@bowt.ie> wrote:
> >
> > On Wed, Apr 1, 2020 at 7:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > Peter, Is this behavior expected?
> > >
> > > Let me summarize the situation so that it would be easier for Peter to
> > > comment. Julien has noticed that parallel vacuum and parallel create
> > > index doesn't seem to report correct values for buffer usage stats.
> > > Sawada-San wrote a patch to fix the problem for both the cases. We
> > > expect that 'total_read_blks' as reported in pg_stat_statements should
> > > give the same value for parallel and non-parallel operations. We see
> > > that is true for parallel vacuum and previously we have the same
> > > observation for the parallel query. Now, for parallel create index
> > > this doesn't seem to be true as test results by Dilip show that. We
> > > have two possibilities here (a) there is some bug in Sawada-San's
> > > patch or (b) this is expected behavior for parallel create index.
> > > What do you think?
> >
> > nbtree CREATE INDEX doesn't even go through the buffer manager.
>
> Thanks for clarifying. So IIUC, it will not go through the buffer
> manager for the index pages, but for the heap pages, it will still go
> through the buffer manager.
>
> > The
> > difference that Dilip showed is probably due to extra catalog accesses
> > in the two parallel workers -- pg_amproc lookups, and the like. Those
> > are rather small differences, overall.
>
> > Can Dilip demonstrate the the "extra" buffer accesses are
> > proportionate to the number of workers launched in some constant,
> > predictable way?
>
> Okay, I will test this.
0-worker
query | total_time | shared_blks_hit |
shared_blks_read | total_read_blks | shared_blks_dirtied |
shared_blks_written
------------------------------+-------------+-----------------+------------------+-----------------+---------------------+---------------------
CREATE INDEX idx1 on test(a) | 1228.895057 | 8947 |
11 | 8971 | 5 |
0
1-worker
query | total_time | shared_blks_hit |
shared_blks_read | total_read_blks | shared_blks_dirtied |
shared_blks_written
------------------------------+-------------+-----------------+------------------+-----------------+---------------------+---------------------
CREATE INDEX idx1 on test(a) | 1006.157231 | 8962 |
12 | 8974 | 5 |
0
2-workers
query | total_time | shared_blks_hit |
shared_blks_read | total_read_blks | shared_blks_dirtied |
shared_blks_written
------------------------------+------------+-----------------+------------------+-----------------+---------------------+---------------------
CREATE INDEX idx1 on test(a) | 949.44663 | 8965 |
12 | 8977 | 5 | 0
3-workers
query | total_time | shared_blks_hit |
shared_blks_read | total_read_blks | shared_blks_dirtied |
shared_blks_written
------------------------------+-------------+-----------------+------------------+-----------------+---------------------+---------------------
CREATE INDEX idx1 on test(a) | 1037.297196 | 8968 |
12 | 8980 | 5 |
0
4-workers
query | total_time | shared_blks_hit |
shared_blks_read | total_read_blks | shared_blks_dirtied |
shared_blks_written
------------------------------+------------+-----------------+------------------+-----------------+---------------------+---------------------
CREATE INDEX idx1 on test(a) | 889.332782 | 8971 |
12 | 8983 | 6 | 0
You are right, it is increasing with some constant factor.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-06 05:48 Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
parent: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
1 sibling, 1 reply; 43+ messages in thread
From: Masahiko Sawada @ 2020-04-06 05:48 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Julien Rouhaud <rjuju123@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Tue, 31 Mar 2020 at 14:13, Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Tue, 31 Mar 2020 at 12:58, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Mar 30, 2020 at 12:31 PM Masahiko Sawada
> > <masahiko.sawada@2ndquadrant.com> wrote:
> > >
> > > The patch for vacuum conflicts with recent changes in vacuum. So I've
> > > attached rebased one.
> > >
> >
> > + /*
> > + * Next, accumulate buffer usage. (This must wait for the workers to
> > + * finish, or we might get incomplete data.)
> > + */
> > + for (i = 0; i < nworkers; i++)
> > + InstrAccumParallelQuery(&lps->buffer_usage[i]);
> > +
> >
> > This should be done for launched workers aka
> > lps->pcxt->nworkers_launched. I think a similar problem exists in
> > create index related patch.
>
> You're right. Fixed in the new patches.
>
> On Mon, 30 Mar 2020 at 17:00, Julien Rouhaud <rjuju123@gmail.com> wrote:
> >
> > Just minor nitpicking:
> >
> > + int i;
> >
> > Assert(!IsParallelWorker());
> > Assert(ParallelVacuumIsActive(lps));
> > @@ -2166,6 +2172,13 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
> > /* Wait for all vacuum workers to finish */
> > WaitForParallelWorkersToFinish(lps->pcxt);
> >
> > + /*
> > + * Next, accumulate buffer usage. (This must wait for the workers to
> > + * finish, or we might get incomplete data.)
> > + */
> > + for (i = 0; i < nworkers; i++)
> > + InstrAccumParallelQuery(&lps->buffer_usage[i]);
> >
> > We now allow declaring a variable in those loops, so it may be better to avoid
> > declaring i outside the for scope?
>
> We can do that but I was not sure if it's good since other codes
> around there don't use that. So I'd like to leave it for committers.
> It's a trivial change.
>
I've updated the buffer usage patch for parallel index creation as the
previous patch conflicts with commit df3b181499b40.
This comment in commit df3b181499b40 seems the comment which had been
replaced by Amit with a better sentence when introducing buffer usage
to parallel vacuum.
+ /*
+ * Estimate space for WalUsage -- PARALLEL_KEY_WAL_USAGE
+ *
+ * WalUsage during execution of maintenance command can be used by an
+ * extension that reports the WAL usage, such as pg_stat_statements. We
+ * have no way of knowing whether anyone's looking at pgWalUsage, so do it
+ * unconditionally.
+ */
Would the following sentence in lazyvacuum.c be also better for
parallel create index?
* If there are no extensions loaded that care, we could skip this. We
* have no way of knowing whether anyone's looking at pgBufferUsage or
* pgWalUsage, so do it unconditionally.
The attached patch changes to the above comment and removed the code
that is used to un-support only buffer usage accumulation.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
bufferusage_create_index_v3.patch (application/octet-stream, 4.7K) download
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index 29a6f5ade6..823d45f074 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -71,6 +71,7 @@
#define PARALLEL_KEY_TUPLESORT_SPOOL2 UINT64CONST(0xA000000000000003)
#define PARALLEL_KEY_QUERY_TEXT UINT64CONST(0xA000000000000004)
#define PARALLEL_KEY_WAL_USAGE UINT64CONST(0xA000000000000005)
+#define PARALLEL_KEY_BUFFER_USAGE UINT64CONST(0xA000000000000006)
/*
* DISABLE_LEADER_PARTICIPATION disables the leader's participation in
@@ -194,6 +195,7 @@ typedef struct BTLeader
Sharedsort *sharedsort2;
Snapshot snapshot;
WalUsage *walusage;
+ BufferUsage *bufferusage;
} BTLeader;
/*
@@ -1457,6 +1459,7 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
BTSpool *btspool = buildstate->spool;
BTLeader *btleader = (BTLeader *) palloc0(sizeof(BTLeader));
WalUsage *walusage;
+ BufferUsage *bufferusage;
bool leaderparticipates = true;
char *sharedquery;
int querylen;
@@ -1510,16 +1513,19 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
}
/*
- * Estimate space for WalUsage -- PARALLEL_KEY_WAL_USAGE
+ * Estimate space for WalUsage and BufferUsage -- PARALLEL_KEY_WAL_USAGE
+ * and PARALLEL_KEY_BUFFER_USAGE.
*
- * WalUsage during execution of maintenance command can be used by an
- * extension that reports the WAL usage, such as pg_stat_statements. We
- * have no way of knowing whether anyone's looking at pgWalUsage, so do it
- * unconditionally.
+ * If there are no extensions loaded that care, we could skip this. We
+ * have no way of knowing whether anyone's looking at pgWalUsage or
+ * pgBufferUsage, so do it unconditionally.
*/
shm_toc_estimate_chunk(&pcxt->estimator,
mul_size(sizeof(WalUsage), pcxt->nworkers));
shm_toc_estimate_keys(&pcxt->estimator, 1);
+ shm_toc_estimate_chunk(&pcxt->estimator,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_estimate_keys(&pcxt->estimator, 1);
/* Finally, estimate PARALLEL_KEY_QUERY_TEXT space */
querylen = strlen(debug_query_string);
@@ -1592,10 +1598,16 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
memcpy(sharedquery, debug_query_string, querylen + 1);
shm_toc_insert(pcxt->toc, PARALLEL_KEY_QUERY_TEXT, sharedquery);
- /* Allocate space for each worker's WalUsage; no need to initialize */
+ /*
+ * Allocate space for each worker's WalUsage and BufferUsage; no need to
+ * initialize.
+ */
walusage = shm_toc_allocate(pcxt->toc,
mul_size(sizeof(WalUsage), pcxt->nworkers));
shm_toc_insert(pcxt->toc, PARALLEL_KEY_WAL_USAGE, walusage);
+ bufferusage = shm_toc_allocate(pcxt->toc,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_insert(pcxt->toc, PARALLEL_KEY_BUFFER_USAGE, bufferusage);
/* Launch workers, saving status for leader/caller */
LaunchParallelWorkers(pcxt);
@@ -1608,6 +1620,7 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
btleader->sharedsort2 = sharedsort2;
btleader->snapshot = snapshot;
btleader->walusage = walusage;
+ btleader->bufferusage = bufferusage;
/* If no workers were successfully launched, back out (do serial build) */
if (pcxt->nworkers_launched == 0)
@@ -1646,7 +1659,7 @@ _bt_end_parallel(BTLeader *btleader)
* or we might get incomplete data.)
*/
for (i = 0; i < btleader->pcxt->nworkers_launched; i++)
- InstrAccumParallelQuery(NULL, &btleader->walusage[i]);
+ InstrAccumParallelQuery(&btleader->bufferusage[i], &btleader->walusage[i]);
/* Free last reference to MVCC snapshot, if one was used */
if (IsMVCCSnapshot(btleader->snapshot))
diff --git a/src/backend/executor/instrument.c b/src/backend/executor/instrument.c
index 74ee4808e3..3b9c6aebb9 100644
--- a/src/backend/executor/instrument.c
+++ b/src/backend/executor/instrument.c
@@ -188,11 +188,8 @@ InstrStartParallelQuery(void)
void
InstrEndParallelQuery(BufferUsage *bufusage, WalUsage *walusage)
{
- if (bufusage)
- {
- memset(bufusage, 0, sizeof(BufferUsage));
- BufferUsageAccumDiff(bufusage, &pgBufferUsage, &save_pgBufferUsage);
- }
+ memset(bufusage, 0, sizeof(BufferUsage));
+ BufferUsageAccumDiff(bufusage, &pgBufferUsage, &save_pgBufferUsage);
memset(walusage, 0, sizeof(WalUsage));
WalUsageAccumDiff(walusage, &pgWalUsage, &save_pgWalUsage);
}
@@ -201,8 +198,7 @@ InstrEndParallelQuery(BufferUsage *bufusage, WalUsage *walusage)
void
InstrAccumParallelQuery(BufferUsage *bufusage, WalUsage *walusage)
{
- if (bufusage)
- BufferUsageAdd(&pgBufferUsage, bufusage);
+ BufferUsageAdd(&pgBufferUsage, bufusage);
WalUsageAdd(&pgWalUsage, walusage);
}
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-06 07:16 Amit Kapila <amit.kapila16@gmail.com>
parent: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
0 siblings, 1 reply; 43+ messages in thread
From: Amit Kapila @ 2020-04-06 07:16 UTC (permalink / raw)
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; +Cc: Julien Rouhaud <rjuju123@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Mon, Apr 6, 2020 at 11:19 AM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> The attached patch changes to the above comment and removed the code
> that is used to un-support only buffer usage accumulation.
>
So, IIUC, the purpose of this patch will be to count the buffer usage
due to the heap scan (in heapam_index_build_range_scan) we perform
while parallel create index? Because the index creation itself won't
use buffer manager.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-06 07:24 Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 43+ messages in thread
From: Masahiko Sawada @ 2020-04-06 07:24 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Julien Rouhaud <rjuju123@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Mon, 6 Apr 2020 at 16:16, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Apr 6, 2020 at 11:19 AM Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
> >
> > The attached patch changes to the above comment and removed the code
> > that is used to un-support only buffer usage accumulation.
> >
>
> So, IIUC, the purpose of this patch will be to count the buffer usage
> due to the heap scan (in heapam_index_build_range_scan) we perform
> while parallel create index? Because the index creation itself won't
> use buffer manager.
Oops, I'd missed Peter's comment. Btree index doesn't use
heapam_index_build_range_scan so it's not necessary. Sorry for the
noise.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-06 09:21 Amit Kapila <amit.kapila16@gmail.com>
parent: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
0 siblings, 1 reply; 43+ messages in thread
From: Amit Kapila @ 2020-04-06 09:21 UTC (permalink / raw)
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; +Cc: Julien Rouhaud <rjuju123@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Mon, Apr 6, 2020 at 12:55 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Mon, 6 Apr 2020 at 16:16, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Apr 6, 2020 at 11:19 AM Masahiko Sawada
> > <masahiko.sawada@2ndquadrant.com> wrote:
> > >
> > > The attached patch changes to the above comment and removed the code
> > > that is used to un-support only buffer usage accumulation.
> > >
> >
> > So, IIUC, the purpose of this patch will be to count the buffer usage
> > due to the heap scan (in heapam_index_build_range_scan) we perform
> > while parallel create index? Because the index creation itself won't
> > use buffer manager.
>
> Oops, I'd missed Peter's comment. Btree index doesn't use
> heapam_index_build_range_scan so it's not necessary.
>
AFAIU, it uses heapam_index_build_range_scan but for writing to index,
it doesn't use buffer manager. So, I guess probably we can accumulate
BufferUsage stats for parallel create index. What I wanted to know is
whether the extra lookup for pg_amproc or any other catalog access via
parallel workers is fine or we somehow want to eliminate that?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-06 17:40 Peter Geoghegan <pg@bowt.ie>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 43+ messages in thread
From: Peter Geoghegan @ 2020-04-06 17:40 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; Julien Rouhaud <rjuju123@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Mon, Apr 6, 2020 at 2:21 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> AFAIU, it uses heapam_index_build_range_scan but for writing to index,
> it doesn't use buffer manager.
Right. It doesn't need to use the buffer manager to write to the
index, unlike (say) GIN's CREATE INDEX.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-07 07:59 Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
parent: Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 43+ messages in thread
From: Masahiko Sawada @ 2020-04-07 07:59 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Julien Rouhaud <rjuju123@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Tue, 7 Apr 2020 at 02:40, Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Mon, Apr 6, 2020 at 2:21 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > AFAIU, it uses heapam_index_build_range_scan but for writing to index,
> > it doesn't use buffer manager.
>
> Right. It doesn't need to use the buffer manager to write to the
> index, unlike (say) GIN's CREATE INDEX.
Hmm, after more thoughts and testing, it seems to me that parallel
btree index creation uses buffer manager while scanning the table in
parallel, i.e in heapam_index_build_range_scan, which affects
shared_blks_xxx in pg_stat_statements. I've some parallel create index
tests with the current HEAD and with the attached patch. The table has
44248 blocks.
HEAD, no workers:
-[ RECORD 1 ]-------+----------
total_plan_time | 0
total_plan_time | 0
shared_blks_hit | 148
shared_blks_read | 44281
total_read_blks | 44429
shared_blks_dirtied | 44261
shared_blks_written | 24644
wal_records | 71693
wal_num_fpw | 71682
wal_bytes | 566815038
HEAD, 4 workers:
-[ RECORD 1 ]-------+----------
total_plan_time | 0
total_plan_time | 0
shared_blks_hit | 160
shared_blks_read | 8892
total_read_blks | 9052
shared_blks_dirtied | 8871
shared_blks_written | 5342
wal_records | 71693
wal_num_fpw | 71682
wal_bytes | 566815038
The WAL usage statistics are good but the buffer usage statistics seem
not correct.
Patched, no workers:
-[ RECORD 1 ]-------+----------
total_plan_time | 0
total_plan_time | 0
shared_blks_hit | 148
shared_blks_read | 44281
total_read_blks | 44429
shared_blks_dirtied | 44261
shared_blks_written | 24843
wal_records | 71693
wal_num_fpw | 71682
wal_bytes | 566815038
Patched, 4 workers:
-[ RECORD 1 ]-------+----------
total_plan_time | 0
total_plan_time | 0
shared_blks_hit | 172
shared_blks_read | 44282
total_read_blks | 44454
shared_blks_dirtied | 44261
shared_blks_written | 26968
wal_records | 71693
wal_num_fpw | 71682
wal_bytes | 566815038
Buffer usage statistics seem correct. The small differences would be
catalog lookups Peter mentioned.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
bufferusage_create_index_v4.patch (application/octet-stream, 5.5K) download
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index 29a6f5ade6..707dd0741f 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -71,6 +71,7 @@
#define PARALLEL_KEY_TUPLESORT_SPOOL2 UINT64CONST(0xA000000000000003)
#define PARALLEL_KEY_QUERY_TEXT UINT64CONST(0xA000000000000004)
#define PARALLEL_KEY_WAL_USAGE UINT64CONST(0xA000000000000005)
+#define PARALLEL_KEY_BUFFER_USAGE UINT64CONST(0xA000000000000006)
/*
* DISABLE_LEADER_PARTICIPATION disables the leader's participation in
@@ -194,6 +195,7 @@ typedef struct BTLeader
Sharedsort *sharedsort2;
Snapshot snapshot;
WalUsage *walusage;
+ BufferUsage *bufferusage;
} BTLeader;
/*
@@ -1457,6 +1459,7 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
BTSpool *btspool = buildstate->spool;
BTLeader *btleader = (BTLeader *) palloc0(sizeof(BTLeader));
WalUsage *walusage;
+ BufferUsage *bufferusage;
bool leaderparticipates = true;
char *sharedquery;
int querylen;
@@ -1510,16 +1513,19 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
}
/*
- * Estimate space for WalUsage -- PARALLEL_KEY_WAL_USAGE
+ * Estimate space for WalUsage and BufferUsage -- PARALLEL_KEY_WAL_USAGE
+ * and PARALLEL_KEY_BUFFER_USAGE.
*
- * WalUsage during execution of maintenance command can be used by an
- * extension that reports the WAL usage, such as pg_stat_statements. We
- * have no way of knowing whether anyone's looking at pgWalUsage, so do it
- * unconditionally.
+ * If there are no extensions loaded that care, we could skip this. We
+ * have no way of knowing whether anyone's looking at pgWalUsage or
+ * pgBufferUsage, so do it unconditionally.
*/
shm_toc_estimate_chunk(&pcxt->estimator,
mul_size(sizeof(WalUsage), pcxt->nworkers));
shm_toc_estimate_keys(&pcxt->estimator, 1);
+ shm_toc_estimate_chunk(&pcxt->estimator,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_estimate_keys(&pcxt->estimator, 1);
/* Finally, estimate PARALLEL_KEY_QUERY_TEXT space */
querylen = strlen(debug_query_string);
@@ -1592,10 +1598,16 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
memcpy(sharedquery, debug_query_string, querylen + 1);
shm_toc_insert(pcxt->toc, PARALLEL_KEY_QUERY_TEXT, sharedquery);
- /* Allocate space for each worker's WalUsage; no need to initialize */
+ /*
+ * Allocate space for each worker's WalUsage and BufferUsage; no need to
+ * initialize.
+ */
walusage = shm_toc_allocate(pcxt->toc,
mul_size(sizeof(WalUsage), pcxt->nworkers));
shm_toc_insert(pcxt->toc, PARALLEL_KEY_WAL_USAGE, walusage);
+ bufferusage = shm_toc_allocate(pcxt->toc,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_insert(pcxt->toc, PARALLEL_KEY_BUFFER_USAGE, bufferusage);
/* Launch workers, saving status for leader/caller */
LaunchParallelWorkers(pcxt);
@@ -1608,6 +1620,7 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
btleader->sharedsort2 = sharedsort2;
btleader->snapshot = snapshot;
btleader->walusage = walusage;
+ btleader->bufferusage = bufferusage;
/* If no workers were successfully launched, back out (do serial build) */
if (pcxt->nworkers_launched == 0)
@@ -1646,7 +1659,7 @@ _bt_end_parallel(BTLeader *btleader)
* or we might get incomplete data.)
*/
for (i = 0; i < btleader->pcxt->nworkers_launched; i++)
- InstrAccumParallelQuery(NULL, &btleader->walusage[i]);
+ InstrAccumParallelQuery(&btleader->bufferusage[i], &btleader->walusage[i]);
/* Free last reference to MVCC snapshot, if one was used */
if (IsMVCCSnapshot(btleader->snapshot))
@@ -1779,6 +1792,7 @@ _bt_parallel_build_main(dsm_segment *seg, shm_toc *toc)
LOCKMODE heapLockmode;
LOCKMODE indexLockmode;
WalUsage *walusage;
+ BufferUsage *bufferusage;
int sortmem;
#ifdef BTREE_BUILD_STATS
@@ -1848,9 +1862,11 @@ _bt_parallel_build_main(dsm_segment *seg, shm_toc *toc)
_bt_parallel_scan_and_sort(btspool, btspool2, btshared, sharedsort,
sharedsort2, sortmem, false);
- /* Report WAL usage during parallel execution */
+ /* Report WAL/buffer usage during parallel execution */
+ bufferusage = shm_toc_lookup(toc, PARALLEL_KEY_BUFFER_USAGE, false);
walusage = shm_toc_lookup(toc, PARALLEL_KEY_WAL_USAGE, false);
- InstrEndParallelQuery(NULL, &walusage[ParallelWorkerNumber]);
+ InstrEndParallelQuery(&bufferusage[ParallelWorkerNumber],
+ &walusage[ParallelWorkerNumber]);
#ifdef BTREE_BUILD_STATS
if (log_btree_build_stats)
diff --git a/src/backend/executor/instrument.c b/src/backend/executor/instrument.c
index 74ee4808e3..3b9c6aebb9 100644
--- a/src/backend/executor/instrument.c
+++ b/src/backend/executor/instrument.c
@@ -188,11 +188,8 @@ InstrStartParallelQuery(void)
void
InstrEndParallelQuery(BufferUsage *bufusage, WalUsage *walusage)
{
- if (bufusage)
- {
- memset(bufusage, 0, sizeof(BufferUsage));
- BufferUsageAccumDiff(bufusage, &pgBufferUsage, &save_pgBufferUsage);
- }
+ memset(bufusage, 0, sizeof(BufferUsage));
+ BufferUsageAccumDiff(bufusage, &pgBufferUsage, &save_pgBufferUsage);
memset(walusage, 0, sizeof(WalUsage));
WalUsageAccumDiff(walusage, &pgWalUsage, &save_pgWalUsage);
}
@@ -201,8 +198,7 @@ InstrEndParallelQuery(BufferUsage *bufusage, WalUsage *walusage)
void
InstrAccumParallelQuery(BufferUsage *bufusage, WalUsage *walusage)
{
- if (bufusage)
- BufferUsageAdd(&pgBufferUsage, bufusage);
+ BufferUsageAdd(&pgBufferUsage, bufusage);
WalUsageAdd(&pgWalUsage, walusage);
}
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-07 08:42 Amit Kapila <amit.kapila16@gmail.com>
parent: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
0 siblings, 1 reply; 43+ messages in thread
From: Amit Kapila @ 2020-04-07 08:42 UTC (permalink / raw)
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; +Cc: Peter Geoghegan <pg@bowt.ie>; Julien Rouhaud <rjuju123@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Tue, Apr 7, 2020 at 1:30 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> Buffer usage statistics seem correct. The small differences would be
> catalog lookups Peter mentioned.
>
Agreed, but can you check which part of code does that lookup? I want
to see if we can avoid that from buffer usage stats or at least write
a comment about it, otherwise, we might have to face this question
again and again.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-07 09:29 Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 43+ messages in thread
From: Masahiko Sawada @ 2020-04-07 09:29 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Peter Geoghegan <pg@bowt.ie>; Julien Rouhaud <rjuju123@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Tue, 7 Apr 2020 at 17:42, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Apr 7, 2020 at 1:30 PM Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
> >
> > Buffer usage statistics seem correct. The small differences would be
> > catalog lookups Peter mentioned.
> >
>
> Agreed, but can you check which part of code does that lookup? I want
> to see if we can avoid that from buffer usage stats or at least write
> a comment about it, otherwise, we might have to face this question
> again and again.
Okay, I'll check it.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-07 11:47 Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
parent: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
0 siblings, 1 reply; 43+ messages in thread
From: Masahiko Sawada @ 2020-04-07 11:47 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Peter Geoghegan <pg@bowt.ie>; Julien Rouhaud <rjuju123@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Tue, 7 Apr 2020 at 18:29, Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Tue, 7 Apr 2020 at 17:42, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Apr 7, 2020 at 1:30 PM Masahiko Sawada
> > <masahiko.sawada@2ndquadrant.com> wrote:
> > >
> > > Buffer usage statistics seem correct. The small differences would be
> > > catalog lookups Peter mentioned.
> > >
> >
> > Agreed, but can you check which part of code does that lookup? I want
> > to see if we can avoid that from buffer usage stats or at least write
> > a comment about it, otherwise, we might have to face this question
> > again and again.
>
> Okay, I'll check it.
>
I've checked the buffer usage differences when parallel btree index creation.
TL;DR;
During tuple sorting individual parallel workers read blocks of
pg_amproc and pg_amproc_fam_proc_index to get the sort support
function. The call flow is like:
ParallelWorkerMain()
_bt_parallel_scan_and_sort()
tuplesort_begin_index_btree()
PrepareSortSupportFromIndexRel()
FinishSortSupportFunction()
get_opfamily_proc()
The details are as follows.
I populated the test table by the following scripts:
create table test (c int) with (autovacuum_enabled = off, parallel_workers = 8);
insert into test select generate_series(1,10000000);
and create index DDL is:
create index test_idx on test (c);
Before executing the test script, I've put code at the following 4
places which checks the buffer usage at that point, and calculated the
difference between points: (a), (b) and (c). For example, (b) shows
the number of blocks read or hit during executing scanning heap and
building index.
1. Before executing CREATE INDEX command (at pgss_ProcessUtility())
(a)
2. Before parallel create index (at _bt_begin_parallel())
(b)
3. After parallel create index, after accumlating workers stats (at
_bt_end_parallel())
(c)
4. After executing CREATE INDEX command (at pgss_ProcessUtility())
And here is the results:
2 workers:
(a) hit: 107, read: 26
(b) hit: 12(=6+3+3), read: 44248(=15538+14453+14527)
(c) hit: 13, read: 2
total hit: 132, read:44276
4 workers:
(a) hit: 107, read: 26
(b) hit: 18(=6+3+3+3+3), read: 44248(=9368+8582+8544+9250+8504)
(c) hit: 13, read: 2
total hit: 138, read:44276
The table 'test' has 44276 blocks.
From the above results, the total number of reading blocks (44248
blocks) during parallel index creation is stable and equals to the
number of blocks of the test table. And we can see that extra three
blocks are read per workers. These three blocks are two for
pg_amproc_fam_proc_index and one for pg_amproc. That is, individual
parallel workers accesses these relations to get the sort support
function. The full backtrace is:
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGSTOP
* frame #0: 0x00007fff779c561a libsystem_kernel.dylib`__select + 10
frame #1: 0x000000010cc9f90d postgres`pg_usleep(microsec=20000000)
at pgsleep.c:56:10
frame #2: 0x000000010ca5a668
postgres`ReadBuffer_common(smgr=0x00007fe872848f70,
relpersistence='p', forkNum=MAIN_FORKNUM, blockNum=3, mode=RBM_NORMAL,
strategy=0x0000000000000000, hit=0x00007ffee363071b) at bufmgr.c:685:3
frame #3: 0x000000010ca5a4b6
postgres`ReadBufferExtended(reln=0x000000010d58f790,
forkNum=MAIN_FORKNUM, blockNum=3, mode=RBM_NORMAL,
strategy=0x0000000000000000) at bufmgr.c:628:8
frame #4: 0x000000010ca5a397
postgres`ReadBuffer(reln=0x000000010d58f790, blockNum=3) at
bufmgr.c:560:9
frame #5: 0x000000010c67187e
postgres`_bt_getbuf(rel=0x000000010d58f790, blkno=3, access=1) at
nbtpage.c:792:9
frame #6: 0x000000010c670507
postgres`_bt_getroot(rel=0x000000010d58f790, access=1) at
nbtpage.c:294:13
frame #7: 0x000000010c679393
postgres`_bt_search(rel=0x000000010d58f790, key=0x00007ffee36312d0,
bufP=0x00007ffee3631bec, access=1, snapshot=0x00007fe8728388e0) at
nbtsearch.c:107:10
frame #8: 0x000000010c67b489
postgres`_bt_first(scan=0x00007fe86f814998, dir=ForwardScanDirection)
at nbtsearch.c:1355:10
frame #9: 0x000000010c676869
postgres`btgettuple(scan=0x00007fe86f814998, dir=ForwardScanDirection)
at nbtree.c:253:10
frame #10: 0x000000010c6656ad
postgres`index_getnext_tid(scan=0x00007fe86f814998,
direction=ForwardScanDirection) at indexam.c:530:10
frame #11: 0x000000010c66585b
postgres`index_getnext_slot(scan=0x00007fe86f814998,
direction=ForwardScanDirection, slot=0x00007fe86f814880) at
indexam.c:622:10
frame #12: 0x000000010c663eac
postgres`systable_getnext(sysscan=0x00007fe86f814828) at genam.c:454:7
frame #13: 0x000000010cc0be41
postgres`SearchCatCacheMiss(cache=0x00007fe872818e80, nkeys=4,
hashValue=3052139574, hashIndex=6, v1=1976, v2=23, v3=23, v4=2) at
catcache.c:1368:9
frame #14: 0x000000010cc0bced
postgres`SearchCatCacheInternal(cache=0x00007fe872818e80, nkeys=4,
v1=1976, v2=23, v3=23, v4=2) at catcache.c:1299:9
frame #15: 0x000000010cc0baa8
postgres`SearchCatCache4(cache=0x00007fe872818e80, v1=1976, v2=23,
v3=23, v4=2) at catcache.c:1191:9
frame #16: 0x000000010cc27c82 postgres`SearchSysCache4(cacheId=5,
key1=1976, key2=23, key3=23, key4=2) at syscache.c:1156:9
frame #17: 0x000000010cc105dd
postgres`get_opfamily_proc(opfamily=1976, lefttype=23, righttype=23,
procnum=2) at lsyscache.c:751:7
frame #18: 0x000000010cc72e1d
postgres`FinishSortSupportFunction(opfamily=1976, opcintype=23,
ssup=0x00007fe86f8147d0) at sortsupport.c:99:24
frame #19: 0x000000010cc73100
postgres`PrepareSortSupportFromIndexRel(indexRel=0x000000010d5ced48,
strategy=1, ssup=0x00007fe86f8147d0) at sortsupport.c:176:2
frame #20: 0x000000010cc75463
postgres`tuplesort_begin_index_btree(heapRel=0x000000010d5cf808,
indexRel=0x000000010d5ced48, enforceUnique=false, workMem=21845,
coordinate=0x00007fe872839248, randomAccess=false) at
tuplesort.c:1114:3
frame #21: 0x000000010c681ffc
postgres`_bt_parallel_scan_and_sort(btspool=0x00007fe872839738,
btspool2=0x0000000000000000, btshared=0x000000010d56c4c0,
sharedsort=0x000000010d56c460, sharedsort2=0x0000000000000000,
sortmem=21845, progress=false) at nbtsort.c:1941:23
frame #22: 0x000000010c681eb2
postgres`_bt_parallel_build_main(seg=0x00007fe87280a058,
toc=0x000000010d56c000) at nbtsort.c:1889:2
frame #23: 0x000000010c6b7358
postgres`ParallelWorkerMain(main_arg=1169089032) at parallel.c:1471:2
frame #24: 0x000000010c9da86f postgres`StartBackgroundWorker at
bgworker.c:813:2
frame #25: 0x000000010c9efbc0
postgres`do_start_bgworker(rw=0x00007fe86f419290) at
postmaster.c:5852:4
frame #26: 0x000000010c9eff9f postgres`maybe_start_bgworkers at
postmaster.c:6078:9
frame #27: 0x000000010c9eee99
postgres`sigusr1_handler(postgres_signal_arg=30) at
postmaster.c:5247:3
frame #28: 0x00007fff77a74b5d libsystem_platform.dylib`_sigtramp + 29
frame #29: 0x00007fff779c561b libsystem_kernel.dylib`__select + 11
frame #30: 0x000000010c9ea48c postgres`ServerLoop at postmaster.c:1691:13
frame #31: 0x000000010c9e9e06 postgres`PostmasterMain(argc=5,
argv=0x00007fe86f4036f0) at postmaster.c:1400:11
frame #32: 0x000000010c8ee399 postgres`main(argc=<unavailable>,
argv=<unavailable>) at main.c:210:3
frame #33: 0x00007fff778893d5 libdyld.dylib`start + 1
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-08 05:43 Amit Kapila <amit.kapila16@gmail.com>
parent: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
0 siblings, 1 reply; 43+ messages in thread
From: Amit Kapila @ 2020-04-08 05:43 UTC (permalink / raw)
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; +Cc: Peter Geoghegan <pg@bowt.ie>; Julien Rouhaud <rjuju123@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Tue, Apr 7, 2020 at 5:17 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Tue, 7 Apr 2020 at 18:29, Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
> >
> > On Tue, 7 Apr 2020 at 17:42, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Tue, Apr 7, 2020 at 1:30 PM Masahiko Sawada
> > > <masahiko.sawada@2ndquadrant.com> wrote:
> > > >
> > > > Buffer usage statistics seem correct. The small differences would be
> > > > catalog lookups Peter mentioned.
> > > >
> > >
> > > Agreed, but can you check which part of code does that lookup? I want
> > > to see if we can avoid that from buffer usage stats or at least write
> > > a comment about it, otherwise, we might have to face this question
> > > again and again.
> >
> > Okay, I'll check it.
> >
>
> I've checked the buffer usage differences when parallel btree index creation.
>
> TL;DR;
>
> During tuple sorting individual parallel workers read blocks of
> pg_amproc and pg_amproc_fam_proc_index to get the sort support
> function. The call flow is like:
>
> ParallelWorkerMain()
> _bt_parallel_scan_and_sort()
> tuplesort_begin_index_btree()
> PrepareSortSupportFromIndexRel()
> FinishSortSupportFunction()
> get_opfamily_proc()
>
Thanks for the investigation. I don't see we can do anything special
about this. In an ideal world, this should be done once and not for
each worker but I guess it doesn't matter too much. I am not sure if
it is worth adding a comment for this, what do you think?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-08 06:23 Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 2 replies; 43+ messages in thread
From: Masahiko Sawada @ 2020-04-08 06:23 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Peter Geoghegan <pg@bowt.ie>; Julien Rouhaud <rjuju123@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Wed, 8 Apr 2020 at 14:44, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Apr 7, 2020 at 5:17 PM Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
> >
> > On Tue, 7 Apr 2020 at 18:29, Masahiko Sawada
> > <masahiko.sawada@2ndquadrant.com> wrote:
> > >
> > > On Tue, 7 Apr 2020 at 17:42, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Tue, Apr 7, 2020 at 1:30 PM Masahiko Sawada
> > > > <masahiko.sawada@2ndquadrant.com> wrote:
> > > > >
> > > > > Buffer usage statistics seem correct. The small differences would be
> > > > > catalog lookups Peter mentioned.
> > > > >
> > > >
> > > > Agreed, but can you check which part of code does that lookup? I want
> > > > to see if we can avoid that from buffer usage stats or at least write
> > > > a comment about it, otherwise, we might have to face this question
> > > > again and again.
> > >
> > > Okay, I'll check it.
> > >
> >
> > I've checked the buffer usage differences when parallel btree index creation.
> >
> > TL;DR;
> >
> > During tuple sorting individual parallel workers read blocks of
> > pg_amproc and pg_amproc_fam_proc_index to get the sort support
> > function. The call flow is like:
> >
> > ParallelWorkerMain()
> > _bt_parallel_scan_and_sort()
> > tuplesort_begin_index_btree()
> > PrepareSortSupportFromIndexRel()
> > FinishSortSupportFunction()
> > get_opfamily_proc()
> >
>
> Thanks for the investigation. I don't see we can do anything special
> about this. In an ideal world, this should be done once and not for
> each worker but I guess it doesn't matter too much. I am not sure if
> it is worth adding a comment for this, what do you think?
>
I agree with you. If the differences were considerably large probably
we would do something but I think we don't need to anything at this
time.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-08 07:04 Amit Kapila <amit.kapila16@gmail.com>
parent: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
1 sibling, 1 reply; 43+ messages in thread
From: Amit Kapila @ 2020-04-08 07:04 UTC (permalink / raw)
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; +Cc: Peter Geoghegan <pg@bowt.ie>; Julien Rouhaud <rjuju123@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Wed, Apr 8, 2020 at 11:53 AM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Wed, 8 Apr 2020 at 14:44, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> >
> > Thanks for the investigation. I don't see we can do anything special
> > about this. In an ideal world, this should be done once and not for
> > each worker but I guess it doesn't matter too much. I am not sure if
> > it is worth adding a comment for this, what do you think?
> >
>
> I agree with you. If the differences were considerably large probably
> we would do something but I think we don't need to anything at this
> time.
>
Fair enough, can you once check this in back-branches as this needs to
be backpatched? I will do that once by myself as well.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-08 07:11 Julien Rouhaud <rjuju123@gmail.com>
parent: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
1 sibling, 0 replies; 43+ messages in thread
From: Julien Rouhaud @ 2020-04-08 07:11 UTC (permalink / raw)
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Peter Geoghegan <pg@bowt.ie>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Wed, Apr 8, 2020 at 8:23 AM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Wed, 8 Apr 2020 at 14:44, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Apr 7, 2020 at 5:17 PM Masahiko Sawada
> > <masahiko.sawada@2ndquadrant.com> wrote:
> > >
> > > On Tue, 7 Apr 2020 at 18:29, Masahiko Sawada
> > > <masahiko.sawada@2ndquadrant.com> wrote:
> > > >
> > > > On Tue, 7 Apr 2020 at 17:42, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > >
> > > > > On Tue, Apr 7, 2020 at 1:30 PM Masahiko Sawada
> > > > > <masahiko.sawada@2ndquadrant.com> wrote:
> > > > > >
> > > > > > Buffer usage statistics seem correct. The small differences would be
> > > > > > catalog lookups Peter mentioned.
> > > > > >
> > > > >
> > > > > Agreed, but can you check which part of code does that lookup? I want
> > > > > to see if we can avoid that from buffer usage stats or at least write
> > > > > a comment about it, otherwise, we might have to face this question
> > > > > again and again.
> > > >
> > > > Okay, I'll check it.
> > > >
> > >
> > > I've checked the buffer usage differences when parallel btree index creation.
> > >
> > > TL;DR;
> > >
> > > During tuple sorting individual parallel workers read blocks of
> > > pg_amproc and pg_amproc_fam_proc_index to get the sort support
> > > function. The call flow is like:
> > >
> > > ParallelWorkerMain()
> > > _bt_parallel_scan_and_sort()
> > > tuplesort_begin_index_btree()
> > > PrepareSortSupportFromIndexRel()
> > > FinishSortSupportFunction()
> > > get_opfamily_proc()
> > >
> >
> > Thanks for the investigation. I don't see we can do anything special
> > about this. In an ideal world, this should be done once and not for
> > each worker but I guess it doesn't matter too much. I am not sure if
> > it is worth adding a comment for this, what do you think?
> >
>
> I agree with you. If the differences were considerably large probably
> we would do something but I think we don't need to anything at this
> time.
+1
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-08 08:19 Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 43+ messages in thread
From: Masahiko Sawada @ 2020-04-08 08:19 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Peter Geoghegan <pg@bowt.ie>; Julien Rouhaud <rjuju123@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Wed, 8 Apr 2020 at 16:04, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Apr 8, 2020 at 11:53 AM Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
> >
> > On Wed, 8 Apr 2020 at 14:44, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > >
> > > Thanks for the investigation. I don't see we can do anything special
> > > about this. In an ideal world, this should be done once and not for
> > > each worker but I guess it doesn't matter too much. I am not sure if
> > > it is worth adding a comment for this, what do you think?
> > >
> >
> > I agree with you. If the differences were considerably large probably
> > we would do something but I think we don't need to anything at this
> > time.
> >
>
> Fair enough, can you once check this in back-branches as this needs to
> be backpatched? I will do that once by myself as well.
I've done the same test with HEAD of both REL_12_STABLE and
REL_11_STABLE. I think the patch needs to be backpatched to PG11 where
parallel index creation was introduced. I've attached the patches
for PG12 and PG11 I used for this test for reference.
Here are the results:
* PG12
With no worker:
-[ RECORD 1 ]-------+-------------
shared_blks_hit | 119
shared_blks_read | 44283
total_read_blks | 44402
shared_blks_dirtied | 44262
shared_blks_written | 24925
With 4 workers:
-[ RECORD 1 ]-------+------------
shared_blks_hit | 128
shared_blks_read | 8844
total_read_blks | 8972
shared_blks_dirtied | 8822
shared_blks_written | 5393
With 4 workers after patching:
-[ RECORD 1 ]-------+------------
shared_blks_hit | 140
shared_blks_read | 44284
total_read_blks | 44424
shared_blks_dirtied | 44262
shared_blks_written | 26574
* PG11
With no worker:
-[ RECORD 1 ]-------+------------
shared_blks_hit | 124
shared_blks_read | 44284
total_read_blks | 44408
shared_blks_dirtied | 44263
shared_blks_written | 24908
With 4 workers:
-[ RECORD 1 ]-------+-------------
shared_blks_hit | 132
shared_blks_read | 8910
total_read_blks | 9042
shared_blks_dirtied | 8888
shared_blks_written | 5370
With 4 workers after patched:
-[ RECORD 1 ]-------+-------------
shared_blks_hit | 144
shared_blks_read | 44285
total_read_blks | 44429
shared_blks_dirtied | 44263
shared_blks_written | 26861
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments: download all (2 files, .tar.gz)
bufferusage_create_index_pg12.patch (application/x-patch, 4.4K) download
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index edc4a82b02..da5b39eb02 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -67,6 +67,7 @@
#include "access/xloginsert.h"
#include "catalog/index.h"
#include "commands/progress.h"
+#include "executor/instrument.h"
#include "miscadmin.h"
#include "pgstat.h"
#include "storage/smgr.h"
@@ -81,6 +82,7 @@
#define PARALLEL_KEY_TUPLESORT UINT64CONST(0xA000000000000002)
#define PARALLEL_KEY_TUPLESORT_SPOOL2 UINT64CONST(0xA000000000000003)
#define PARALLEL_KEY_QUERY_TEXT UINT64CONST(0xA000000000000004)
+#define PARALLEL_KEY_BUFFER_USAGE UINT64CONST(0xA000000000000005)
/*
* DISABLE_LEADER_PARTICIPATION disables the leader's participation in
@@ -203,6 +205,7 @@ typedef struct BTLeader
Sharedsort *sharedsort;
Sharedsort *sharedsort2;
Snapshot snapshot;
+ BufferUsage *bufferusage;
} BTLeader;
/*
@@ -1336,6 +1339,7 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
Sharedsort *sharedsort2;
BTSpool *btspool = buildstate->spool;
BTLeader *btleader = (BTLeader *) palloc0(sizeof(BTLeader));
+ BufferUsage *bufferusage;
bool leaderparticipates = true;
char *sharedquery;
int querylen;
@@ -1388,6 +1392,17 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
shm_toc_estimate_keys(&pcxt->estimator, 3);
}
+ /*
+ * Estimate space for BufferUsage -- PARALLEL_KEY_BUFFER_USAGE.
+ *
+ * If there are no extensions loaded that care, we could skip this. We
+ * have no way of knowing whether anyone's looking at pgBufferUsage,
+ * so do it unconditionally.
+ */
+ shm_toc_estimate_chunk(&pcxt->estimator,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_estimate_keys(&pcxt->estimator, 1);
+
/* Finally, estimate PARALLEL_KEY_QUERY_TEXT space */
querylen = strlen(debug_query_string);
shm_toc_estimate_chunk(&pcxt->estimator, querylen + 1);
@@ -1459,6 +1474,11 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
memcpy(sharedquery, debug_query_string, querylen + 1);
shm_toc_insert(pcxt->toc, PARALLEL_KEY_QUERY_TEXT, sharedquery);
+ /* Allocate space for each worker's BufferUsage; no need to initialize */
+ bufferusage = shm_toc_allocate(pcxt->toc,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_insert(pcxt->toc, PARALLEL_KEY_BUFFER_USAGE, bufferusage);
+
/* Launch workers, saving status for leader/caller */
LaunchParallelWorkers(pcxt);
btleader->pcxt = pcxt;
@@ -1469,6 +1489,7 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
btleader->sharedsort = sharedsort;
btleader->sharedsort2 = sharedsort2;
btleader->snapshot = snapshot;
+ btleader->bufferusage = bufferusage;
/* If no workers were successfully launched, back out (do serial build) */
if (pcxt->nworkers_launched == 0)
@@ -1497,8 +1518,18 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
static void
_bt_end_parallel(BTLeader *btleader)
{
+ int i;
+
/* Shutdown worker processes */
WaitForParallelWorkersToFinish(btleader->pcxt);
+
+ /*
+ * Next, accumulate buffer usage. (This must wait for the workers to
+ * finish, or we might get incomplete data.)
+ */
+ for (i = 0; i < btleader->pcxt->nworkers_launched; i++)
+ InstrAccumParallelQuery(&btleader->bufferusage[i]);
+
/* Free last reference to MVCC snapshot, if one was used */
if (IsMVCCSnapshot(btleader->snapshot))
UnregisterSnapshot(btleader->snapshot);
@@ -1629,6 +1660,7 @@ _bt_parallel_build_main(dsm_segment *seg, shm_toc *toc)
Relation indexRel;
LOCKMODE heapLockmode;
LOCKMODE indexLockmode;
+ BufferUsage *bufferusage;
int sortmem;
#ifdef BTREE_BUILD_STATS
@@ -1690,11 +1722,18 @@ _bt_parallel_build_main(dsm_segment *seg, shm_toc *toc)
tuplesort_attach_shared(sharedsort2, seg);
}
+ /* Prepare to track buffer usage during parallel execution */
+ InstrStartParallelQuery();
+
/* Perform sorting of spool, and possibly a spool2 */
sortmem = maintenance_work_mem / btshared->scantuplesortstates;
_bt_parallel_scan_and_sort(btspool, btspool2, btshared, sharedsort,
sharedsort2, sortmem, false);
+ /* Report buffer usage during parallel execution */
+ bufferusage = shm_toc_lookup(toc, PARALLEL_KEY_BUFFER_USAGE, false);
+ InstrEndParallelQuery(&bufferusage[ParallelWorkerNumber]);
+
#ifdef BTREE_BUILD_STATS
if (log_btree_build_stats)
{
bufferusage_create_index_pg11.patch (application/x-patch, 4.4K) download
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index dab41ea298..54627b786a 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -64,6 +64,7 @@
#include "access/xlog.h"
#include "access/xloginsert.h"
#include "catalog/index.h"
+#include "executor/instrument.h"
#include "miscadmin.h"
#include "pgstat.h"
#include "storage/smgr.h"
@@ -78,6 +79,7 @@
#define PARALLEL_KEY_TUPLESORT UINT64CONST(0xA000000000000002)
#define PARALLEL_KEY_TUPLESORT_SPOOL2 UINT64CONST(0xA000000000000003)
#define PARALLEL_KEY_QUERY_TEXT UINT64CONST(0xA000000000000004)
+#define PARALLEL_KEY_BUFFER_USAGE UINT64CONST(0xA000000000000005)
/*
* DISABLE_LEADER_PARTICIPATION disables the leader's participation in
@@ -192,6 +194,7 @@ typedef struct BTLeader
Sharedsort *sharedsort;
Sharedsort *sharedsort2;
Snapshot snapshot;
+ BufferUsage *bufferusage;
} BTLeader;
/*
@@ -1240,6 +1243,7 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
Sharedsort *sharedsort2;
BTSpool *btspool = buildstate->spool;
BTLeader *btleader = (BTLeader *) palloc0(sizeof(BTLeader));
+ BufferUsage *bufferusage;
bool leaderparticipates = true;
char *sharedquery;
int querylen;
@@ -1292,6 +1296,17 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
shm_toc_estimate_keys(&pcxt->estimator, 3);
}
+ /*
+ * Estimate space for BufferUsage -- PARALLEL_KEY_BUFFER_USAGE.
+ *
+ * If there are no extensions loaded that care, we could skip this. We
+ * have no way of knowing whether anyone's looking at pgBufferUsage,
+ * so do it unconditionally.
+ */
+ shm_toc_estimate_chunk(&pcxt->estimator,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_estimate_keys(&pcxt->estimator, 1);
+
/* Finally, estimate PARALLEL_KEY_QUERY_TEXT space */
querylen = strlen(debug_query_string);
shm_toc_estimate_chunk(&pcxt->estimator, querylen + 1);
@@ -1361,6 +1376,11 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
memcpy(sharedquery, debug_query_string, querylen + 1);
shm_toc_insert(pcxt->toc, PARALLEL_KEY_QUERY_TEXT, sharedquery);
+ /* Allocate space for each worker's BufferUsage; no need to initialize */
+ bufferusage = shm_toc_allocate(pcxt->toc,
+ mul_size(sizeof(BufferUsage), pcxt->nworkers));
+ shm_toc_insert(pcxt->toc, PARALLEL_KEY_BUFFER_USAGE, bufferusage);
+
/* Launch workers, saving status for leader/caller */
LaunchParallelWorkers(pcxt);
btleader->pcxt = pcxt;
@@ -1371,6 +1391,7 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
btleader->sharedsort = sharedsort;
btleader->sharedsort2 = sharedsort2;
btleader->snapshot = snapshot;
+ btleader->bufferusage = bufferusage;
/* If no workers were successfully launched, back out (do serial build) */
if (pcxt->nworkers_launched == 0)
@@ -1399,8 +1420,18 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
static void
_bt_end_parallel(BTLeader *btleader)
{
+ int i;
+
/* Shutdown worker processes */
WaitForParallelWorkersToFinish(btleader->pcxt);
+
+ /*
+ * Next, accumulate buffer usage. (This must wait for the workers to
+ * finish, or we might get incomplete data.)
+ */
+ for (i = 0; i < btleader->pcxt->nworkers_launched; i++)
+ InstrAccumParallelQuery(&btleader->bufferusage[i]);
+
/* Free last reference to MVCC snapshot, if one was used */
if (IsMVCCSnapshot(btleader->snapshot))
UnregisterSnapshot(btleader->snapshot);
@@ -1537,6 +1568,7 @@ _bt_parallel_build_main(dsm_segment *seg, shm_toc *toc)
Relation indexRel;
LOCKMODE heapLockmode;
LOCKMODE indexLockmode;
+ BufferUsage *bufferusage;
int sortmem;
#ifdef BTREE_BUILD_STATS
@@ -1598,11 +1630,18 @@ _bt_parallel_build_main(dsm_segment *seg, shm_toc *toc)
tuplesort_attach_shared(sharedsort2, seg);
}
+ /* Prepare to track buffer usage during parallel execution */
+ InstrStartParallelQuery();
+
/* Perform sorting of spool, and possibly a spool2 */
sortmem = maintenance_work_mem / btshared->scantuplesortstates;
_bt_parallel_scan_and_sort(btspool, btspool2, btshared, sharedsort,
sharedsort2, sortmem);
+ /* Report buffer usage during parallel execution */
+ bufferusage = shm_toc_lookup(toc, PARALLEL_KEY_BUFFER_USAGE, false);
+ InstrEndParallelQuery(&bufferusage[ParallelWorkerNumber]);
+
#ifdef BTREE_BUILD_STATS
if (log_btree_build_stats)
{
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-08 09:05 Amit Kapila <amit.kapila16@gmail.com>
parent: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
0 siblings, 0 replies; 43+ messages in thread
From: Amit Kapila @ 2020-04-08 09:05 UTC (permalink / raw)
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; +Cc: Peter Geoghegan <pg@bowt.ie>; Julien Rouhaud <rjuju123@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Wed, Apr 8, 2020 at 1:49 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Wed, 8 Apr 2020 at 16:04, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Apr 8, 2020 at 11:53 AM Masahiko Sawada
> > <masahiko.sawada@2ndquadrant.com> wrote:
> > >
> > > On Wed, 8 Apr 2020 at 14:44, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > >
> > > > Thanks for the investigation. I don't see we can do anything special
> > > > about this. In an ideal world, this should be done once and not for
> > > > each worker but I guess it doesn't matter too much. I am not sure if
> > > > it is worth adding a comment for this, what do you think?
> > > >
> > >
> > > I agree with you. If the differences were considerably large probably
> > > we would do something but I think we don't need to anything at this
> > > time.
> > >
> >
> > Fair enough, can you once check this in back-branches as this needs to
> > be backpatched? I will do that once by myself as well.
>
> I've done the same test with HEAD of both REL_12_STABLE and
> REL_11_STABLE. I think the patch needs to be backpatched to PG11 where
> parallel index creation was introduced. I've attached the patches
> for PG12 and PG11 I used for this test for reference.
>
Thanks, I will once again verify and push this tomorrow if there are
no other comments.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-11 22:33 Justin Pryzby <pryzby@telsasoft.com>
parent: Julien Rouhaud <rjuju123@gmail.com>
1 sibling, 2 replies; 43+ messages in thread
From: Justin Pryzby @ 2020-04-11 22:33 UTC (permalink / raw)
To: Julien Rouhaud <rjuju123@gmail.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; robertmhaas@gmail.com; pgsql-hackers
On Sat, Mar 28, 2020 at 04:17:21PM +0100, Julien Rouhaud 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.
>
> I added an open for that, and adding Robert in Cc as 9da0cc352 is the first
> commit adding parallel maintenance.
I believe this is resolved for parallel vacuum in master and parallel create
index back to PG11.
I marked this as closed.
https://wiki.postgresql.org/index.php?title=PostgreSQL_13_Open_Items&diff=34802&oldid=34781
--
Justin
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-12 10:53 Julien Rouhaud <rjuju123@gmail.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 0 replies; 43+ messages in thread
From: Julien Rouhaud @ 2020-04-12 10:53 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
Le dim. 12 avr. 2020 à 00:33, Justin Pryzby <pryzby@telsasoft.com> a écrit :
> On Sat, Mar 28, 2020 at 04:17:21PM +0100, Julien Rouhaud wrote:
> >
> > 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.
> >
> > I added an open for that, and adding Robert in Cc as 9da0cc352 is the
> first
> > commit adding parallel maintenance.
>
> I believe this is resolved for parallel vacuum in master and parallel
> create
> index back to PG11.
>
indeed, I was about to take care of this too
> I marked this as closed.
>
> https://wiki.postgresql.org/index.php?title=PostgreSQL_13_Open_Items&diff=34802&oldid=34781
thanks a lot!
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch)
@ 2020-04-13 04:42 Amit Kapila <amit.kapila16@gmail.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 0 replies; 43+ messages in thread
From: Amit Kapila @ 2020-04-13 04:42 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Julien Rouhaud <rjuju123@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Sun, Apr 12, 2020 at 4:03 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Sat, Mar 28, 2020 at 04:17:21PM +0100, Julien Rouhaud 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.
> >
> > I added an open for that, and adding Robert in Cc as 9da0cc352 is the first
> > commit adding parallel maintenance.
>
> I believe this is resolved for parallel vacuum in master and parallel create
> index back to PG11.
>
> I marked this as closed.
> https://wiki.postgresql.org/index.php?title=PostgreSQL_13_Open_Items&diff=34802&oldid=34781
>
Okay, thanks.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 43+ messages in thread
end of thread, other threads:[~2020-04-13 04:42 UTC | newest]
Thread overview: 43+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-03-28 15:17 pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch) Julien Rouhaud <rjuju123@gmail.com>
2020-03-29 05:23 ` Amit Kapila <amit.kapila16@gmail.com>
2020-03-29 06:19 ` Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
2020-03-29 07:51 ` Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
2020-03-29 08:13 ` Julien Rouhaud <rjuju123@gmail.com>
2020-03-29 11:15 ` Amit Kapila <amit.kapila16@gmail.com>
2020-03-29 11:44 ` Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
2020-03-30 06:46 ` Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
2020-03-30 07:01 ` Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
2020-03-30 08:00 ` Julien Rouhaud <rjuju123@gmail.com>
2020-03-31 03:57 ` Amit Kapila <amit.kapila16@gmail.com>
2020-03-31 05:13 ` Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
2020-03-31 06:50 ` Dilip Kumar <dilipbalaut@gmail.com>
2020-03-31 14:02 ` Dilip Kumar <dilipbalaut@gmail.com>
2020-04-01 02:46 ` Amit Kapila <amit.kapila16@gmail.com>
2020-04-01 02:50 ` Dilip Kumar <dilipbalaut@gmail.com>
2020-04-01 02:56 ` Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
2020-04-01 03:21 ` Dilip Kumar <dilipbalaut@gmail.com>
2020-04-01 06:31 ` Amit Kapila <amit.kapila16@gmail.com>
2020-04-01 12:25 ` Dilip Kumar <dilipbalaut@gmail.com>
2020-04-01 07:11 ` Dilip Kumar <dilipbalaut@gmail.com>
2020-04-02 02:52 ` Amit Kapila <amit.kapila16@gmail.com>
2020-04-02 03:04 ` Peter Geoghegan <pg@bowt.ie>
2020-04-02 03:43 ` Dilip Kumar <dilipbalaut@gmail.com>
2020-04-02 04:43 ` Dilip Kumar <dilipbalaut@gmail.com>
2020-04-06 05:48 ` Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
2020-04-06 07:16 ` Amit Kapila <amit.kapila16@gmail.com>
2020-04-06 07:24 ` Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
2020-04-06 09:21 ` Amit Kapila <amit.kapila16@gmail.com>
2020-04-06 17:40 ` Peter Geoghegan <pg@bowt.ie>
2020-04-07 07:59 ` Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
2020-04-07 08:42 ` Amit Kapila <amit.kapila16@gmail.com>
2020-04-07 09:29 ` Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
2020-04-07 11:47 ` Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
2020-04-08 05:43 ` Amit Kapila <amit.kapila16@gmail.com>
2020-04-08 06:23 ` Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
2020-04-08 07:04 ` Amit Kapila <amit.kapila16@gmail.com>
2020-04-08 08:19 ` Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
2020-04-08 09:05 ` Amit Kapila <amit.kapila16@gmail.com>
2020-04-08 07:11 ` Julien Rouhaud <rjuju123@gmail.com>
2020-04-11 22:33 ` Justin Pryzby <pryzby@telsasoft.com>
2020-04-12 10:53 ` Julien Rouhaud <rjuju123@gmail.com>
2020-04-13 04:42 ` Amit Kapila <amit.kapila16@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox