Re: [HACKERS] ANALYZE command progress checker

[vinayak]

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.paqu...@gmail.com <mailto:michael.paqu...@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);


are indexes on the table.

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

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

pg_stat_progress_analyze_v2.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers