Thread

Commits

Fix progress reporting of CLUSTER / VACUUM FULL
- da47e43dc32e 12.0 cited

Re: [HACKERS] CLUSTER command progress monitor

Tatsuro Yamada <yamatattsu@gmail.com> — 2019-09-14T04:06:32Z

Hi Alvaro!



> Hello Tatsuro,
> On 2019-Aug-13, Tatsuro Yamada wrote:
> > On 2019/08/02 3:43, Alvaro Herrera wrote:
> > > Hmm, I'm trying this out now and I don't see the index_rebuild_count
> > > ever go up.  I think it's because the indexes are built using parallel
> > > index build ... or maybe it was the table AM changes that moved things
> > > around, not sure.  There's a period at the end when the CLUSTER command
> > > keeps working, but it's gone from pg_stat_progress_cluster.
> >
> > Thanks for your report.
> > I'll investigate it. :)
>


I have fixed it.  Can you please verify?
>


Thanks! I can review your patch for fix it.
However, I was starting fixing the problem from the last day of PGConf.Asia
(11 Sep).
Attached file is WIP patch.In my patch, I added "command id" to all APIs of
progress reporting to isolate commands. Therefore, it doesn't allow to
cascade updating system views. And my patch is on WIP so it needs clean-up
and test.
I share it anyway. :)

Here is a test result of my patch.
The last column index_rebuild count is increased.
========================================
postgres=# select * from pg_stat_progress_cluster ; \watch 0.001;
11636|13591|postgres|16384|CLUSTER|initializing|0|0|0|0|0|0
11636|13591|postgres|16384|CLUSTER|index scanning heap|16389|251|251|0|0|0
...
11636|13591|postgres|16384|CLUSTER|index scanning
heap|16389|10000|10000|0|0|0
11636|13591|postgres|16384|CLUSTER|rebuilding
index|16389|10000|10000|0|0|0...
11636|13591|postgres|16384|CLUSTER|rebuilding index|16389|10000|10000|0|0|1
...
11636|13591|postgres|16384|CLUSTER|rebuilding index|16389|10000|10000|0|0|2
...
11636|13591|postgres|16384|CLUSTER|rebuilding index|16389|10000|10000|0|0|3
...
11636|13591|postgres|16384|CLUSTER|rebuilding index|16389|10000|10000|0|0|4
...
11636|13591|postgres|16384|CLUSTER|performing final
cleanup|16389|10000|10000|0|0|5
========================================

Thanks,
Tatsuro Yamada

Re: [HACKERS] CLUSTER command progress monitor

Michael Paquier <michael@paquier.xyz> — 2019-09-14T04:30:44Z

On Sat, Sep 14, 2019 at 01:06:32PM +0900, Tattsu Yama wrote:
> Thanks! I can review your patch for fix it.
> However, I was starting fixing the problem from the last day of PGConf.Asia
> (11 Sep).
> Attached file is WIP patch.In my patch, I added "command id" to all APIs of
> progress reporting to isolate commands. Therefore, it doesn't allow to
> cascade updating system views. And my patch is on WIP so it needs clean-up
> and test.
> I share it anyway. :)

+       if (cmdtype == PROGRESS_COMMAND_INVALID || beentry->st_progress_command == cmdtype)
+       {
+               PGSTAT_BEGIN_WRITE_ACTIVITY(beentry);
+               beentry->st_progress_param[index] = val;
+               PGSTAT_END_WRITE_ACTIVITY(beentry);
+       }
You basically don't need the progress reports if the command ID is
invalid, no?

Another note is that you don't actually fix the problems related to
the calls of pgstat_progress_end_command() which have been added for
REINDEX reporting, so a progress report started for CLUSTER can get
ended earlier than expected, preventing the follow-up progress updates
to show up.
--
Michael

Re: [HACKERS] CLUSTER command progress monitor

Tatsuro Yamada <yamatattsu@gmail.com> — 2019-09-15T03:35:10Z

Hi Michael!

