Re: [HACKERS] ANALYZE command progress checker - Mailing list pgsql-hackers

On Mon, Mar 6, 2017 at 3:58 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2017-03-03 15:33:15 -0500, David Steele wrote:
>> On 3/1/17 1:25 PM, Andres Freund wrote:
>> > On 2017-03-01 10:20:41 -0800, David Fetter wrote:
>> >> On Wed, Mar 01, 2017 at 09:45:40AM -0500, Peter Eisentraut wrote:
>> >>> On 2/28/17 04:24, vinayak wrote:
>> >>>> The view provides the information of analyze command progress details as
>> >>>> follows
>> >>>> postgres=# \d pg_stat_progress_analyze
>> >>>>            View "pg_catalog.pg_stat_progress_analyze"
>> >>>
>> >>> Hmm, do we want a separate "progress" system view for every kind of
>> >>> command?  What if someone comes up with a progress checker for CREATE
>> >>> INDEX, REINDEX, CLUSTER, etc.?
>> >
>> > I don't think that'd be that bad, otherwise the naming of the fields is
>> > complicated.  I guess the alternative (or do both?) would be to to have
>> > a pivoted table, but that'd painful to query.  Do you have a better idea?
>> >
>> >
>> >> Some kind of design for progress seems like a good plan.  Some ideas:
>> >>
>> >> - System view(s)
>> >>
>> >>     This has the advantage of being shown to work at least to a PoC by
>> >>     this patch, and is similar to extant system views like
>> >>     pg_stat_activity in the sense of capturing a moment in time.
>> >>
>> >> - NOTIFY
>> >>
>> >>     Event-driven model as opposed to a polling one.  This is
>> >>     attractive on efficiency grounds, less so on reliability ones.
>> >>
>> >> - Something added to the wire protocol
>> >>
>> >>     More specialized, limits the information to the session where the
>> >>     command was issued
>> >>
>> >> - Other things not named here
>> >
>> > We now have a framework for this [1] (currently used by vacuum, but
>> > extensible). The question is about presentation.  I'm fairly sure that
>> > we shouldn't just add yet another framework, and I doubt that that's
>> > what's proposed by Peter.
>>
>> I think the idea of a general progress view is very valuable and there
>> are a ton of operations it could be used for:  full table scans, index
>> rebuilds, vacuum, copy, etc.
>> However, I feel that this proposal is not flexible enough and comes too
>> late in the release cycle to allow development into something that could
>> be committed.
>
> I'm not following. As I pointed out, we already have this framework?
> This patch is just a short one using that framework?
>
>
>> I propose we move this to the 2017-07 CF so the idea can be more fully
>> developed.
>
> I don't see that being warranted in this case, we're really not talking
> about something complicated:
>  doc/src/sgml/monitoring.sgml         |  135 +++++++++++++++++++++++++++++++++++
>  src/backend/catalog/system_views.sql |   16 ++++
>  src/backend/commands/analyze.c       |   34 ++++++++
>  src/backend/utils/adt/pgstatfuncs.c  |    2
>  src/include/commands/progress.h      |   13 +++
>  src/include/pgstat.h                 |    3
>  src/test/regress/expected/rules.out  |   18 ++++
>  7 files changed, 219 insertions(+), 2 deletions(-)
> excepting tests and docs, this is very little actual code.

Or 35 lines just for the backend portion, it is hard to something smaller.

@@ -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().
-- 
Michael