Re: [HACKERS] CLUSTER command progress monitor

On 2019/03/02 4:15, Robert Haas wrote:
> On Thu, Feb 28, 2019 at 11:54 PM Tatsuro Yamada
> <yamada.tatsuro@lab.ntt.co.jp> wrote:
>> Attached patch is wip patch.


Thanks for your comments! :)
I revised the code and the document.


> +       <command>CLUSTER</command> and <command>VACUUM FULL</command>,
> showing current progress.
> 
> and -> or

Fixed.


> +   certain commands during command execution.  Currently, the suppoted
> +   progress reporting commands are <command>VACUUM</command> and
> <command>CLUSTER</command>.
> 
> suppoted -> supported
> 
> But I'd just say: Currently, the only commands which support progress
> reporting are <command>VACUUM</command> and
> <command>CLUSTER</command>.

I choose the latter. Fixed.


> +   Running <command>VACUUM FULL</command> is listed in
> <structname>pg_stat_progress_cluster</structname>
> +   view because it uses <command>CLUSTER</command> command
> internally.  See <xref linkend='cluster-progress-reporting'>.
> 
> How about: Running <command>VACUUM FULL</command> is listed in
> <structname>pg_stat_progress_cluster</structname> because both
> <command>VACUUM FULL</command> and <command>CLUSTER</command> rewrite
> the table, while regular <command>VACUUM</command> only modifies it in
> place.

Fixed.


> +       Current processing command: CLUSTER/VACUUM FULL.
> 
> The command that is running.  Either CLUSTER or VACUUM FULL.

Fixed.


> +       Current processing phase of cluster/vacuum full.  See <xref
> linkend='cluster-phases'> or <xref linkend='vacuum-full-phases'>.
> 
> Current processing phase of CLUSTER or VACUUM FULL.
> 
> Or maybe better, just abbreviate to: Current processing phase.

Fixed as you suggested.


> +       Scan method of table: index scan/seq scan.
> 
> Eh, shouldn't this be gone now?  And likewise for the view definition?

Fixed. Sorry, It was an oversight.


> +       OID of the index.
> 
> If the table is being scanned using an index, this is the OID of the
> index being used; otherwise, it is zero.

Fixed.


> +     <entry><structfield>heap_tuples_total</structfield></entry>
> 
> Leftovers.  Skipping over the rest of your documentation changes since
> it looks like a bunch of things there still need to be updated.

I agree. Thanks a lot!
I'll divide the patch into two patch such as code and document.


> + pgstat_progress_start_command(PROGRESS_COMMAND_CLUSTER, tableOid);
> 
> This now appears inside cluster_rel(), but also vacuum_rel() is still
> doing the same thing.  That's wrong.

It was an oversight too. I fixed.


> + if(OidIsValid(indexOid))
> 
> Missing space.  Please pgindent.

Fixed.
I Will do pgindent later.


Please find attached files. :)

Thanks,
Tatsuro Yamada