On Sat, Mar 5, 2016 at 7:11 AM, Robert Haas <robertmh...@gmail.com> wrote: > On Fri, Feb 26, 2016 at 3:28 AM, <poku...@pm.nttdata.co.jp> wrote: >> Thank you for your comments. >> Please find attached patch addressing following comments. > > I'm positive I've said this at least once before while reviewing this > patch, and I think more than once: we should be trying to build a > general progress-reporting facility here with vacuum as the first > user. Therefore, for example, pg_stat_get_progress_info's output > columns should have generic names, not names specific to VACUUM. > pg_stat_vacuum_progress can alias them to a vacuum-specific name. See > for example the relationship between pg_stats and pg_statistic. > > I think VACUUM should have three phases, not two. lazy_vacuum_index() > and lazy_vacuum_heap() are lumped together right now, but I think they > shouldn't be. > > Please create named constants for the first argument to > pgstat_report_progress_update_counter(), maybe with names like > PROGRESS_VACUUM_WHATEVER. > > + /* Update current block number of the relation */ > + pgstat_report_progress_update_counter(2, blkno + 1); > > Why + 1? > > I thought we had a plan to update the counter of scanned index pages > after each index page was vacuumed by the AM. Doing it only after > vacuuming the entire index is much less granular and generally less > useful. See > http://www.postgresql.org/message-id/56500356.4070...@bluetreble.com > > + if (blkno == nblocks - 1 && > vacrelstats->num_dead_tuples == 0 && nindexes != 0 > + && vacrelstats->num_index_scans == 0) > + total_index_pages = 0; > > I'm not sure what this is trying to do, perhaps because there is no > comment explaining it. Whatever the intent, I suspect that such a > complex test is likely to be fragile. Perhaps there is a better way?
So, I took the Vinayak's latest patch and rewrote it a little while maintaining the original idea but modifying code to some degree. Hope original author(s) are okay with it. Vinayak, do see if the rewritten patch is alright and improve it anyway you want. I broke it into two: 0001-Provide-a-way-for-utility-commands-to-report-progres.patch 0002-Implement-progress-reporting-for-VACUUM-command.patch The code review comments received recently (including mine) have been incorporated. However, I didn't implement the report-per-index-page-vacuumed bit but should be easy to code once the details are finalized (questions like whether it requires modifying any existing interfaces, etc). Thanks, Amit
Description: Binary data
Description: Binary data
-- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers