Re: ANALYZE command progress checker

Thank you for reviewing the patch.

The attached patch incorporated Michael and Amit comments also.

On 2017/03/07 15:45, Haribabu Kommi wrote:
>
>
> On Tue, Mar 7, 2017 at 5:01 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com <mailto:michael(dot)paquier(at)gmail(dot)com>> wrote:
>
>
> @@ -496,7 +499,6 @@ do_analyze_rel(Relation onerel, int options,
> VacuumParams *params,
> numrows = (*acquirefunc) (onerel, elevel,
> rows, targrows,
> &totalrows, &totaldeadrows);
> -
> /*
> Useless diff.
>
Fixed.
>
> + <entry>
> + <command>ANALYZE</> is currently collecting the sample rows.
> + The sample it reads is taken randomly.Its size depends on
> + the default_statistics_target parameter value.
> + </entry>
> This should use a <varname> markup for default_statistics_target.
>
Fixed.
>
>
> @@ -203,6 +204,8 @@ analyze_rel(Oid relid, RangeVar *relation, int
> options,
> if (onerel->rd_rel->relkind == RELKIND_RELATION ||
> onerel->rd_rel->relkind == RELKIND_MATVIEW)
> {
> + pgstat_progress_start_command(PROGRESS_COMMAND_ANALYZE,
> + RelationGetRelid(onerel));
> It seems to me that the report should begin in do_analyze_rel().
>
Fixed.
>
> some more comments,
>
> +/* Report compute heap stats phase */
> +pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE,
> +PROGRESS_ANALYZE_PHASE_COMPUTE_HEAP_STATS);
>
> The above analyze phase is updated inside a for loop, instead just set
> it above once.
Fixed.
>
> + /* Report compute index stats phase */
> + pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE,
> + PROGRESS_ANALYZE_PHASE_COMPUTE_INDEX_STATS);
>
> Irrespective of whether the index exists on the table or not, the
> above analyze phase
> is set. It is better to set the above phase and index cleanup phase
> only when there
> are indexes on the table.
>
Agreed. Fixed.
> +/* Report total number of heap blocks and collectinf sample row phase*/
>
> Typo? collecting?
>
Fixed.
>
> +/* Report total number of heap blocks and collectinf sample row phase*/
> +initprog_val[0] = PROGRESS_ANALYZE_PHASE_COLLECT_HEAP_SAMPLE_ROWS;
> +initprog_val[1] = totalblocks;
> +pgstat_progress_update_multi_param(2, initprog_index, initprog_val);
> acquire_sample_rows function is called from acquire_inherited_sample_rows
> function, so adding the phase in that function will provide wrong info.
>
I agree with you.
>
> +#define PROGRESS_ANALYZE_PHASE_COLLECT_INH_SAMPLE_ROWS2
>
> why there is no code added for the phase, any specific reason?
I am thinking how to report this phase. Do you have any suggestion?

> +/* Phases of analyze */
>
> Can be written as following for better understanding, and also
> similar like vacuum.
>
> /* Phases of analyze (as advertised via PROGRESS_ANALYZE_PHASE) */
>
Done.

Regards,
Vinayak Pokale
NTT Open Source Software Center