On Sat, Mar 5, 2016 at 7:11 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Feb 26, 2016 at 3:28 AM, <pokurev@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.4070101@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