Re: ANALYZE command progress checker

From: vinayak <Pokale_Vinayak_q3(at)lab(dot)ntt(dot)co(dot)jp>
To: Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>
Cc: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, David Steele <david(at)pgmasters(dot)net>, David Fetter <david(at)fetter(dot)org>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: ANALYZE command progress checker
Date: 2017-03-10 07:46:42
Message-ID: 0f32300a-685f-d12f-09a7-db9bf5d9e634@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
pg_stat_progress_analyze_v2.patch binary/octet-stream 12.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Jim Nasby 2017-03-10 07:56:40 Re: REINDEX CONCURRENTLY 2.0
Previous Message tushar 2017-03-10 07:39:16 Re: increasing the default WAL segment size