Re: [HACKERS] ANALYZE command progress checker

Thank you for reviewing the patch.

The attached patch incorporated Michael and Amit comments also.

On Tue, Mar 7, 2017 at 5:01 PM, Michael Paquier <michael.paquier@gmail.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_ROWS 2

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