On Thu, Mar 10, 2016 at 3:08 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Hi Vinayak,
>
> Thanks for the quick review!
Committed 0001 earlier this morning.
On 0002:
+ /* total_index_blks */
+ current_index_blks = (BlockNumber *) palloc(nindexes *
sizeof(BlockNumber));
+ total_index_blks = 0;
+ for (i = 0; i < nindexes; i++)
+ {
+ BlockNumber nblocks =
RelationGetNumberOfBlocks(Irel[i]);
+
+ current_index_blks[i] = nblocks;
+ total_index_blks += nblocks;
+ }
+ pgstat_progress_update_param(PROG_PARAM_VAC_IDX_BLKS, total_index_blks);
I think this is a bad idea. The value calculated here isn't
necessarily accurate, because the number of index blocks can change
between the time this is calculated and the time the indexes are
actually vacuumed. If a client just wants the length of the indexes
in round figures, that's already SQL-visible, and there's little
reason to make VACUUM do it all the time whether anyone is looking at
the progress information or not. Note that I'm not complaining about
the fact that you exposed the heap block count, because in that case
you are exposing the actual value that VACUUM is using to guide its
work. The client can get the *current* length of the relation, but
the value you are exposing gives you the number of blocks *this
particular VACUUM intends to scan*. That has some incremental value -
but the index information doesn't have the same thing going for it.
On 0003:
I think you should make this work for all AMs, not just btree, and
then consolidate it with 0002.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company