On 2016/03/10 23:29, Robert Haas wrote: > On Thu, Mar 10, 2016 at 3:08 AM, Amit Langote > <langote_amit...@lab.ntt.co.jp> wrote: >> Hi Vinayak, >> >> Thanks for the quick review! > > Committed 0001 earlier this morning.
Thanks! > 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. So, from what I understand here, we should not put total count of index pages into st_progress_param; rather, have the client (reading pg_stat_progress_vacuum) derive it using pg_indexes_size() (?), as and when necessary. However, only server is able to tell the current position within an index vacuuming round (or how many pages into a given index vacuuming round), so report that using some not-yet-existent mechanism. > On 0003: > > I think you should make this work for all AMs, not just btree, and > then consolidate it with 0002. OK. 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