> Attached file is WIP patch.In my patch, I added "command id" to all APIs
> of
> > progress reporting to isolate commands. Therefore, it doesn't allow to
> > cascade updating system views. And my patch is on WIP so it needs
> clean-up
> > and test.
> > I share it anyway. :)
>
> +       if (cmdtype == PROGRESS_COMMAND_INVALID ||
> beentry->st_progress_command == cmdtype)
> +       {
> +               PGSTAT_BEGIN_WRITE_ACTIVITY(beentry);
> +               beentry->st_progress_param[index] = val;
> +               PGSTAT_END_WRITE_ACTIVITY(beentry);
> +       }
> You basically don't need the progress reports if the command ID is
> invalid, no?
>


Ah, right.
I'll check and fix that today. :)



>
> Another note is that you don't actually fix the problems related to
> the calls of pgstat_progress_end_command() which have been added for
> REINDEX reporting, so a progress report started for CLUSTER can get
> ended earlier than expected, preventing the follow-up progress updates
> to show up.
>
>

Hmm... I fixed the problem. Please confirm the test result repeated below.
CLUSTER is able to get the last phase: performing final clean up by using
the patch.

# Test result
========================================
postgres=# select * from pg_stat_progress_cluster ; \watch 0.001;
11636|13591|postgres|16384|CLUSTER|initializing|0|0|0|0|0|0
11636|13591|postgres|16384|CLUSTER|index scanning heap|16389|251|251|0|0|0
11636|13591|postgres|16384|CLUSTER|index scanning
heap|16389|10000|10000|0|0|0
11636|13591|postgres|16384|CLUSTER|rebuilding
index|16389|10000|10000|0|0|0  <== The last column rebuild_index_count is
increasing!
11636|13591|postgres|16384|CLUSTER|rebuilding index|16389|10000|10000|0|0|1
11636|13591|postgres|16384|CLUSTER|rebuilding index|16389|10000|10000|0|0|2
11636|13591|postgres|16384|CLUSTER|rebuilding index|16389|10000|10000|0|0|3
11636|13591|postgres|16384|CLUSTER|rebuilding index|16389|10000|10000|0|0|4
11636|13591|postgres|16384|CLUSTER|performing final
cleanup|16389|10000|10000|0|0|5  <== The last phase of CLUSTER!
========================================

Thanks,
Tatsuro Yamada

Re: [HACKERS] CLUSTER command progress monitor

Tatsuro Yamada <yamatattsu@gmail.com> — 2019-09-16T06:26:10Z

Hi Michael,


>> > Attached file is WIP patch.In my patch, I added "command id" to all
>> APIs of
>> > progress reporting to isolate commands. Therefore, it doesn't allow to
>> > cascade updating system views. And my patch is on WIP so it needs
>> clean-up
>> > and test.
>> > I share it anyway. :)
>>
>> +       if (cmdtype == PROGRESS_COMMAND_INVALID ||
>> beentry->st_progress_command == cmdtype)
>> +       {
>> +               PGSTAT_BEGIN_WRITE_ACTIVITY(beentry);
>> +               beentry->st_progress_param[index] = val;
>> +               PGSTAT_END_WRITE_ACTIVITY(beentry);
>> +       }
>> You basically don't need the progress reports if the command ID is
>> invalid, no?
>>
>
>
> Ah, right.
> I'll check and fix that today. :)
>
>

I fixed the patch based on your comment.
Please find attached file.  :)

I should have explained the API changes more. I added cmdtype as a given
parameter for all functions (See below).
Therefore, I suppose that my patch is similar to the following fix as you
mentioned on -hackers.

- Allow only reporting for a given command ID, which would basically
> require to pass down the command ID to progress update APIs and bypass an
> update
> if the command ID provided by caller does not match the existing one
> started (?).


#pgstat.c
pgstat_progress_start_command(ProgressCommandType cmdtype,...)
   - Progress reporter starts when beentry->st_progress_command is
PROGRESS_COMMAND_INVALID

