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