On 2015/12/10 15:28, Michael Paquier wrote: > On Thu, Dec 10, 2015 at 4:40 AM, Robert Haas <robertmh...@gmail.com> wrote: >> It's going to be *really* important that this facility provides a >> lightweight way of updating progress, so I think this whole API is >> badly designed. VACUUM, for example, is going to want a way to update >> the individual counter for the number of pages it's scanned after >> every page. It should not have to pass all of the other information >> that is part of a complete report. It should just be able to say >> pgstat_report_progress_update_counter(1, pages_scanned) or something >> of this sort. Don't marshal all of the data and then make >> pgstat_report_progress figure out what's changed. Provide a series of >> narrow APIs where the caller tells you exactly what they want to >> update, and you update only that. It's fine to have a >> pgstat_report_progress() function to update everything at once, for >> the use at the beginning of a command, but the incremental updates >> within the command should do something lighter-weight. > > [first time looking really at the patch and catching up with this thread] > > Agreed. As far as I can guess from the code, those should be as > followed to bloat pgstat message queue a minimum: > > + pgstat_report_activity_flag(ACTIVITY_IS_VACUUM); > /* > * Loop to process each selected relation. > */ > Defining a new routine for this purpose is a bit surprising. Cannot we > just use pgstat_report_activity with a new BackendState STATE_INVACUUM > or similar if we pursue the progress tracking approach?
ISTM, PgBackendStatus.st_state is normally manipulated at quite different sites (mostly tcop) than where a backend would be able to report that a command like VACUUM is running. Also, the value 'active' of pg_stat_activity.state has an established meaning as Horiguchi-san seems to point out in his reply. IMHO, this patch shouldn't affect such meaning of a pg_stat_activity field. > A couple of comments: > - The relation OID should be reported and not its name. In case of a > relation rename that would not be cool for tracking, and most users > are surely going to join with other system tables using it. +1 > - The progress tracking facility adds a whole level of complexity for > very little gain, and IMO this should *not* be part of PgBackendStatus > because in most cases its data finishes wasted. We don't expect > backends to run frequently such progress reports, do we? My opinion on > the matter if that we should define a different collector data for > vacuum, with something like PgStat_StatVacuumEntry, then have on top > of it a couple of routines dedicated at feeding up data with it when > some work is done on a vacuum job. I assume your comment here means we should use stats collector to the track/publish progress info, is that right? AIUI, the counts published via stats collector are updated asynchronously w.r.t. operations they count and mostly as aggregate figures. For example, PgStat_StatTabEntry.blocks_fetched. IOW, we never see pg_statio_all_tables.heap_blks_read updating as a scan reads blocks. Maybe that helps keep traffic to pgstat collector to sane levels. But that is not to mean that I think controlling stats collector levels was the only design consideration behind how such counters are published. In case of reporting counters as progress info, it seems we might have to send too many PgStat_Msg's, for example, for every block we finish processing during vacuum. That kind of message traffic may swamp the collector. Then we need to see the updated counters from other counters in near real-time though that may be possible with suitable (build?) configuration. Moreover, the counters reported as progress info seem to be of different nature than those published via the stats collector. I would think of it in terms of the distinction between track_activities and track_counts. I find these lines on "The Statistics Collector" page somewhat related: "PostgreSQL also supports reporting dynamic information about exactly what is going on in the system right now, such as the exact command currently being executed by other server processes, and which other connections exist in the system. This facility is independent of the collector process." Then, "When using the statistics to monitor collected data, it is important to realize that the information does not update instantaneously. Each individual server process transmits new statistical counts to the collector just before going idle; so a query or transaction still in progress does not affect the displayed totals. Also, the collector itself emits a new report at most once per PGSTAT_STAT_INTERVAL milliseconds (500 ms unless altered while building the server). So the displayed information lags behind actual activity. However, current-query information collected by track_activities is always up-to-date." http://www.postgresql.org/docs/devel/static/monitoring-stats.html Am I misunderstanding what you mean? > > In short, it seems to me that this patch needs a rework, and should be > returned with feedback. Other opinions? Yeah, some more thought needs to be put into design of the general reporting interface. Then we also need to pay attention to another important aspect of this patch - lazy vacuum instrumentation. Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