pgstat_progress_end_command(ProgressCommandType cmdtype,...)
   - Progress reporter ends when beentry->st_progress_command equals cmdtype

pgstat_progress_update_param(ProgressCommandType cmdtype,...)  and
pgstat_progress_update_multi_param(ProgressCommandType cmdtype,...)
   - Progress reporter updates parameters if beentry->st_progress_command
equals cmdtype

Note:
cmdtype means the ProgressCommandType below:

# pgstat.h
typedef enum ProgressCommandType
{
    PROGRESS_COMMAND_INVALID,
    PROGRESS_COMMAND_VACUUM,
    PROGRESS_COMMAND_CLUSTER,
    PROGRESS_COMMAND_CREATE_INDEX
} ProgressCommandType;

Thanks,
Tatsuro Yamada

Re: [HACKERS] CLUSTER command progress monitor

Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-09-16T14:12:15Z

On 2019-Sep-16, Tattsu Yama wrote:

> I should have explained the API changes more. I added cmdtype as a given
> parameter for all functions (See below).
> Therefore, I suppose that my patch is similar to the following fix as you
> mentioned on -hackers.

Is this fix strictly necessary for pg12, or is this something that we
can leave for pg13?


-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] CLUSTER command progress monitor

Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp> — 2019-09-17T02:01:28Z

Hi Alvaro,

On 2019/09/16 23:12, Alvaro Herrera wrote:
> On 2019-Sep-16, Tattsu Yama wrote:
> 
>> I should have explained the API changes more. I added cmdtype as a given
>> parameter for all functions (See below).
>> Therefore, I suppose that my patch is similar to the following fix as you
>> mentioned on -hackers.
> 
> Is this fix strictly necessary for pg12, or is this something that we
> can leave for pg13?

Not only me but many DBA needs this progress report feature on PG12,
therefore I'm trying to fix the problem. If you send other patch to
fix the problem, and it is more elegant than mine, I can withdraw my patch.
Anyway, I want to avoid this feature being reverted.
Do you have any ideas to fix the problem?

Thanks,
Tatsuro Yamada

Re: [HACKERS] CLUSTER command progress monitor

Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-09-17T02:08:32Z

On 2019-Sep-17, Tatsuro Yamada wrote:

> On 2019/09/16 23:12, Alvaro Herrera wrote:

> > Is this fix strictly necessary for pg12, or is this something that we
> > can leave for pg13?
> 
> Not only me but many DBA needs this progress report feature on PG12,
> therefore I'm trying to fix the problem. If you send other patch to
> fix the problem, and it is more elegant than mine, I can withdraw my patch.
> Anyway, I want to avoid this feature being reverted.
> Do you have any ideas to fix the problem?

I committed a fix for the originally reported problem as da47e43dc32e in
branch REL_12_STABLE.  Is that insufficient, and if so why?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] CLUSTER command progress monitor

Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp> — 2019-09-17T02:34:21Z

Hi Alvaro!

>>> Is this fix strictly necessary for pg12, or is this something that we
>>> can leave for pg13?
>>
>> Not only me but many DBA needs this progress report feature on PG12,
>> therefore I'm trying to fix the problem. If you send other patch to
>> fix the problem, and it is more elegant than mine, I can withdraw my patch.
>> Anyway, I want to avoid this feature being reverted.
>> Do you have any ideas to fix the problem?
> 
> I committed a fix for the originally reported problem as da47e43dc32e in
> branch REL_12_STABLE.  Is that insufficient, and if so why?


Ooops, I misunderstood. I now realized you committed your patch to
fix the problem. Thanks! I'll test it later. :)

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=da47e43dc32e3c5916396f0cbcfa974b371e4875


Thanks,
Tatsuro Yamada

Re: [HACKERS] CLUSTER command progress monitor

Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp> — 2019-09-17T03:30:12Z

Hi Alvaro!

>>>> Is this fix strictly necessary for pg12, or is this something that we
>>>> can leave for pg13?
>>>
>>> Not only me but many DBA needs this progress report feature on PG12,
>>> therefore I'm trying to fix the problem. If you send other patch to
>>> fix the problem, and it is more elegant than mine, I can withdraw my patch.
>>> Anyway, I want to avoid this feature being reverted.
>>> Do you have any ideas to fix the problem?
>>
>> I committed a fix for the originally reported problem as da47e43dc32e in
>> branch REL_12_STABLE.  Is that insufficient, and if so why?
> 
> 
> Ooops, I misunderstood. I now realized you committed your patch to
> fix the problem. Thanks! I'll test it later. :)
> 
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=da47e43dc32e3c5916396f0cbcfa974b371e4875


I tested your patch (da47e43d) and it works fine. Thanks! :)
So, my patch improving progress reporting API can leave for PG13.


#Test scenario
===================
[Session #1]
select * from pg_stat_progress_cluster ; \watch 0.0001

[Session #2]
create table hoge as select a from generate_series(1, 100000) a;
create index ind_hoge1 on hoge(a);
create index ind_hoge2 on hoge((a%2));
create index ind_hoge3 on hoge((a%3));
create index ind_hoge4 on hoge((a%4));
create index ind_hoge5 on hoge((a%5));
cluster hoge using ind_hoge1;
===================

#Test result
===================
22283|13593|postgres|16384|CLUSTER|initializing|0|0|0|0|0|0
...
22283|13593|postgres|16384|CLUSTER|rebuilding index|16387|100000|100000|0|0|0 <= Increasing from 0 to 5
22283|13593|postgres|16384|CLUSTER|rebuilding index|16387|100000|100000|0|0|1
22283|13593|postgres|16384|CLUSTER|rebuilding index|16387|100000|100000|0|0|2
22283|13593|postgres|16384|CLUSTER|rebuilding index|16387|100000|100000|0|0|3
22283|13593|postgres|16384|CLUSTER|rebuilding index|16387|100000|100000|0|0|4
22283|13593|postgres|16384|CLUSTER|performing final cleanup|16387|100000|100000|0|0|5
===================


Thanks,
Tatsuro Yamada

Re: [HACKERS] CLUSTER command progress monitor

Michael Paquier <michael@paquier.xyz> — 2019-09-17T05:13:39Z

On Mon, Sep 16, 2019 at 03:26:10PM +0900, Tattsu Yama wrote:
> I should have explained the API changes more. I added cmdtype as a given
> parameter for all functions (See below).
> Therefore, I suppose that my patch is similar to the following fix as you
> mentioned on -hackers.

Yes, that's an option I mentioned here, but it has drawbacks:
https://www.postgresql.org/message-id/20190914024547.GB15406@paquier.xyz

I have just looked at it again after a small rebase and there are
issues with the design of your patch:
- When aborting a transaction, we need to enforce a reset of the
command ID used in st_progress_command to be PROGRESS_COMMAND_INVALID.
Unfortunately, your patch does not consider the case where an error
happens while a command ID is set, causing a command to still be
tracked with the next transactions of the session.  Even worse, it
prevents pgstat_progress_start_command() to be called again in this
case for another command.
- CLUSTER can rebuild indexes, and we'd likely want to be able to
track some of the information from CREATE INDEX for CLUSTER.

The second issue is perhaps fine as it is not really straight-forward
to share the same progress phases across multiple commands, and we
could live without it for now, or require a follow-up patch to make
the information of CREATE INDEX available to CLUSTER.

Now, the first issue is of another caliber and a no-go :(

On HEAD, pgstat_progress_end_command() has the limitation to not be
able to stack multiple commands, so calling it in cascade has the
disadvantage to perhaps erase the progress state of a command (and it
is not designed for that anyway), which is what happens with CLUSTER
when reindex_index() starts a new progress report, but the simplicity
of the current infrastructure is very safe when it comes to failure
handling, to make sure that an reset happens as long as the command ID
is not invalid.  Your patch makes that part unpredictable.
--
Michael