Thread: [HACKERS] ANALYZE command progress checker
Hello Hackers,
Following is a proposal for reporting the progress of ANALYZE command:
It seems that the following could be the phases of ANALYZE processing:
1. Collecting sample rows
2. Collecting inherited sample rows
3. Computing heap stats
4. Computing index stats
5. Cleaning up indexes
The first phase is easy if there is no inheritance but in case of
inheritance we need to sample the blocks from multiple heaps.
Here the progress is counted against total number of blocks processed.
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"
Column | Type | Collation | Nullable | Default
-------------------+---------+-----------+----------+---------
pid | integer | | |
datid | oid | | |
datname | name | | |
relid | oid | | |
phase | text | | |
heap_blks_total | bigint | | |
heap_blks_scanned | bigint | | |
total_sample_rows | bigint | | |
I feel this view information may be useful in checking the progress of
long running ANALYZE command.
The attached patch reports the different phases of analyze command.
Added this patch to CF 2017-03.
Opinions?
Note: Collecting inherited sample rows phase is not reported yet in the
patch.
Regards,
Vinayak Pokale
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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.? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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.? 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 viewslike 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 Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
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. [1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c16dc1aca5e
On 2017-03-01 10:25:49 -0800, Andres Freund wrote: > 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. > > [1] > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c16dc1aca5e Majority of that is actually in https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=b6fb6471f6afaf649e52f38269fd8c5c60647669
On Wed, Mar 01, 2017 at 10:28:23AM -0800, Andres Freund wrote: > On 2017-03-01 10:25:49 -0800, Andres Freund wrote: > > 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. > > > > [1] > > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c16dc1aca5e > > Majority of that is actually in > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=b6fb6471f6afaf649e52f38269fd8c5c60647669 If that's even vaguely usable, I'd say we should use it for this. I notice that that commit has no SGML component. Should it have one? Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On March 1, 2017 11:34:48 AM PST, David Fetter <david@fetter.org> wrote: >I notice that that commit has no SGML component. Should it have one? Don't think so. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
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 propose we move this to the 2017-07 CF so the idea can be more fully developed. -- -David david@pgmasters.net
On Sat, Mar 4, 2017 at 5:33 AM, David Steele <david@pgmasters.net> wrote: > 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. Well, each command really has its own requirements in terms of data to store, so we either finish with a bunch of small tables that anyone could query and join as they wish or a somewhat unique table that is bloated with all the information, with a set of views on top of it to query all the information. For extensibility's sake of each command (for example imagine that REINDEX could be extended with a CONCURRENTLY option and multiple phases), I would think that having a table per command type would not be that bad. -- Michael
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. Greetings, Andres Freund
Hi Vinayak, On 2017/02/28 18:24, vinayak wrote: > The attached patch reports the different phases of analyze command. > Added this patch to CF 2017-03. In the updated monitoring.sgml: + <row> + <entry><literal>computing heap stats</literal></entry> + <entry> + <command>VACUUM</> is currently computing heap stats. + </entry> + </row> + <row> + <entry><literal>computing index stats</literal></entry> + <entry> + <command>VACUUM</> is currently computing index stats. + </entry> + </row> + <row> + <entry><literal>cleaning up indexes</literal></entry> + <entry> + <command>ANALYZE</> is currently cleaning up indexes. + </entry> + </row> + </tbody> + </tgroup> + </table> The entries mentioning VACUUM should actually say ANALYZE. Thanks, Amit
On 2017/03/06 17:02, Amit Langote wrote: > Hi Vinayak, > > On 2017/02/28 18:24, vinayak wrote: >> The attached patch reports the different phases of analyze command. >> Added this patch to CF 2017-03. > In the updated monitoring.sgml: > > + <row> > + <entry><literal>computing heap stats</literal></entry> > + <entry> > + <command>VACUUM</> is currently computing heap stats. > + </entry> > + </row> > + <row> > + <entry><literal>computing index stats</literal></entry> > + <entry> > + <command>VACUUM</> is currently computing index stats. > + </entry> > + </row> > + <row> > + <entry><literal>cleaning up indexes</literal></entry> > + <entry> > + <command>ANALYZE</> is currently cleaning up indexes. > + </entry> > + </row> > + </tbody> > + </tgroup> > + </table> > > The entries mentioning VACUUM should actually say ANALYZE. Yes. Thank you. I will fix it. Regards, Vinayak Pokale NTT Open Source Software Center
On 3/6/17 1:58 AM, Andres Freund wrote: > On 2017-03-03 15:33:15 -0500, David Steele wrote: > >> 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: <...> > excepting tests and docs, this is very little actual code. Fair enough. From my read through it appeared a redesign was required/requested. -- -David david@pgmasters.net
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
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.
+ <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
On Wed, Mar 1, 2017 at 1:25 PM, Andres Freund <andres@anarazel.de> 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. +1. I suppose if it gets really out of control we might have to rethink, but 2 is not 50, and having appropriate column names is worth a lot. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Thank you for reviewing the patch.
The attached patch incorporated Michael and Amit comments also.
On 2017/03/07 15:45, Haribabu Kommi wrote:
Fixed.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.
Agreed. 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 phaseis set. It is better to set the above phase and index cleanup phase only when thereare indexes on the table.
Fixed.+ /* 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);
I agree with you.acquire_sample_rows function is called from acquire_inherited_sample_rowsfunction, so adding the phase in that function will provide wrong info.
I am thinking how to report this phase. Do you have any suggestion?+#define PROGRESS_ANALYZE_PHASE_COLLECT_INH_SAMPLE_ROWS 2why there is no code added for the phase, any specific reason?
Done.+/* Phases of analyze */Can be written as following for better understanding, and alsosimilar like vacuum./* Phases of analyze (as advertised via PROGRESS_ANALYZE_PHASE) */
Regards,
Vinayak Pokale
NTT Open Source Software Center
Attachment
On 3/6/17 12:49 AM, Michael Paquier wrote: > On Sat, Mar 4, 2017 at 5:33 AM, David Steele <david@pgmasters.net> wrote: >> 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. > > Well, each command really has its own requirements in terms of data to > store, so we either finish with a bunch of small tables that anyone > could query and join as they wish or a somewhat unique table that is > bloated with all the information, with a set of views on top of it to > query all the information. For extensibility's sake of each command > (for example imagine that REINDEX could be extended with a > CONCURRENTLY option and multiple phases), I would think that having a > table per command type would not be that bad. Well, the ideal scenario is that someone uses the raw data to come up with a good way to just provide ye olde 0-100% progress bar. At that point a single view would do the trick. Perhaps instead of adding more clutter to \dvS we could just have a SRF for now. At over 2800 rows currently, you're not going to notice one more addition to \dfS. -- Jim Nasby, Chief Data Architect, OpenSCG http://OpenSCG.com
Hi, On 2017-03-10 02:11:18 -0600, Jim Nasby wrote: > Perhaps instead of adding more clutter to \dvS we could just have a SRF for > now. I don't see that as clutter, it's useful information, and keeping it discoverable is good, not bad. > At over 2800 rows currently, you're not going to notice one more > addition to \dfS. I think it's hard to design a good SRF for this. Because the fields for different types of progress are different / empty, you can't just trivially return them as rows. You'd have to do some EAV like 'command, field_name1, field_value1, ...' type of thing - not particularly pretty / easy to use. Greetings, Andres Freund
On 3/10/17 1:06 PM, Andres Freund wrote: > Hi, > > On 2017-03-10 02:11:18 -0600, Jim Nasby wrote: >> Perhaps instead of adding more clutter to \dvS we could just have a SRF for >> now. > > I don't see that as clutter, it's useful information, and keeping it > discoverable is good, not bad. If we keep adding status reporting commands at some point it's going to get unwieldy. Though, if they were in their own schema... >> At over 2800 rows currently, you're not going to notice one more >> addition to \dfS. > > I think it's hard to design a good SRF for this. Because the fields for > different types of progress are different / empty, you can't just > trivially return them as rows. You'd have to do some EAV like > 'command, field_name1, field_value1, ...' type of thing - not > particularly pretty / easy to use. Oh, I wasn't suggesting a single SRF for everything. Hopefully users will eventually figure out a good formula to drive a "progress bar" for each type of monitor, which is what you really want anyway (at least 99% of the time). If we got there we could have a single view that gave the % complete for every command that was providing feedback. If someone wanted details they could hit the individual SRF. -- Jim Nasby, Chief Data Architect, OpenSCG http://OpenSCG.com
On Fri, Mar 10, 2017 at 2:57 PM, Jim Nasby <jim.nasby@openscg.com> wrote: > Oh, I wasn't suggesting a single SRF for everything. Hopefully users will > eventually figure out a good formula to drive a "progress bar" for each type > of monitor, which is what you really want anyway (at least 99% of the time). > If we got there we could have a single view that gave the % complete for > every command that was providing feedback. If someone wanted details they > could hit the individual SRF. So, the point, at least with the VACUUM progress indicator and perhaps here also, is that we really can't just give a single measure of progress -- we are 58.32% done. That's almost impossible to estimate accurately. For example, consider VACUUM. maintenance_work_mem is 69% used, and we are 74% done vacuuming the table. What percentage done are we? Well, it depends. If the latter part of the table has exactly the same density of dead tuples we've already witnessed, we will need only one index vac pass, but there is a good chance that there's more activity toward the tail end of the table and we will need two index vac passes. If the table has several large indexes, that's a big difference, so if we guess wrong about whether we're going to run out of memory, we will be way off in estimating overall progress. That's why the vacuum progress checker reports the facts we actually know -- like how many blocks of the relation we've scanned, and how many dead tuples we've accumulated so far, and how many dead tuples we are able to store before triggering index vacuuming -- rather than just a percentage. You can try to derive a percentage from that if you want, and you can use any formula you like to derive that, but what the system reports are straight-up facts, not predictions. I don't really want to get into the prediction business. The in-core progress reporting structure can spit out enough data for people to write queries or tools that attempt to predict stuff, and *maybe* someday somebody will write one of those tools that is enough unlike https://xkcd.com/612/ that we'll feel moved to put it in core. But my guess is most likely not. It's easy to get the easy cases right in such a prediction tool, but AFAICS getting the hard cases right requires a crystal ball. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Mar 10, 2017 at 6:46 PM, vinayak <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote:
+ /* 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); I agree with you.acquire_sample_rows function is called from acquire_inherited_sample_rowsfunction, so adding the phase in that function will provide wrong info.I am thinking how to report this phase. Do you have any suggestion?+#define PROGRESS_ANALYZE_PHASE_COLLECT_INH_SAMPLE_ROWS 2 why there is no code added for the phase, any specific reason?
Thanks for the update patch.
Actually the analyze operation is done in two stages for the relation that
contains child relations,
1. For parent relation
2. All child relations
So the progress with start each time for the above two stages. This needs
to clearly mentioned in the docs.
The acquire_sample_rows function collects statistics of the relation
that is provided, updating the analyze details like number of rows and etc
works fine for a single relation, but if it contains many child relations, the
data will be misleading.
Apart from the above, some more comments.
+ /* Report compute index stats phase */
+ pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE,
+ PROGRESS_ANALYZE_PHASE_COMPUTE_INDEX_STATS);
The above block of the code should be set when it actually doing the
compute_index_stats.
+ /* Report that we are now doing index cleanup */
+ pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE,
+ PROGRESS_ANALYZE_PHASE_INDEX_CLEANUP);
The above code block should be inside if (!(options & VACOPT_VACUUM)) block.
Regards,
Hari Babu
Fujitsu Australia
On Fri, Mar 10, 2017 at 2:46 AM, vinayak <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote: > Thank you for reviewing the patch. > > The attached patch incorporated Michael and Amit comments also. I reviewed this tonight. + /* Report compute index stats phase */ + pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE, + PROGRESS_ANALYZE_PHASE_COMPUTE_INDEX_STATS); Hmm, is there really any point to this? And is it even accurate? It doesn't look to me like we are computing any index statistics here; we're only allocating a few in-memory data structures here, I think, which is not a statistics computation and probably doesn't take any significant time. + /* Report compute heap stats phase */ + pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE, + PROGRESS_ANALYZE_PHASE_COMPUTE_HEAP_STATS); The phase you label as computing heap statistics also includes the computation of index statistics. I wouldn't bother separating the computation of heap and index statistics; I'd just have a "compute statistics" phase that begins right after the comment that starts "Compute the statistics." + /* Report that we are now doing index cleanup */ + pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE, + PROGRESS_ANALYZE_PHASE_INDEX_CLEANUP); OK, this doesn't make any sense either. We are not cleaning up the indexes here. We are just closing them and releasing resources. I don't see why you need this phase at all; it can't last more than some tiny fraction of a second. It seems like you're trying to copy the exact same phases that exist for vacuum instead of thinking about what makes sense for ANALYZE. + /* Report number of heap blocks scaned so far*/ + pgstat_progress_update_param(PROGRESS_ANALYZE_HEAP_BLKS_SCANNED, targblock); While I don't think this is necessarily a bad thing to report, I find it very surprising that you're not reporting what seems to me like the most useful thing here - namely, the number of blocks that will be sampled in total and the number of those that you've selected. Instead, you're just reporting the length of the relation and the last-sampled block, which is a less-accurate guide to total progress. There are enough counters to report both things, so maybe we should. + /* Report total number of sample rows*/ + pgstat_progress_update_param(PROGRESS_ANALYZE_TOTAL_SAMPLE_ROWS, numrows); As an alternative to the previous paragraph, yet another thing you could report is number of rows sampled out of total rows to be sampled. But this isn't the way to do it: you are reporting the number of rows you sampled only once you've finished sampling. The point of progress reporting is to report progress -- that is, to report this value as it's updated, not just to report it when ANALYZE is practically done and the value has reached its maximum. The documentation for this patch isn't very good, either. You forgot to update the part of the documentation that says that VACUUM is the only command that currently supports progress reporting, and the descriptions of the phases and progress counters are brief and not entirely accurate - e.g. heap_blks_scanned is not actually the number of heap blocks scanned, because we are sampling, not reading all the blocks. When adding calls to the progress reporting functions, please consider whether a blank line should be added before or after the new code, or both, or neither. I'd say some blank lines are needed in a few places where you didn't add them. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2017/03/17 10:38, Robert Haas wrote:
> On Fri, Mar 10, 2017 at 2:46 AM, vinayak
> <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote:
>> Thank you for reviewing the patch.
>>
>> The attached patch incorporated Michael and Amit comments also.
> I reviewed this tonight.
Thank you for reviewing the patch.
> + /* Report compute index stats phase */
> + pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE,
> +
> PROGRESS_ANALYZE_PHASE_COMPUTE_INDEX_STATS);
>
> Hmm, is there really any point to this? And is it even accurate? It
> doesn't look to me like we are computing any index statistics here;
> we're only allocating a few in-memory data structures here, I think,
> which is not a statistics computation and probably doesn't take any
> significant time.
>
> + /* Report compute heap stats phase */
> + pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE,
> + PROGRESS_ANALYZE_PHASE_COMPUTE_HEAP_STATS);
>
> The phase you label as computing heap statistics also includes the
> computation of index statistics. I wouldn't bother separating the
> computation of heap and index statistics; I'd just have a "compute
> statistics" phase that begins right after the comment that starts
> "Compute the statistics."
Understood. Fixed in the attached patch.
>
> + /* Report that we are now doing index cleanup */
> + pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE,
> + PROGRESS_ANALYZE_PHASE_INDEX_CLEANUP);
>
> OK, this doesn't make any sense either. We are not cleaning up the
> indexes here. We are just closing them and releasing resources. I
> don't see why you need this phase at all; it can't last more than some
> tiny fraction of a second. It seems like you're trying to copy the
> exact same phases that exist for vacuum instead of thinking about what
> makes sense for ANALYZE.
Understood. I have removed this phase.
>
> + /* Report number of heap blocks scaned so far*/
> + pgstat_progress_update_param(PROGRESS_ANALYZE_HEAP_BLKS_SCANNED,
> targblock);
>
> While I don't think this is necessarily a bad thing to report, I find
> it very surprising that you're not reporting what seems to me like the
> most useful thing here - namely, the number of blocks that will be
> sampled in total and the number of those that you've selected.
> Instead, you're just reporting the length of the relation and the
> last-sampled block, which is a less-accurate guide to total progress.
> There are enough counters to report both things, so maybe we should.
>
> + /* Report total number of sample rows*/
> + pgstat_progress_update_param(PROGRESS_ANALYZE_TOTAL_SAMPLE_ROWS, numrows);
>
> As an alternative to the previous paragraph, yet another thing you
> could report is number of rows sampled out of total rows to be
> sampled. But this isn't the way to do it: you are reporting the
> number of rows you sampled only once you've finished sampling. The
> point of progress reporting is to report progress -- that is, to
> report this value as it's updated, not just to report it when ANALYZE
> is practically done and the value has reached its maximum.
Understood.
I have reported number of rows sampled out of total rows to be sampled.
I have not reported the number of blocks that will be sampled in total.
So currect pg_stat_progress_analyze view looks like:
=# \d+ pg_stat_progress_analyze
View "pg_catalog.pg_stat_progress_analyze"
Column | Type | Collation | Nullable | Default |
Storage | Descripti
on
------------------------+---------+-----------+----------+---------+----------+----------
---
pid | integer | | | |
plain |
datid | oid | | | |
plain |
datname | name | | | |
plain |
relid | oid | | | |
plain |
phase | text | | | |
extended |
num_target_sample_rows | bigint | | | |
plain |
num_rows_sampled | bigint | | | |
plain |
View definition:
SELECT s.pid,
s.datid,
d.datname,
s.relid,
CASE s.param1
WHEN 0 THEN 'initializing'::text
WHEN 1 THEN 'collecting sample rows'::text
WHEN 2 THEN 'computing statistics'::text
ELSE NULL::text
END AS phase,
s.param2 AS num_target_sample_rows,
s.param3 AS num_rows_sampled
FROM pg_stat_get_progress_info('ANALYZE'::text) s(pid, datid, relid,
param1, param2, p
aram3, param4, param5, param6, param7, param8, param9, param10)
LEFT JOIN pg_database d ON s.datid = d.oid;
Comment?
> The documentation for this patch isn't very good, either. You forgot
> to update the part of the documentation that says that VACUUM is the
> only command that currently supports progress reporting, and the
> descriptions of the phases and progress counters are brief and not
> entirely accurate - e.g. heap_blks_scanned is not actually the number
> of heap blocks scanned, because we are sampling, not reading all the
> blocks.
Understood. I have updated the documentation.
I will also try to improve documentation.
> When adding calls to the progress reporting functions, please consider
> whether a blank line should be added before or after the new code, or
> both, or neither. I'd say some blank lines are needed in a few places
> where you didn't add them.
+1. I have added blank lines in a few places.
Regards,
Vinayak Pokale
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Hi Vinayak, Here are couple of review comments that may need your attention. 1. Firstly, I am seeing some trailing whitespace errors when trying to apply your v3 patch using git apply command. [ashu@localhost postgresql]$ git apply pg_stat_progress_analyze_v3.patch pg_stat_progress_analyze_v3.patch:155: trailing whitespace. CREATE VIEW pg_stat_progress_analyze AS pg_stat_progress_analyze_v3.patch:156: trailing whitespace. SELECT pg_stat_progress_analyze_v3.patch:157: trailing whitespace. S.pid AS pid, S.datid AS datid, D.datname AS datname, pg_stat_progress_analyze_v3.patch:158: trailing whitespace. S.relid AS relid, pg_stat_progress_analyze_v3.patch:159: trailing whitespace. CASE S.param1 WHEN 0 THEN 'initializing' error: patch failed: doc/src/sgml/monitoring.sgml:521 2) The test-case 'rules' is failing. I think it's failing because in rules.sql 'ORDERBY viewname' is used which will put 'pg_stat_progress_analyze' ahead of 'pg_stat_progress_vacuum' in the list of catalog views. You may need to correct rules.out file accordingly. This is what i could see in rules.sql, SELECT viewname, definition FROM pg_views WHERE schemaname <> 'information_schema' ORDER BY viewname; I am still reviewing your patch and doing some testing. Will update if i find any issues. Thanks. -- With Regards, Ashutosh Sharma EnterpriseDB:http://www.enterprisedb.com On Fri, Mar 17, 2017 at 3:16 PM, vinayak <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote: > > On 2017/03/17 10:38, Robert Haas wrote: >> >> On Fri, Mar 10, 2017 at 2:46 AM, vinayak >> <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote: >>> >>> Thank you for reviewing the patch. >>> >>> The attached patch incorporated Michael and Amit comments also. >> >> I reviewed this tonight. > > Thank you for reviewing the patch. >> >> + /* Report compute index stats phase */ >> + pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE, >> + >> PROGRESS_ANALYZE_PHASE_COMPUTE_INDEX_STATS); >> >> Hmm, is there really any point to this? And is it even accurate? It >> doesn't look to me like we are computing any index statistics here; >> we're only allocating a few in-memory data structures here, I think, >> which is not a statistics computation and probably doesn't take any >> significant time. >> >> + /* Report compute heap stats phase */ >> + pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE, >> + >> PROGRESS_ANALYZE_PHASE_COMPUTE_HEAP_STATS); >> >> The phase you label as computing heap statistics also includes the >> computation of index statistics. I wouldn't bother separating the >> computation of heap and index statistics; I'd just have a "compute >> statistics" phase that begins right after the comment that starts >> "Compute the statistics." > > Understood. Fixed in the attached patch. >> >> >> + /* Report that we are now doing index cleanup */ >> + pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE, >> + PROGRESS_ANALYZE_PHASE_INDEX_CLEANUP); >> >> OK, this doesn't make any sense either. We are not cleaning up the >> indexes here. We are just closing them and releasing resources. I >> don't see why you need this phase at all; it can't last more than some >> tiny fraction of a second. It seems like you're trying to copy the >> exact same phases that exist for vacuum instead of thinking about what >> makes sense for ANALYZE. > > Understood. I have removed this phase. >> >> >> + /* Report number of heap blocks scaned so far*/ >> + pgstat_progress_update_param(PROGRESS_ANALYZE_HEAP_BLKS_SCANNED, >> targblock); >> >> While I don't think this is necessarily a bad thing to report, I find >> it very surprising that you're not reporting what seems to me like the >> most useful thing here - namely, the number of blocks that will be >> sampled in total and the number of those that you've selected. >> Instead, you're just reporting the length of the relation and the >> last-sampled block, which is a less-accurate guide to total progress. >> There are enough counters to report both things, so maybe we should. >> >> + /* Report total number of sample rows*/ >> + pgstat_progress_update_param(PROGRESS_ANALYZE_TOTAL_SAMPLE_ROWS, >> numrows); >> >> As an alternative to the previous paragraph, yet another thing you >> could report is number of rows sampled out of total rows to be >> sampled. But this isn't the way to do it: you are reporting the >> number of rows you sampled only once you've finished sampling. The >> point of progress reporting is to report progress -- that is, to >> report this value as it's updated, not just to report it when ANALYZE >> is practically done and the value has reached its maximum. > > Understood. > I have reported number of rows sampled out of total rows to be sampled. > I have not reported the number of blocks that will be sampled in total. > So currect pg_stat_progress_analyze view looks like: > > =# \d+ pg_stat_progress_analyze > View "pg_catalog.pg_stat_progress_analyze" > Column | Type | Collation | Nullable | Default | Storage > | Descripti > on > ------------------------+---------+-----------+----------+---------+----------+---------- > --- > pid | integer | | | | plain > | > datid | oid | | | | plain > | > datname | name | | | | plain > | > relid | oid | | | | plain > | > phase | text | | | | > extended | > num_target_sample_rows | bigint | | | | plain > | > num_rows_sampled | bigint | | | | plain > | > View definition: > SELECT s.pid, > s.datid, > d.datname, > s.relid, > CASE s.param1 > WHEN 0 THEN 'initializing'::text > WHEN 1 THEN 'collecting sample rows'::text > WHEN 2 THEN 'computing statistics'::text > ELSE NULL::text > END AS phase, > s.param2 AS num_target_sample_rows, > s.param3 AS num_rows_sampled > FROM pg_stat_get_progress_info('ANALYZE'::text) s(pid, datid, relid, > param1, param2, p > aram3, param4, param5, param6, param7, param8, param9, param10) > LEFT JOIN pg_database d ON s.datid = d.oid; > > Comment? >> >> The documentation for this patch isn't very good, either. You forgot >> to update the part of the documentation that says that VACUUM is the >> only command that currently supports progress reporting, and the >> descriptions of the phases and progress counters are brief and not >> entirely accurate - e.g. heap_blks_scanned is not actually the number >> of heap blocks scanned, because we are sampling, not reading all the >> blocks. > > Understood. I have updated the documentation. > I will also try to improve documentation. >> >> When adding calls to the progress reporting functions, please consider >> whether a blank line should be added before or after the new code, or >> both, or neither. I'd say some blank lines are needed in a few places >> where you didn't add them. > > +1. I have added blank lines in a few places. > > Regards, > Vinayak Pokale > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Hi, I didn't find any major issues with the patch. It works as expected. However, I have few minor comments that I would like to share, + <entry> + Total number of sample rows. The sample it reads is taken randomly. + Its size depends on the <varname>default_statistics_target</> parameter value. + </entry> 1) I think it would be better if you could specify reference link to the guc variable 'default_statistics_target'. Something like this, <xref linkend='guc-default-statistics-target'>. If you go through monitoring.sgml, you would find that there is reference link to all guc variables being used. 2) I feel that the 'computing_statistics' phase could have been splitted into 'computing_column_stats', 'computing_index_stats'.... Please let me know your thoughts on this. + certain commands during command execution. Currently, the command + which supports progress reporting is <command>VACUUM</> and <command>ANALYZE</>. This may be expanded in the future. 3) I think above needs to be rephrased. Something like...Currently, the supported progress reporting commands are 'VACUUM' and 'ANALYZE'. Moreover, I also ran your patch on Windows platform and didn't find any issues. Thanks. -- With Regards, Ashutosh Sharma EnterpriseDB:http://www.enterprisedb.com On Sat, Mar 18, 2017 at 5:30 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > Hi Vinayak, > > Here are couple of review comments that may need your attention. > > 1. Firstly, I am seeing some trailing whitespace errors when trying to > apply your v3 patch using git apply command. > > [ashu@localhost postgresql]$ git apply pg_stat_progress_analyze_v3.patch > pg_stat_progress_analyze_v3.patch:155: trailing whitespace. > CREATE VIEW pg_stat_progress_analyze AS > pg_stat_progress_analyze_v3.patch:156: trailing whitespace. > SELECT > pg_stat_progress_analyze_v3.patch:157: trailing whitespace. > S.pid AS pid, S.datid AS datid, D.datname AS datname, > pg_stat_progress_analyze_v3.patch:158: trailing whitespace. > S.relid AS relid, > pg_stat_progress_analyze_v3.patch:159: trailing whitespace. > CASE S.param1 WHEN 0 THEN 'initializing' > error: patch failed: doc/src/sgml/monitoring.sgml:521 > > 2) The test-case 'rules' is failing. I think it's failing because in > rules.sql 'ORDERBY viewname' is used which will put > 'pg_stat_progress_analyze' ahead of 'pg_stat_progress_vacuum' in the > list of catalog views. You may need to correct rules.out file > accordingly. This is what i could see in rules.sql, > > SELECT viewname, definition FROM pg_views WHERE schemaname <> > 'information_schema' ORDER BY viewname; > > I am still reviewing your patch and doing some testing. Will update if > i find any issues. Thanks. > > -- > With Regards, > Ashutosh Sharma > EnterpriseDB:http://www.enterprisedb.com > > On Fri, Mar 17, 2017 at 3:16 PM, vinayak > <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote: >> >> On 2017/03/17 10:38, Robert Haas wrote: >>> >>> On Fri, Mar 10, 2017 at 2:46 AM, vinayak >>> <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote: >>>> >>>> Thank you for reviewing the patch. >>>> >>>> The attached patch incorporated Michael and Amit comments also. >>> >>> I reviewed this tonight. >> >> Thank you for reviewing the patch. >>> >>> + /* Report compute index stats phase */ >>> + pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE, >>> + >>> PROGRESS_ANALYZE_PHASE_COMPUTE_INDEX_STATS); >>> >>> Hmm, is there really any point to this? And is it even accurate? It >>> doesn't look to me like we are computing any index statistics here; >>> we're only allocating a few in-memory data structures here, I think, >>> which is not a statistics computation and probably doesn't take any >>> significant time. >>> >>> + /* Report compute heap stats phase */ >>> + pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE, >>> + >>> PROGRESS_ANALYZE_PHASE_COMPUTE_HEAP_STATS); >>> >>> The phase you label as computing heap statistics also includes the >>> computation of index statistics. I wouldn't bother separating the >>> computation of heap and index statistics; I'd just have a "compute >>> statistics" phase that begins right after the comment that starts >>> "Compute the statistics." >> >> Understood. Fixed in the attached patch. >>> >>> >>> + /* Report that we are now doing index cleanup */ >>> + pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE, >>> + PROGRESS_ANALYZE_PHASE_INDEX_CLEANUP); >>> >>> OK, this doesn't make any sense either. We are not cleaning up the >>> indexes here. We are just closing them and releasing resources. I >>> don't see why you need this phase at all; it can't last more than some >>> tiny fraction of a second. It seems like you're trying to copy the >>> exact same phases that exist for vacuum instead of thinking about what >>> makes sense for ANALYZE. >> >> Understood. I have removed this phase. >>> >>> >>> + /* Report number of heap blocks scaned so far*/ >>> + pgstat_progress_update_param(PROGRESS_ANALYZE_HEAP_BLKS_SCANNED, >>> targblock); >>> >>> While I don't think this is necessarily a bad thing to report, I find >>> it very surprising that you're not reporting what seems to me like the >>> most useful thing here - namely, the number of blocks that will be >>> sampled in total and the number of those that you've selected. >>> Instead, you're just reporting the length of the relation and the >>> last-sampled block, which is a less-accurate guide to total progress. >>> There are enough counters to report both things, so maybe we should. >>> >>> + /* Report total number of sample rows*/ >>> + pgstat_progress_update_param(PROGRESS_ANALYZE_TOTAL_SAMPLE_ROWS, >>> numrows); >>> >>> As an alternative to the previous paragraph, yet another thing you >>> could report is number of rows sampled out of total rows to be >>> sampled. But this isn't the way to do it: you are reporting the >>> number of rows you sampled only once you've finished sampling. The >>> point of progress reporting is to report progress -- that is, to >>> report this value as it's updated, not just to report it when ANALYZE >>> is practically done and the value has reached its maximum. >> >> Understood. >> I have reported number of rows sampled out of total rows to be sampled. >> I have not reported the number of blocks that will be sampled in total. >> So currect pg_stat_progress_analyze view looks like: >> >> =# \d+ pg_stat_progress_analyze >> View "pg_catalog.pg_stat_progress_analyze" >> Column | Type | Collation | Nullable | Default | Storage >> | Descripti >> on >> ------------------------+---------+-----------+----------+---------+----------+---------- >> --- >> pid | integer | | | | plain >> | >> datid | oid | | | | plain >> | >> datname | name | | | | plain >> | >> relid | oid | | | | plain >> | >> phase | text | | | | >> extended | >> num_target_sample_rows | bigint | | | | plain >> | >> num_rows_sampled | bigint | | | | plain >> | >> View definition: >> SELECT s.pid, >> s.datid, >> d.datname, >> s.relid, >> CASE s.param1 >> WHEN 0 THEN 'initializing'::text >> WHEN 1 THEN 'collecting sample rows'::text >> WHEN 2 THEN 'computing statistics'::text >> ELSE NULL::text >> END AS phase, >> s.param2 AS num_target_sample_rows, >> s.param3 AS num_rows_sampled >> FROM pg_stat_get_progress_info('ANALYZE'::text) s(pid, datid, relid, >> param1, param2, p >> aram3, param4, param5, param6, param7, param8, param9, param10) >> LEFT JOIN pg_database d ON s.datid = d.oid; >> >> Comment? >>> >>> The documentation for this patch isn't very good, either. You forgot >>> to update the part of the documentation that says that VACUUM is the >>> only command that currently supports progress reporting, and the >>> descriptions of the phases and progress counters are brief and not >>> entirely accurate - e.g. heap_blks_scanned is not actually the number >>> of heap blocks scanned, because we are sampling, not reading all the >>> blocks. >> >> Understood. I have updated the documentation. >> I will also try to improve documentation. >>> >>> When adding calls to the progress reporting functions, please consider >>> whether a blank line should be added before or after the new code, or >>> both, or neither. I'd say some blank lines are needed in a few places >>> where you didn't add them. >> >> +1. I have added blank lines in a few places. >> >> Regards, >> Vinayak Pokale >> >> >> -- >> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) >> To make changes to your subscription: >> http://www.postgresql.org/mailpref/pgsql-hackers >>
Hi Ashutosh, Thank you for reviewing the patch. On 2017/03/18 21:00, Ashutosh Sharma wrote: > Hi Vinayak, > > Here are couple of review comments that may need your attention. > > 1. Firstly, I am seeing some trailing whitespace errors when trying to > apply your v3 patch using git apply command. > > [ashu@localhost postgresql]$ git apply pg_stat_progress_analyze_v3.patch > pg_stat_progress_analyze_v3.patch:155: trailing whitespace. > CREATE VIEW pg_stat_progress_analyze AS > pg_stat_progress_analyze_v3.patch:156: trailing whitespace. > SELECT > pg_stat_progress_analyze_v3.patch:157: trailing whitespace. > S.pid AS pid, S.datid AS datid, D.datname AS datname, > pg_stat_progress_analyze_v3.patch:158: trailing whitespace. > S.relid AS relid, > pg_stat_progress_analyze_v3.patch:159: trailing whitespace. > CASE S.param1 WHEN 0 THEN 'initializing' > error: patch failed: doc/src/sgml/monitoring.sgml:521 I have tried to apply patch using "git apply" and it works fine in my environment. I use below command to apply the patch. patch -p1 < pg_stat_progress_analyze_v3.patch > 2) The test-case 'rules' is failing. I think it's failing because in > rules.sql 'ORDERBY viewname' is used which will put > 'pg_stat_progress_analyze' ahead of 'pg_stat_progress_vacuum' in the > list of catalog views. You may need to correct rules.out file > accordingly. This is what i could see in rules.sql, > > SELECT viewname, definition FROM pg_views WHERE schemaname <> > 'information_schema' ORDER BY viewname; > > I am still reviewing your patch and doing some testing. Will update if > i find any issues. Thanks. Understood. I have fixed it. Regards, Vinayak Pokale NTT Open Source Software Center
Hi Ashutosh, On 2017/03/19 17:56, Ashutosh Sharma wrote: > Hi, > > I didn't find any major issues with the patch. It works as expected. > However, I have few minor comments that I would like to share, > > + <entry> > + Total number of sample rows. The sample it reads is taken randomly. > + Its size depends on the <varname>default_statistics_target</> > parameter value. > + </entry> > > 1) I think it would be better if you could specify reference link to > the guc variable 'default_statistics_target'. Something like this, > > <xref linkend='guc-default-statistics-target'>. > > If you go through monitoring.sgml, you would find that there is > reference link to all guc variables being used. +1. Updated in the attached patch. > > 2) I feel that the 'computing_statistics' phase could have been > splitted into 'computing_column_stats', 'computing_index_stats'.... > Please let me know your thoughts on this. > Initially I have spitted this phase as 'computing heap stats' and 'computing index stats' but I agreed with Roberts suggestion to merge two phases into one as 'computing statistics'. > + certain commands during command execution. Currently, the command > + which supports progress reporting is <command>VACUUM</> and > <command>ANALYZE</>. This may be > expanded in the future. > > 3) I think above needs to be rephrased. Something like...Currently, > the supported progress reporting commands are 'VACUUM' and > 'ANALYZE'. +1. Updated in the attached patch. > Moreover, I also ran your patch on Windows platform and didn't find > any issues. Thanks. Thank you for testing the patch on Windows platform. Regards, Vinayak Pokale NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Tue, Mar 21, 2017 at 3:41 PM, vinayak <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote:
Thank you for testing the patch on Windows platform.
Thanks for the updated patch.
It works good for a normal relation. But for a relation that contains child tables,
the PROGRESS_ANALYZE_NUM_ROWS_SAMPLED produces wrong results.
How about adding another phase called PROGRESS_ANALYZE_PHASE_COLLECT_INHERIT_SAMPLE_ROWS
and set this phase only when it is an inheritance analyze operation. And adding
some explanation of ROWS_SAMPLED phase about inheritance tables
how these sampled rows are calculated will provide good analyze progress of
relation that contains child relations also.
Regards,
Hari Babu
Fujitsu Australia
On 2017/03/21 21:25, Haribabu Kommi wrote:
Thank you for reviewing the patch.On Tue, Mar 21, 2017 at 3:41 PM, vinayak <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote:Thank you for testing the patch on Windows platform.Thanks for the updated patch.It works good for a normal relation. But for a relation that contains child tables,the PROGRESS_ANALYZE_NUM_ROWS_SAMPLED produces wrong results.
The attached patch implements a way to report sample rows count from acquire_sample_rows() even if called for child tables.
I have added the phase called PROGRESS_ANALYZE_PHASE_COLLECT_INH_SAMPLE_ROWS.How about adding another phase called PROGRESS_ANALYZE_PHASE_COLLECT_INHERIT_SAMPLE_ROWSand set this phase only when it is an inheritance analyze operation. And addingsome explanation of ROWS_SAMPLED phase about inheritance tableshow these sampled rows are calculated will provide good analyze progress ofrelation that contains child relations also.
I have also updated the documentation.
The ANALYZE command takes long time in computing statistics phase.So I think we can add some column or phase so that user can easily understand the progress.
How about adding new column like "num_rows_processed" will compute the statistics of specified column?
How about separate the computing "inheritance statistics" phase from the computing regular "single table" statistics.
Comment?
Regards,
Vinayak Pokale
NTT Open Source Software Center
Attachment
On Wed, Mar 22, 2017 at 8:11 PM, vinayak <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote:
On 2017/03/21 21:25, Haribabu Kommi wrote:Thank you for reviewing the patch.On Tue, Mar 21, 2017 at 3:41 PM, vinayak <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote: Thank you for testing the patch on Windows platform.Thanks for the updated patch.It works good for a normal relation. But for a relation that contains child tables,the PROGRESS_ANALYZE_NUM_ROWS_SAMPLED produces wrong results.
The attached patch implements a way to report sample rows count from acquire_sample_rows() even if called for child tables.I have added the phase called PROGRESS_ANALYZE_PHASE_How about adding another phase called PROGRESS_ANALYZE_PHASE_COLLECT_INHERIT_SAMPLE_ROWS and set this phase only when it is an inheritance analyze operation. And addingsome explanation of ROWS_SAMPLED phase about inheritance tableshow these sampled rows are calculated will provide good analyze progress ofrelation that contains child relations also.COLLECT_INH_SAMPLE_ROWS.
I have also updated the documentation.
Thanks for the updated patch. I will check it.
The ANALYZE command takes long time in computing statistics phase.So I think we can add some column or phase so that user can easily understand the progress.
How about adding new column like "num_rows_processed" will compute the statistics of specified column?
I prefer a column with rows processed instead of a phase.
Because we already set the phase of compute stats and showing
the progress there will number of rows processed will be good.
How about separate the computing "inheritance statistics" phase from the computing regular "single table" statistics.
Comment?
Yes, this will be good to show both that states of inheritance of sampled rows and
compute inheritance stats, so that it will be clearly visible to the user the current
status.
Regards,
Hari Babu
Fujitsu Australia
On 2017/03/23 15:04, Haribabu Kommi wrote:
I have updated the patch.On Wed, Mar 22, 2017 at 8:11 PM, vinayak <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote:
On 2017/03/21 21:25, Haribabu Kommi wrote:Thank you for reviewing the patch.On Tue, Mar 21, 2017 at 3:41 PM, vinayak <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote: Thank you for testing the patch on Windows platform.Thanks for the updated patch.It works good for a normal relation. But for a relation that contains child tables,the PROGRESS_ANALYZE_NUM_ROWS_SAMPLED produces wrong results.
The attached patch implements a way to report sample rows count from acquire_sample_rows() even if called for child tables.I have added the phase called PROGRESS_ANALYZE_PHASE_How about adding another phase called PROGRESS_ANALYZE_PHASE_COLLECT_INHERIT_SAMPLE_ROWS and set this phase only when it is an inheritance analyze operation. And addingsome explanation of ROWS_SAMPLED phase about inheritance tableshow these sampled rows are calculated will provide good analyze progress ofrelation that contains child relations also.COLLECT_INH_SAMPLE_ROWS.
I have also updated the documentation.Thanks for the updated patch. I will check it.The ANALYZE command takes long time in computing statistics phase.So I think we can add some column or phase so that user can easily understand the progress.
How about adding new column like "num_rows_processed" will compute the statistics of specified column?I prefer a column with rows processed instead of a phase.Because we already set the phase of compute stats and showingthe progress there will number of rows processed will be good.How about separate the computing "inheritance statistics" phase from the computing regular "single table" statistics.
Comment?
Yes, this will be good to show both that states of inheritance of sampled rows andcompute inheritance stats, so that it will be clearly visible to the user the currentstatus.
I have added column "num_rows_processed" and phase "computing inherited statistics" in the view.
=# \d+ pg_stat_progress_analyze
View "pg_catalog.pg_stat_progress_analyze"
Column | Type | Collation | Nullable | Default | Storage | Description
------------------------+---------+-----------+----------+---------+----------+-------------
pid | integer | | | | plain |
datid | oid | | | | plain |
datname | name | | | | plain |
relid | oid | | | | plain |
phase | text | | | | extended |
num_target_sample_rows | bigint | | | | plain |
num_rows_sampled | bigint | | | | plain |
num_rows_processed | bigint | | | | plain |
View definition:
SELECT s.pid,
s.datid,
d.datname,
s.relid,
CASE s.param1
WHEN 0 THEN 'initializing'::text
WHEN 1 THEN 'collecting sample rows'::text
WHEN 2 THEN 'collecting inherited sample rows'::text
WHEN 3 THEN 'computing statistics'::text
WHEN 4 THEN 'computing inherited statistics'::text
ELSE NULL::text
END AS phase,
s.param2 AS num_target_sample_rows,
s.param3 AS num_rows_sampled,
s.param4 AS num_rows_processed
FROM pg_stat_get_progress_info('ANALYZE'::text) s(pid, datid, relid, param1, param2, param3, param4, param5, param6, param7, param8, param9, param10)
LEFT JOIN pg_database d ON s.datid = d.oid;
Regards,
Vinayak Pokale
NTT Open Source Software Center
Attachment
On Fri, Mar 24, 2017 at 3:41 AM, vinayak <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote: > I have updated the patch. You can't change the definition of AcquireSampleRowsFunc without updating the documentation in fdwhandler.sgml, but I think I don't immediately understand why that's a thing we want to do at all if neither file_fdw nor postgres_fdw are going to use the additional argument. It seems to be entirely pointless because the new value being passed down is always zero and even if the callee were to update it, do_analyze_rel() would just ignore the change on return. Am I missing something here? Also, the whitespace-only changes to fdwapi.h related to AcquireSampleRowsFunc going to get undone by pgindent, so even if there's some reason to change that you should leave the lines that don't need changing untouched. + /* Reset rows processed to zero for the next column */ + pgstat_progress_update_param(PROGRESS_ANALYZE_NUM_ROWS_PROCESSED, 0); This seems like a bad design. If you see a value other than zero in this field, you still won't know how much progress analyze has made, because you don't know which column you're processing. I also feel like you're not paying enough attention to a key point here that I made in my last review, which is that you need to look at where ANALYZE actually spends most of the time. If the process of computing the per-row statistics consumes significant CPU time, then maybe there's some point in trying to instrument it, but does it? Unless you know the answer to that question, you don't know whether there's even a point to trying to instrument this. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2017/03/25 4:30, Robert Haas wrote: > On Fri, Mar 24, 2017 at 3:41 AM, vinayak > <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote: >> I have updated the patch. > You can't change the definition of AcquireSampleRowsFunc without > updating the documentation in fdwhandler.sgml, but I think I don't > immediately understand why that's a thing we want to do at all if > neither file_fdw nor postgres_fdw are going to use the additional > argument. It seems to be entirely pointless because the new value > being passed down is always zero and even if the callee were to update > it, do_analyze_rel() would just ignore the change on return. Am I > missing something here? Also, the whitespace-only changes to fdwapi.h > related to AcquireSampleRowsFunc going to get undone by pgindent, so > even if there's some reason to change that you should leave the lines > that don't need changing untouched. I Understood that we could not change the definition of AcquireSampleRowsFunc without updating the documentation in fdwhandler.sgml. In the last patch I have modified the definition of AcquireSampleRowsFunc to handle the inheritance case. If the table being analyzed has one or more children, ANALYZE will gather statistics twice: once on the rows of parent table only and second on the rows of the parent table with all of its children. So while reporting the progress the value of number of target sample rows and number of rows sampled is mismatched. For single relation it works fine. In the attached patch I have not change the definition of AcquireSampleRowsFunc. I have updated inheritance case in the the documentation. > > + /* Reset rows processed to zero for the next column */ > + pgstat_progress_update_param(PROGRESS_ANALYZE_NUM_ROWS_PROCESSED, > 0); > > This seems like a bad design. If you see a value other than zero in > this field, you still won't know how much progress analyze has made, > because you don't know which column you're processing. I also feel > like you're not paying enough attention to a key point here that I > made in my last review, which is that you need to look at where > ANALYZE actually spends most of the time. If the process of computing > the per-row statistics consumes significant CPU time, then maybe > there's some point in trying to instrument it, but does it? Unless > you know the answer to that question, you don't know whether there's > even a point to trying to instrument this. > Understood. The computing statistics phase takes long time so I am looking at the code. Regards, Vinayak Pokale NTT Open Source Software Center
Attachment
On Wed, Mar 29, 2017 at 5:38 PM, vinayak
<Pokale_Vinayak_q3@lab.ntt.co.jp> wrote:
>
> On 2017/03/25 4:30, Robert Haas wrote:
>>
>> On Fri, Mar 24, 2017 at 3:41 AM, vinayak
>> <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote:
>>>
>>> I have updated the patch.
>>
>> You can't change the definition of AcquireSampleRowsFunc without
>> updating the documentation in fdwhandler.sgml, but I think I don't
>> immediately understand why that's a thing we want to do at all if
>> neither file_fdw nor postgres_fdw are going to use the additional
>> argument. It seems to be entirely pointless because the new value
>> being passed down is always zero and even if the callee were to update
>> it, do_analyze_rel() would just ignore the change on return. Am I
>> missing something here? Also, the whitespace-only changes to fdwapi.h
>> related to AcquireSampleRowsFunc going to get undone by pgindent, so
>> even if there's some reason to change that you should leave the lines
>> that don't need changing untouched.
I reviewed v7 patch.
When I ran ANALYZE command to the table having 5 millions rows with 3
child tables, the progress report I got shows the strange result. The
following result was output after sampled all target rows from child
tables (i.g, after finished acquire_inherited_sample_rows). I think
the progress information should report 100% complete at this point.
#= select * from pg_stat_progress_analyze ; pid | datid | datname | relid | phase |
num_target_sample_rows | num_rows_sampled
-------+-------+----------+-------+----------------------------------+------------------------+------------------81719
|13215 | postgres | 16440 | collecting inherited sample rows | 3000000 | 1800000
(1 row)
I guess the cause of this is that you always count the number of
sampled rows in acquire_sample_rows staring from 0 for each child
table. num_rows_sampled is reset whenever starting collecting sample
rows.
Also, even if table has child table, the total number of sampling row
is not changed. I think that main differences between analyzing on
normal table and on partitioned table is where we fetch the sample row
from. So I'm not sure if adding "computing inherited statistics" phase
is desirable. If you want to report progress of collecting sample rows
on child table I think that you might want to add the information
showing which child table is being analyzed.
--
pg_stat_progress_analyze.num_target_sample_rows is set while ignoring
the number of rows the target table has actually. So If the table has
rows less than target number of rows, the num_rows_sampled never reach
to num_target_sample_rows.
--
@@ -1180,6 +1213,9 @@ acquire_sample_rows(Relation onerel, int elevel, }
samplerows += 1;
+
+ /* Report number of rows sampled so far */
+
pgstat_progress_update_param(PROGRESS_ANALYZE_NUM_ROWS_SAMPLED,
numrows); } }
I think that it's wrong to use numrows variable to report the progress
of the number of rows sampled. As the following comment mentioned, we
first save row into rows[] and increment numrows until numrows <
targrows. And then we could replace stored sample row with new sampled
row. That is, after collecting "numrows" rows, analyze can still take
a long time to get and replace the sample row. To computing progress
of collecting sample row, IMO reporting the number of blocks we
scanned so far is better than number of sample rows. Thought?
> /*
> * The first targrows sample rows are simply copied into the
> * reservoir. Then we start replacing tuples in the sample
> * until we reach the end of the relation. This algorithm is
> * from Jeff Vitter's paper (see full citation below). It
> * works by repeatedly computing the number of tuples to skip
> * before selecting a tuple, which replaces a randomly chosen
> * element of the reservoir (current set of tuples). At all
> * times the reservoir is a true random sample of the tuples
> * we've passed over so far, so when we fall off the end of
> * the relation we're done.
> */
> I Understood that we could not change the definition of
> AcquireSampleRowsFunc without updating the documentation in fdwhandler.sgml.
> In the last patch I have modified the definition of AcquireSampleRowsFunc to
> handle the inheritance case.
> If the table being analyzed has one or more children, ANALYZE will gather
> statistics twice:
> once on the rows of parent table only and second on the rows of the parent
> table with all of its children.
> So while reporting the progress the value of number of target sample rows
> and number of rows sampled is mismatched.
> For single relation it works fine.
>
> In the attached patch I have not change the definition of
> AcquireSampleRowsFunc.
> I have updated inheritance case in the the documentation.
>
>>
>> + /* Reset rows processed to zero for the next column */
>> +
>> pgstat_progress_update_param(PROGRESS_ANALYZE_NUM_ROWS_PROCESSED,
>> 0);
>>
>> This seems like a bad design. If you see a value other than zero in
>> this field, you still won't know how much progress analyze has made,
>> because you don't know which column you're processing. I also feel
>> like you're not paying enough attention to a key point here that I
>> made in my last review, which is that you need to look at where
>> ANALYZE actually spends most of the time. If the process of computing
>> the per-row statistics consumes significant CPU time, then maybe
>> there's some point in trying to instrument it, but does it? Unless
>> you know the answer to that question, you don't know whether there's
>> even a point to trying to instrument this.
>>
> Understood. The computing statistics phase takes long time so I am looking
> at the code.
Yes, ANALYZE spends most of time on computing statistics phase. I
measured the duration time for each phases on my environment. I set up
the table by pgbench -i -s 100 (total 10 million rows), and filled the
filler column of pgbench_accounts table with random string. And I set
default_statistics_target to 1000, and ran analyze on that table. The
analyze scanned all blocks of target table and collected 3000000
sample rows. The durations of each phase are,
* Sampling : 7 sec
* Computing statistics : 28 sec * aid column : 1 sec * bid column : 1 sec * abalance column : 1 sec * filler
column: 25 sec
I'm not sure if we can calculate the meaningful progress information
in computing statistics function such as compute_scalar_stats,
compute_trivial_stats. But I think that at least showing which
statistics of column is being computed would be good information for
user.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
On 2017/03/30 17:39, Masahiko Sawada wrote: > On Wed, Mar 29, 2017 at 5:38 PM, vinayak > <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote: >> On 2017/03/25 4:30, Robert Haas wrote: >>> On Fri, Mar 24, 2017 at 3:41 AM, vinayak >>> <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote: >>>> I have updated the patch. >>> You can't change the definition of AcquireSampleRowsFunc without >>> updating the documentation in fdwhandler.sgml, but I think I don't >>> immediately understand why that's a thing we want to do at all if >>> neither file_fdw nor postgres_fdw are going to use the additional >>> argument. It seems to be entirely pointless because the new value >>> being passed down is always zero and even if the callee were to update >>> it, do_analyze_rel() would just ignore the change on return. Am I >>> missing something here? Also, the whitespace-only changes to fdwapi.h >>> related to AcquireSampleRowsFunc going to get undone by pgindent, so >>> even if there's some reason to change that you should leave the lines >>> that don't need changing untouched. > I reviewed v7 patch. Thank you for reviewing the patch. > > When I ran ANALYZE command to the table having 5 millions rows with 3 > child tables, the progress report I got shows the strange result. The > following result was output after sampled all target rows from child > tables (i.g, after finished acquire_inherited_sample_rows). I think > the progress information should report 100% complete at this point. > > #= select * from pg_stat_progress_analyze ; > pid | datid | datname | relid | phase | > num_target_sample_rows | num_rows_sampled > -------+-------+----------+-------+----------------------------------+------------------------+------------------ > 81719 | 13215 | postgres | 16440 | collecting inherited sample rows | > 3000000 | 1800000 > (1 row) > > I guess the cause of this is that you always count the number of > sampled rows in acquire_sample_rows staring from 0 for each child > table. num_rows_sampled is reset whenever starting collecting sample > rows. > Also, even if table has child table, the total number of sampling row > is not changed. I think that main differences between analyzing on > normal table and on partitioned table is where we fetch the sample row > from. So I'm not sure if adding "computing inherited statistics" phase > is desirable. If you want to report progress of collecting sample rows > on child table I think that you might want to add the information > showing which child table is being analyzed. Yes. I think showing child table information would be good to user/DBA. > -- > pg_stat_progress_analyze.num_target_sample_rows is set while ignoring > the number of rows the target table has actually. So If the table has > rows less than target number of rows, the num_rows_sampled never reach > to num_target_sample_rows. > > -- > @@ -1180,6 +1213,9 @@ acquire_sample_rows(Relation onerel, int elevel, > } > > samplerows += 1; > + > + /* Report number of rows sampled so far */ > + > pgstat_progress_update_param(PROGRESS_ANALYZE_NUM_ROWS_SAMPLED, > numrows); > } > } > > I think that it's wrong to use numrows variable to report the progress > of the number of rows sampled. As the following comment mentioned, we > first save row into rows[] and increment numrows until numrows < > targrows. And then we could replace stored sample row with new sampled > row. That is, after collecting "numrows" rows, analyze can still take > a long time to get and replace the sample row. To computing progress > of collecting sample row, IMO reporting the number of blocks we > scanned so far is better than number of sample rows. Thought? > >> /* >> * The first targrows sample rows are simply copied into the >> * reservoir. Then we start replacing tuples in the sample >> * until we reach the end of the relation. This algorithm is >> * from Jeff Vitter's paper (see full citation below). It >> * works by repeatedly computing the number of tuples to skip >> * before selecting a tuple, which replaces a randomly chosen >> * element of the reservoir (current set of tuples). At all >> * times the reservoir is a true random sample of the tuples >> * we've passed over so far, so when we fall off the end of >> * the relation we're done. >> */ I think we can either report number of blocks scanned so far or number of sample rows. But I agree with you that reporting the number of blocks scanned so far would be better than reporting number of sample rows. >> I Understood that we could not change the definition of >> AcquireSampleRowsFunc without updating the documentation in fdwhandler.sgml. >> In the last patch I have modified the definition of AcquireSampleRowsFunc to >> handle the inheritance case. >> If the table being analyzed has one or more children, ANALYZE will gather >> statistics twice: >> once on the rows of parent table only and second on the rows of the parent >> table with all of its children. >> So while reporting the progress the value of number of target sample rows >> and number of rows sampled is mismatched. >> For single relation it works fine. >> >> In the attached patch I have not change the definition of >> AcquireSampleRowsFunc. >> I have updated inheritance case in the the documentation. >> >>> + /* Reset rows processed to zero for the next column */ >>> + >>> pgstat_progress_update_param(PROGRESS_ANALYZE_NUM_ROWS_PROCESSED, >>> 0); >>> >>> This seems like a bad design. If you see a value other than zero in >>> this field, you still won't know how much progress analyze has made, >>> because you don't know which column you're processing. I also feel >>> like you're not paying enough attention to a key point here that I >>> made in my last review, which is that you need to look at where >>> ANALYZE actually spends most of the time. If the process of computing >>> the per-row statistics consumes significant CPU time, then maybe >>> there's some point in trying to instrument it, but does it? Unless >>> you know the answer to that question, you don't know whether there's >>> even a point to trying to instrument this. >>> >> Understood. The computing statistics phase takes long time so I am looking >> at the code. > Yes, ANALYZE spends most of time on computing statistics phase. I > measured the duration time for each phases on my environment. I set up > the table by pgbench -i -s 100 (total 10 million rows), and filled the > filler column of pgbench_accounts table with random string. And I set > default_statistics_target to 1000, and ran analyze on that table. The > analyze scanned all blocks of target table and collected 3000000 > sample rows. The durations of each phase are, > > * Sampling : 7 sec > * Computing statistics : 28 sec > * aid column : 1 sec > * bid column : 1 sec > * abalance column : 1 sec > * filler column : 25 sec > > I'm not sure if we can calculate the meaningful progress information > in computing statistics function such as compute_scalar_stats, > compute_trivial_stats. But I think that at least showing which > statistics of column is being computed would be good information for > user. +1. I think this kind of progress information would be helpful for user. Regards, Vinayak Pokale NTT Open Source Software Center
On Thu, Mar 30, 2017 at 6:19 PM, vinayak <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote: > > On 2017/03/30 17:39, Masahiko Sawada wrote: >> >> On Wed, Mar 29, 2017 at 5:38 PM, vinayak >> <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote: >>> >>> On 2017/03/25 4:30, Robert Haas wrote: >>>> >>>> On Fri, Mar 24, 2017 at 3:41 AM, vinayak >>>> <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote: >>>>> >>>>> I have updated the patch. >>>> >>>> You can't change the definition of AcquireSampleRowsFunc without >>>> updating the documentation in fdwhandler.sgml, but I think I don't >>>> immediately understand why that's a thing we want to do at all if >>>> neither file_fdw nor postgres_fdw are going to use the additional >>>> argument. It seems to be entirely pointless because the new value >>>> being passed down is always zero and even if the callee were to update >>>> it, do_analyze_rel() would just ignore the change on return. Am I >>>> missing something here? Also, the whitespace-only changes to fdwapi.h >>>> related to AcquireSampleRowsFunc going to get undone by pgindent, so >>>> even if there's some reason to change that you should leave the lines >>>> that don't need changing untouched. >> >> I reviewed v7 patch. > > Thank you for reviewing the patch. >> >> >> When I ran ANALYZE command to the table having 5 millions rows with 3 >> child tables, the progress report I got shows the strange result. The >> following result was output after sampled all target rows from child >> tables (i.g, after finished acquire_inherited_sample_rows). I think >> the progress information should report 100% complete at this point. >> >> #= select * from pg_stat_progress_analyze ; >> pid | datid | datname | relid | phase | >> num_target_sample_rows | num_rows_sampled >> >> -------+-------+----------+-------+----------------------------------+------------------------+------------------ >> 81719 | 13215 | postgres | 16440 | collecting inherited sample rows | >> 3000000 | 1800000 >> (1 row) >> >> I guess the cause of this is that you always count the number of >> sampled rows in acquire_sample_rows staring from 0 for each child >> table. num_rows_sampled is reset whenever starting collecting sample >> rows. >> Also, even if table has child table, the total number of sampling row >> is not changed. I think that main differences between analyzing on >> normal table and on partitioned table is where we fetch the sample row >> from. So I'm not sure if adding "computing inherited statistics" phase >> is desirable. If you want to report progress of collecting sample rows >> on child table I think that you might want to add the information >> showing which child table is being analyzed. > > Yes. I think showing child table information would be good to user/DBA. > >> -- >> pg_stat_progress_analyze.num_target_sample_rows is set while ignoring >> the number of rows the target table has actually. So If the table has >> rows less than target number of rows, the num_rows_sampled never reach >> to num_target_sample_rows. >> >> -- >> @@ -1180,6 +1213,9 @@ acquire_sample_rows(Relation onerel, int elevel, >> } >> >> samplerows += 1; >> + >> + /* Report number of rows sampled so far */ >> + >> pgstat_progress_update_param(PROGRESS_ANALYZE_NUM_ROWS_SAMPLED, >> numrows); >> } >> } >> >> I think that it's wrong to use numrows variable to report the progress >> of the number of rows sampled. As the following comment mentioned, we >> first save row into rows[] and increment numrows until numrows < >> targrows. And then we could replace stored sample row with new sampled >> row. That is, after collecting "numrows" rows, analyze can still take >> a long time to get and replace the sample row. To computing progress >> of collecting sample row, IMO reporting the number of blocks we >> scanned so far is better than number of sample rows. Thought? >> >>> /* >>> * The first targrows sample rows are simply copied into the >>> * reservoir. Then we start replacing tuples in the sample >>> * until we reach the end of the relation. This algorithm is >>> * from Jeff Vitter's paper (see full citation below). It >>> * works by repeatedly computing the number of tuples to skip >>> * before selecting a tuple, which replaces a randomly chosen >>> * element of the reservoir (current set of tuples). At all >>> * times the reservoir is a true random sample of the tuples >>> * we've passed over so far, so when we fall off the end of >>> * the relation we're done. >>> */ > > I think we can either report number of blocks scanned so far or number of > sample rows. > But I agree with you that reporting the number of blocks scanned so far > would be better than reporting number of sample rows. > >>> I Understood that we could not change the definition of >>> AcquireSampleRowsFunc without updating the documentation in >>> fdwhandler.sgml. >>> In the last patch I have modified the definition of AcquireSampleRowsFunc >>> to >>> handle the inheritance case. >>> If the table being analyzed has one or more children, ANALYZE will gather >>> statistics twice: >>> once on the rows of parent table only and second on the rows of the >>> parent >>> table with all of its children. >>> So while reporting the progress the value of number of target sample rows >>> and number of rows sampled is mismatched. >>> For single relation it works fine. >>> >>> In the attached patch I have not change the definition of >>> AcquireSampleRowsFunc. >>> I have updated inheritance case in the the documentation. >>> >>>> + /* Reset rows processed to zero for the next column */ >>>> + >>>> pgstat_progress_update_param(PROGRESS_ANALYZE_NUM_ROWS_PROCESSED, >>>> 0); >>>> >>>> This seems like a bad design. If you see a value other than zero in >>>> this field, you still won't know how much progress analyze has made, >>>> because you don't know which column you're processing. I also feel >>>> like you're not paying enough attention to a key point here that I >>>> made in my last review, which is that you need to look at where >>>> ANALYZE actually spends most of the time. If the process of computing >>>> the per-row statistics consumes significant CPU time, then maybe >>>> there's some point in trying to instrument it, but does it? Unless >>>> you know the answer to that question, you don't know whether there's >>>> even a point to trying to instrument this. >>>> >>> Understood. The computing statistics phase takes long time so I am >>> looking >>> at the code. >> >> Yes, ANALYZE spends most of time on computing statistics phase. I >> measured the duration time for each phases on my environment. I set up >> the table by pgbench -i -s 100 (total 10 million rows), and filled the >> filler column of pgbench_accounts table with random string. And I set >> default_statistics_target to 1000, and ran analyze on that table. The >> analyze scanned all blocks of target table and collected 3000000 >> sample rows. The durations of each phase are, >> >> * Sampling : 7 sec >> * Computing statistics : 28 sec >> * aid column : 1 sec >> * bid column : 1 sec >> * abalance column : 1 sec >> * filler column : 25 sec >> >> I'm not sure if we can calculate the meaningful progress information >> in computing statistics function such as compute_scalar_stats, >> compute_trivial_stats. But I think that at least showing which >> statistics of column is being computed would be good information for >> user. > > +1. > I think this kind of progress information would be helpful for user. > Also, how are you going to support the progress checker for foreign table? In current design, each FDW has to count and report the progress in their analyze function (AnalyzeForeignTable API), in order to support the analyze progress of the foreign table. You can leave it at this time but I think that we should document about it in either case. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
On 2017/03/30 17:39, Masahiko Sawada wrote: > On Wed, Mar 29, 2017 at 5:38 PM, vinayak > <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote: >> On 2017/03/25 4:30, Robert Haas wrote: >>> On Fri, Mar 24, 2017 at 3:41 AM, vinayak >>> <Pokale_Vinayak_q3@lab.ntt.co.jp> wrote: >>>> I have updated the patch. >>> You can't change the definition of AcquireSampleRowsFunc without >>> updating the documentation in fdwhandler.sgml, but I think I don't >>> immediately understand why that's a thing we want to do at all if >>> neither file_fdw nor postgres_fdw are going to use the additional >>> argument. It seems to be entirely pointless because the new value >>> being passed down is always zero and even if the callee were to update >>> it, do_analyze_rel() would just ignore the change on return. Am I >>> missing something here? Also, the whitespace-only changes to fdwapi.h >>> related to AcquireSampleRowsFunc going to get undone by pgindent, so >>> even if there's some reason to change that you should leave the lines >>> that don't need changing untouched. > I reviewed v7 patch. > > When I ran ANALYZE command to the table having 5 millions rows with 3 > child tables, the progress report I got shows the strange result. The > following result was output after sampled all target rows from child > tables (i.g, after finished acquire_inherited_sample_rows). I think > the progress information should report 100% complete at this point. > > #= select * from pg_stat_progress_analyze ; > pid | datid | datname | relid | phase | > num_target_sample_rows | num_rows_sampled > -------+-------+----------+-------+----------------------------------+------------------------+------------------ > 81719 | 13215 | postgres | 16440 | collecting inherited sample rows | > 3000000 | 1800000 > (1 row) > > I guess the cause of this is that you always count the number of > sampled rows in acquire_sample_rows staring from 0 for each child > table. num_rows_sampled is reset whenever starting collecting sample > rows. > Also, even if table has child table, the total number of sampling row > is not changed. I think that main differences between analyzing on > normal table and on partitioned table is where we fetch the sample row > from. So I'm not sure if adding "computing inherited statistics" phase > is desirable. If you want to report progress of collecting sample rows > on child table I think that you might want to add the information > showing which child table is being analyzed. > > -- > pg_stat_progress_analyze.num_target_sample_rows is set while ignoring > the number of rows the target table has actually. So If the table has > rows less than target number of rows, the num_rows_sampled never reach > to num_target_sample_rows. > > -- > @@ -1180,6 +1213,9 @@ acquire_sample_rows(Relation onerel, int elevel, > } > > samplerows += 1; > + > + /* Report number of rows sampled so far */ > + > pgstat_progress_update_param(PROGRESS_ANALYZE_NUM_ROWS_SAMPLED, > numrows); > } > } > > I think that it's wrong to use numrows variable to report the progress > of the number of rows sampled. As the following comment mentioned, we > first save row into rows[] and increment numrows until numrows < > targrows. And then we could replace stored sample row with new sampled > row. That is, after collecting "numrows" rows, analyze can still take > a long time to get and replace the sample row. To computing progress > of collecting sample row, IMO reporting the number of blocks we > scanned so far is better than number of sample rows. Thought? > >> /* >> * The first targrows sample rows are simply copied into the >> * reservoir. Then we start replacing tuples in the sample >> * until we reach the end of the relation. This algorithm is >> * from Jeff Vitter's paper (see full citation below). It >> * works by repeatedly computing the number of tuples to skip >> * before selecting a tuple, which replaces a randomly chosen >> * element of the reservoir (current set of tuples). At all >> * times the reservoir is a true random sample of the tuples >> * we've passed over so far, so when we fall off the end of >> * the relation we're done. >> */ Looks good to me. In the attached patch I have reported number of blocks scanned so far instead of number of sample rows. In the 'collecting inherited sample rows' phase, child_relid is reported as a separate column. The child_relid is reported its value only in 'collecting inherited sample rows' phase. For single relation it return 0. I am not sure whether this column would helpful or not. Any suggestions are welcome. >>> + /* Reset rows processed to zero for the next column */ >>> + >>> pgstat_progress_update_param(PROGRESS_ANALYZE_NUM_ROWS_PROCESSED, >>> 0); >>> >>> This seems like a bad design. If you see a value other than zero in >>> this field, you still won't know how much progress analyze has made, >>> because you don't know which column you're processing. I also feel >>> like you're not paying enough attention to a key point here that I >>> made in my last review, which is that you need to look at where >>> ANALYZE actually spends most of the time. If the process of computing >>> the per-row statistics consumes significant CPU time, then maybe >>> there's some point in trying to instrument it, but does it? Unless >>> you know the answer to that question, you don't know whether there's >>> even a point to trying to instrument this. >>> >> Understood. The computing statistics phase takes long time so I am looking >> at the code. > Yes, ANALYZE spends most of time on computing statistics phase. I > measured the duration time for each phases on my environment. I set up > the table by pgbench -i -s 100 (total 10 million rows), and filled the > filler column of pgbench_accounts table with random string. And I set > default_statistics_target to 1000, and ran analyze on that table. The > analyze scanned all blocks of target table and collected 3000000 > sample rows. The durations of each phase are, > > * Sampling : 7 sec > * Computing statistics : 28 sec > * aid column : 1 sec > * bid column : 1 sec > * abalance column : 1 sec > * filler column : 25 sec > > I'm not sure if we can calculate the meaningful progress information > in computing statistics function such as compute_scalar_stats, > compute_trivial_stats. But I think that at least showing which > statistics of column is being computed would be good information for > user. I agree with you that showing statistics of the column is being computed would be good information but progress reporting API is only supported for int64 values. So I think it would be good if we report such value that will be helpful for user in computing statistics phase. Regards, Vinayak Pokale NTT Open Source Software Center
Attachment
On 2017/03/30 17:39, Masahiko Sawada wrote: > On Wed, Mar 29, 2017 at 5:38 PM, vinayak wrote: >>>> I have updated the patch. > > I reviewed v7 patch. > > When I ran ANALYZE command to the table having 5 millions rows with 3 > child tables, the progress report I got shows the strange result. The > following result was output after sampled all target rows from child > tables (i.g, after finished acquire_inherited_sample_rows). I think > the progress information should report 100% complete at this point. > [...] > > I guess the cause of this is that you always count the number of > sampled rows in acquire_sample_rows staring from 0 for each child > table. num_rows_sampled is reset whenever starting collecting sample > rows. > Also, even if table has child table, the total number of sampling row > is not changed. I think that main differences between analyzing on > normal table and on partitioned table is where we fetch the sample row > from. So I'm not sure if adding "computing inherited statistics" phase > is desirable. If you want to report progress of collecting sample rows > on child table I think that you might want to add the information > showing which child table is being analyzed. Two kinds of statistics are collected if the table is a inheritance parent. First kind considers the table by itself and calculates regular single-table statistics using rows sampled from the only available heap (by calling do_analyze_rel() with inh=false). The second kind are called inheritance statistics, which consider rows sampled from the whole inheritance hierarchy (by calling do_analyze_rel() with inh=true). Note that we are still collecting statistics for the parent table, so we not really "analyzing" child tables; we just collect sample rows from them to calculate whole-tree statistics for the root parent table. It might still be possible to show the child table being sampled though. > -- > pg_stat_progress_analyze.num_target_sample_rows is set while ignoring > the number of rows the target table has actually. So If the table has > rows less than target number of rows, the num_rows_sampled never reach > to num_target_sample_rows. > > -- > @@ -1180,6 +1213,9 @@ acquire_sample_rows(Relation onerel, int elevel, > } > > samplerows += 1; > + > + /* Report number of rows sampled so far */ > + > pgstat_progress_update_param(PROGRESS_ANALYZE_NUM_ROWS_SAMPLED, > numrows); > } > } > > I think that it's wrong to use numrows variable to report the progress > of the number of rows sampled. As the following comment mentioned, we > first save row into rows[] and increment numrows until numrows < > targrows. And then we could replace stored sample row with new sampled > row. That is, after collecting "numrows" rows, analyze can still take > a long time to get and replace the sample row. To computing progress > of collecting sample row, IMO reporting the number of blocks we > scanned so far is better than number of sample rows. Thought? We can report progress in terms of individual blocks only inside acquire_sample_rows(), which seems undesirable when one thinks that we will be resetting the target for every child table. We should have a global target that considers all child tables in the inheritance hierarchy, which maybe is possible if we count them beforehand in acquire_inheritance_sample_rows(). But why not use target sample rows, which remains the same for both when we're collecting sample rows from one table and from the whole inheritance hierarchy. We can keep the count of already collected rows in a struct that is used across calls for all the child tables and increment upward from that count when we start collecting from a new child table. >> /* >> * The first targrows sample rows are simply copied into the >> * reservoir. Then we start replacing tuples in the sample >> * until we reach the end of the relation. This algorithm is >> * from Jeff Vitter's paper (see full citation below). It >> * works by repeatedly computing the number of tuples to skip >> * before selecting a tuple, which replaces a randomly chosen >> * element of the reservoir (current set of tuples). At all >> * times the reservoir is a true random sample of the tuples >> * we've passed over so far, so when we fall off the end of >> * the relation we're done. >> */ It seems that we could use samplerows instead of numrows to count the progress (if we choose to count progress in terms of sample rows collected). Thanks, Amit
On 2017/04/04 14:12, Amit Langote wrote: > Two kinds of statistics are collected if the table is a inheritance parent. > > First kind considers the table by itself and calculates regular > single-table statistics using rows sampled from the only available heap > (by calling do_analyze_rel() with inh=false). Oops, should have also mentioned that this part is not true for the new partitioned tables. There are single-table statistics for partitioned tables, because it contains no data. Thanks, Amit
On 2017/04/04 14:15, Amit Langote wrote: > On 2017/04/04 14:12, Amit Langote wrote: >> Two kinds of statistics are collected if the table is a inheritance parent. >> >> First kind considers the table by itself and calculates regular >> single-table statistics using rows sampled from the only available heap >> (by calling do_analyze_rel() with inh=false). > > Oops, should have also mentioned that this part is not true for the new > partitioned tables. There are single-table statistics for partitioned > tables, because it contains no data. Sorry again, I meant *no* single-table statistics partitioned tables... Thanks, Amit
On Tue, Apr 4, 2017 at 2:12 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > On 2017/03/30 17:39, Masahiko Sawada wrote: >> On Wed, Mar 29, 2017 at 5:38 PM, vinayak wrote: >>>>> I have updated the patch. >> >> I reviewed v7 patch. >> >> When I ran ANALYZE command to the table having 5 millions rows with 3 >> child tables, the progress report I got shows the strange result. The >> following result was output after sampled all target rows from child >> tables (i.g, after finished acquire_inherited_sample_rows). I think >> the progress information should report 100% complete at this point. >> > > [...] > >> >> I guess the cause of this is that you always count the number of >> sampled rows in acquire_sample_rows staring from 0 for each child >> table. num_rows_sampled is reset whenever starting collecting sample >> rows. >> Also, even if table has child table, the total number of sampling row >> is not changed. I think that main differences between analyzing on >> normal table and on partitioned table is where we fetch the sample row >> from. So I'm not sure if adding "computing inherited statistics" phase >> is desirable. If you want to report progress of collecting sample rows >> on child table I think that you might want to add the information >> showing which child table is being analyzed. > > Two kinds of statistics are collected if the table is a inheritance parent. > > First kind considers the table by itself and calculates regular > single-table statistics using rows sampled from the only available heap > (by calling do_analyze_rel() with inh=false). > > The second kind are called inheritance statistics, which consider rows > sampled from the whole inheritance hierarchy (by calling do_analyze_rel() > with inh=true). Note that we are still collecting statistics for the > parent table, so we not really "analyzing" child tables; we just collect > sample rows from them to calculate whole-tree statistics for the root > parent table. Agreed. > It might still be possible to show the child table being > sampled though. > >> -- >> pg_stat_progress_analyze.num_target_sample_rows is set while ignoring >> the number of rows the target table has actually. So If the table has >> rows less than target number of rows, the num_rows_sampled never reach >> to num_target_sample_rows. >> >> -- >> @@ -1180,6 +1213,9 @@ acquire_sample_rows(Relation onerel, int elevel, >> } >> >> samplerows += 1; >> + >> + /* Report number of rows sampled so far */ >> + >> pgstat_progress_update_param(PROGRESS_ANALYZE_NUM_ROWS_SAMPLED, >> numrows); >> } >> } >> >> I think that it's wrong to use numrows variable to report the progress >> of the number of rows sampled. As the following comment mentioned, we >> first save row into rows[] and increment numrows until numrows < >> targrows. And then we could replace stored sample row with new sampled >> row. That is, after collecting "numrows" rows, analyze can still take >> a long time to get and replace the sample row. To computing progress >> of collecting sample row, IMO reporting the number of blocks we >> scanned so far is better than number of sample rows. Thought? > > We can report progress in terms of individual blocks only inside > acquire_sample_rows(), which seems undesirable when one thinks that we > will be resetting the target for every child table. We should have a > global target that considers all child tables in the inheritance > hierarchy, which maybe is possible if we count them beforehand in > acquire_inheritance_sample_rows(). But why not use target sample rows, > which remains the same for both when we're collecting sample rows from one > table and from the whole inheritance hierarchy. We can keep the count of > already collected rows in a struct that is used across calls for all the > child tables and increment upward from that count when we start collecting > from a new child table. An another option I came up with is that support new pgstat progress function, say pgstat_progress_incr_param, which increments index'th member in st_progress_param[]. That way we just need to report a delta using that function. > >>> /* >>> * The first targrows sample rows are simply copied into the >>> * reservoir. Then we start replacing tuples in the sample >>> * until we reach the end of the relation. This algorithm is >>> * from Jeff Vitter's paper (see full citation below). It >>> * works by repeatedly computing the number of tuples to skip >>> * before selecting a tuple, which replaces a randomly chosen >>> * element of the reservoir (current set of tuples). At all >>> * times the reservoir is a true random sample of the tuples >>> * we've passed over so far, so when we fall off the end of >>> * the relation we're done. >>> */ > > It seems that we could use samplerows instead of numrows to count the > progress (if we choose to count progress in terms of sample rows collected). > I guess it's hard to count progress in terms of sample rows collected even if we use samplerows instead, because samplerows can be incremented independently of the target number of sampling rows. The samplerows can be incremented up to the total number of rows of relation. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
On 2017/04/04 15:30, Masahiko Sawada wrote: >> We can report progress in terms of individual blocks only inside >> acquire_sample_rows(), which seems undesirable when one thinks that we >> will be resetting the target for every child table. We should have a >> global target that considers all child tables in the inheritance >> hierarchy, which maybe is possible if we count them beforehand in >> acquire_inheritance_sample_rows(). But why not use target sample rows, >> which remains the same for both when we're collecting sample rows from one >> table and from the whole inheritance hierarchy. We can keep the count of >> already collected rows in a struct that is used across calls for all the >> child tables and increment upward from that count when we start collecting >> from a new child table. > > An another option I came up with is that support new pgstat progress > function, say pgstat_progress_incr_param, which increments index'th > member in st_progress_param[]. That way we just need to report a delta > using that function. That's an interesting idea. It could be made to work and would not require changing the interface of AcquireSampleRowsFunc, which seems very desirable. >>>> /* >>>> * The first targrows sample rows are simply copied into the >>>> * reservoir. Then we start replacing tuples in the sample >>>> * until we reach the end of the relation. This algorithm is >>>> * from Jeff Vitter's paper (see full citation below). It >>>> * works by repeatedly computing the number of tuples to skip >>>> * before selecting a tuple, which replaces a randomly chosen >>>> * element of the reservoir (current set of tuples). At all >>>> * times the reservoir is a true random sample of the tuples >>>> * we've passed over so far, so when we fall off the end of >>>> * the relation we're done. >>>> */ >> >> It seems that we could use samplerows instead of numrows to count the >> progress (if we choose to count progress in terms of sample rows collected). >> > > I guess it's hard to count progress in terms of sample rows collected > even if we use samplerows instead, because samplerows can be > incremented independently of the target number of sampling rows. The > samplerows can be incremented up to the total number of rows of > relation. Hmm, you're right. It could be counted with a separate variable initialized to 0 and incremented every time we decide to add a row to the final set of sampled rows, although different implementations of AcquireSampleRowsFunc have different ways of deciding if a given row will be part of the final set of sampled rows. On the other hand, if we decide to count progress in terms of blocks as you suggested afraid, I'm afraid that FDWs won't be able to report the progress. Thanks, Amit
On Tue, Apr 4, 2017 at 4:57 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > Hmm, you're right. It could be counted with a separate variable > initialized to 0 and incremented every time we decide to add a row to the > final set of sampled rows, although different implementations of > AcquireSampleRowsFunc have different ways of deciding if a given row will > be part of the final set of sampled rows. > > On the other hand, if we decide to count progress in terms of blocks as > you suggested afraid, I'm afraid that FDWs won't be able to report the > progress. I think it may be time to push this patch out to v11. It was submitted one day before the start of the last CommitFest, the design wasn't really right, and it's not clear even now that we know what the right design is. And we're pretty much out of time. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Apr 5, 2017 at 1:49 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Apr 4, 2017 at 4:57 AM, Amit Langote > <Langote_Amit_f8@lab.ntt.co.jp> wrote: >> Hmm, you're right. It could be counted with a separate variable >> initialized to 0 and incremented every time we decide to add a row to the >> final set of sampled rows, although different implementations of >> AcquireSampleRowsFunc have different ways of deciding if a given row will >> be part of the final set of sampled rows. >> >> On the other hand, if we decide to count progress in terms of blocks as >> you suggested afraid, I'm afraid that FDWs won't be able to report the >> progress. > > I think it may be time to push this patch out to v11. It was > submitted one day before the start of the last CommitFest, the design > wasn't really right, and it's not clear even now that we know what the > right design is. And we're pretty much out of time. > +1 We're encountering the design issue and it takes more time to find out right design including FDWs support. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
On 2017/04/05 10:17, Masahiko Sawada wrote:
> On Wed, Apr 5, 2017 at 1:49 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Tue, Apr 4, 2017 at 4:57 AM, Amit Langote
>> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>> Hmm, you're right. It could be counted with a separate variable
>>> initialized to 0 and incremented every time we decide to add a row to the
>>> final set of sampled rows, although different implementations of
>>> AcquireSampleRowsFunc have different ways of deciding if a given row will
>>> be part of the final set of sampled rows.
>>>
>>> On the other hand, if we decide to count progress in terms of blocks as
>>> you suggested afraid, I'm afraid that FDWs won't be able to report the
>>> progress.
>>
>> I think it may be time to push this patch out to v11. It was
>> submitted one day before the start of the last CommitFest, the design
>> wasn't really right, and it's not clear even now that we know what the
>> right design is. And we're pretty much out of time.
>>
>
> +1
> We're encountering the design issue and it takes more time to find out
> right design including FDWs support.
>
> Regards,
>
> --
> Masahiko Sawada
> NIPPON TELEGRAPH AND TELEPHONE CORPORATION
> NTT Open Source Software Center
Hi Vinayak,
I found a typo in your patch: s/taht/that/
sampling.c /* Report total number of blocks taht will be sampled */
Then, regarding FDWs support, I believe it means postgres_fdw support (is my understanding right?).
You might better to put pgstat_progress_update_param() into these functions, maybe. postgres_fdw.c -
postgresAnalyzeForeignTable - postgresAcquireSampleRowsFunc
And PgFdwAnalyzeState has these variables, hopefully you can get current number of rows
as a progress indicator.
sturuct PgFdwAnalyzeState ... /* collected sample rows */ HeapTuple *rows; /* array of size
targrows*/ int targrows; /* target # of sample rows */ int numrows; /* # of sample
rowscollected */
/* for random sampling */ double samplerows; /* # of rows fetched */ double rowstoskip; /*
#of rows to skip before next sample */ ...
I hope it will help you.
Regards,
Tatsuro Yamada
> On 05 Apr 2017, at 03:17, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Wed, Apr 5, 2017 at 1:49 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Tue, Apr 4, 2017 at 4:57 AM, Amit Langote >> <Langote_Amit_f8@lab.ntt.co.jp> wrote: >>> Hmm, you're right. It could be counted with a separate variable >>> initialized to 0 and incremented every time we decide to add a row to the >>> final set of sampled rows, although different implementations of >>> AcquireSampleRowsFunc have different ways of deciding if a given row will >>> be part of the final set of sampled rows. >>> >>> On the other hand, if we decide to count progress in terms of blocks as >>> you suggested afraid, I'm afraid that FDWs won't be able to report the >>> progress. >> >> I think it may be time to push this patch out to v11. It was >> submitted one day before the start of the last CommitFest, the design >> wasn't really right, and it's not clear even now that we know what the >> right design is. And we're pretty much out of time. > > +1 > We're encountering the design issue and it takes more time to find out > right design including FDWs support. I’m marking this patch Returned with Feedback since it has been Waiting for author during the commitfest without updates. cheers ./daniel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers