Re: ANALYZE command progress checker

On Tue, Mar 7, 2017 at 5:01 PM, Michael Paquier <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.
>
> + <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.
>
> @@ -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().

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.

+ /* 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.

+ /* Report total number of heap blocks and collectinf sample row phase*/

Typo? collecting?

+ /* 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.

+#define PROGRESS_ANALYZE_PHASE_COLLECT_INH_SAMPLE_ROWS 2

why there is no code added for the phase, any specific reason?

+/* 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) */

Regards,
Hari Babu
Fujitsu Australia