Re: [PROPOSAL] VACUUM Progress Checker.

Hi,

On 2016/02/16 18:25, Kyotaro HORIGUCHI wrote:
> At Tue, 16 Feb 2016 10:39:27 +0900, Amit Langote wrote:
>> On 2016/02/15 20:21, Kyotaro HORIGUCHI wrote:
>>> CREATE FUNCTION
>>> pg_stat_get_command_progress(IN cmdtype integer)
>>> RETURNS SETOF integer[] as $$....
>>>
>>> SELECT * from pg_stat_get_command_progress(PROGRESS_COMMAND_VACUUM) as x
>>> x
>>> ---------------------_
>>> {1233, 16233, 1, ....}
>>> {3244, 16236, 2, ....}
>>> ....
>>
>> I am not sure what we would pass as argument to the (SQL) function
>> pg_stat_get_command_progress() in the system view definition for
>> individual commands - what is PROGRESS_COMMAND_VACUUM exactly? Would
>> string literals like "vacuum", "cluster", etc. to represent command names
>> work?
>
> Sorry, it is a symbol to tell pg_stat_get_command_progress() to
> return stats numbers of backends running VACUUM. It should have
> been COMMAND_LAZY_VACUUM for this patch. If we want progress of
> CREATE INDEX, it would be COMMAND_CREATE_INDEX.

Oh I see:

CREATE VIEW pg_stat_vacuum_prgress AS
SELECT * from pg_stat_get_command_progress(PROGRESS_COMMAND_VACUUM) as x

is actually:

CREATE VIEW pg_stat_vacuum_prgress AS
SELECT * from pg_stat_get_command_progress(1) as x

where PROGRESS_COMMAND_VACUUM is 1 in backend code (macro, enum,
whatever). I was confused because we never say relkind = RELKIND_INDEX in
SQL queries, :)

>> That said, there is discussion upthread about more precise reporting on
>> index vacuuming by utilizing the lazy_tid_reaped() (the index bulk delete
>> callback) as a place where we can report what index block number we are
>> at. I think that would mean the current IndexBulkDeleteCallback signature
>> is insufficient, which is the following:
>>
>> /* Typedef for callback function to determine if a tuple is bulk-deletable */
>> typedef bool (*IndexBulkDeleteCallback) (ItemPointer itemptr, void *state);
>>
>> One more parameter would be necessary:
>>
>> typedef bool (*IndexBulkDeleteCallback) (ItemPointer itemptr, BlockNumber
>> current_index_blkno, void *state);
>
> It could work for btree but doesn't for, for example,
> gin. ginbulkdelete finds the next page in the following way.
>
>> blkno = GinPageGetOpaque(page)->rightlink;
>
> We should use another value to fagure the progress. If the
> callback is called centainly the same or around the same number
> of times with the total page numbers, the callback should just
> increment a static counter for processed pages.
>
>> That would also require changing all the am specific vacuumpage routines
>> (like btvacuumpage) to also pass the new argument. Needless to say, some
>> bookkeeping information would also need to be kept in LVRelStats (the
>> "state" in above signature).
>>
>> Am I missing something?
>
> So, maybe missing the case of other than btree..

More or less, the callback is called maxoffset number of times for all
index pages containing pointers to heap tuples. Robert said upthread that
counting in granularity lower than pages may not be useful:

"Let's report blocks, not tuples. The reason is that
pg_class.reltuples is only an estimate and might be wildly wrong on
occasion, but the length of the relation in blocks can be known with
certainty."

With the existing interface of the callback, it's difficult to keep the
count of pages, hence a proposal to enhance the interface. Also, now I
wonder whether scanned_index_pages will always converge to whatever
total_index_pages we get from RelationGetNumberOfBlocks(index), because
callback is not called for *every* index page and tends to differ per
index method (am). Thanks for pointing me to confirm so.

Thanks,
Amit