Re: [PROPOSAL] VACUUM Progress Checker.

Hello, thank for testing this.

At Sat, 27 Feb 2016 17:19:05 +0000, 大山真実 <oyama(dot)masanori(dot)1987(at)gmail(dot)com> wrote in <CAJ_V8TmJG0Z8RPpN9DhTLEnfDxWEUoBQXQRQvQ7mbE47y3X9+Q(at)mail(dot)gmail(dot)com>
> Hi!
>
> I'm interesting this patch and tested it. I found two strange thing.
>
> * Incorrect counting
>
> Reproduce:
> 1. Client1 execute "VACUUM"
> 2. Client2 execute "VACUUM"
> 3. Client3 execute "SELECT * FROM pg_stat_vacuum_progress".
> pid | relid | phase | total_heap_blks | current_heap_blkno |
> total_index_pages | scanned_index_pages | index_scan_count |
> percent_complete
> ------+-------+---------------+-----------------+--------------------+-------------------+---------------------+------------------+------------------
> 9267 | 16551 | Scanning Heap | 164151 | 316 |
> 27422 | 7 | 1 | 0
> 9764 | 16554 | Scanning Heap | 2 | 2 |
> 2 | 27422 | 1 | 100
> (2 rows)
>
> Client2 is waiting for Clinet1 "VACUUM" but percent_complete of Client2
> "VACUUM" is 100.
> * Not end VACUUM ANALYZE in spite of "percent_complete=100"

The inidividual record is telling about *one* relation now under
vacuuming (or just after the processing), not about all relations
to be vacuumed as a whole. It is the specification of this patch
for now. However it cannot tell how long the invoker should wait
for the vauum to end, it seems to be way difficult to calculate
statistics against the all relations to be processed.

Anyway other status messages such as "Waiting for XXXX" would be
necessary.

> Client_1 execute "VACUUM ANALYZE", then Client_2 execute "SELECT * FROM
> pg_stat_vacuum_progress".
>
> pid | relid | phase | total_heap_blks | current_heap_blkno |
> total_index_pages | scanned_index_pages | index_scan_count |
> percent_complete
> ------+-------+---------------+-----------------+--------------------+-------------------+---------------------+------------------+------------------
> 9277 | 16551 | Scanning Heap | 163935 | 163935 |
> 27422 | 7 | 1 | 100
> (1 row
>
> percent_complete is 100 but Client_1 "VACUUM ANALYZE" do not response yet.
>
> Of course, Client_1 is executing analyze after vacuum. But it seem to me
> that this confuses users.
> If percent_complete becomes 100 that row should be deleted quickly.

Maybe some works other than vacuuming pages is performing or
waiting a lock to be acquired. If it is a matter of progress, it
should be counted in the progress, but not for something like
waiting for a lock. It is a matter of status messages.

> Regards,
> Masanori Ohyama
> NTT Open Source Software Center
>
> 2016年2月27日(土) 13:54 Vinayak Pokale <vinpokale(at)gmail(dot)com>:
>
> > Hello,
> >
> > On Fri, Feb 26, 2016 at 6:19 PM, Amit Langote <
> > Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> >
> >>
> >> Hi Vinayak,
> >>
> >> Thanks for updating the patch! A quick comment:
> >>
> >> On 2016/02/26 17:28, pokurev(at)pm(dot)nttdata(dot)co(dot)jp wrote:
> >> >> CREATE VIEW pg_stat_vacuum_progress AS
> >> >> SELECT S.s[1] as pid,
> >> >> S.s[2] as relid,
> >> >> CASE S.s[3]
> >> >> WHEN 1 THEN 'Scanning Heap'
> >> >> WHEN 2 THEN 'Vacuuming Index and Heap'
> >> >> ELSE 'Unknown phase'
> >> >> END,
> >> >> ....
> >> >> FROM pg_stat_get_command_progress(PROGRESS_COMMAND_VACUUM) as S;
> >> >>
> >> >> # The name of the function could be other than *_command_progress.
> >> > The name of function is updated as pg_stat_get_progress_info() and also
> >> updated the function.
> >> > Updated the pg_stat_vacuum_progress view as suggested.
> >>
> >> So, pg_stat_get_progress_info() now accepts a parameter to distinguish
> >> different commands. I see the following in its definition:
> >>
> >> + /* Report values for only those backends which are
> >> running VACUUM
> >> command */
> >> + if (cmdtype == COMMAND_LAZY_VACUUM)
> >> + {
> >> + /*Progress can only be viewed by role member.*/
> >> + if (has_privs_of_role(GetUserId(),
> >> beentry->st_userid))
> >> + {
> >> + values[2] =
> >> UInt32GetDatum(beentry->st_progress_param[0]);
> >> + values[3] =
> >> UInt32GetDatum(beentry->st_progress_param[1]);
> >> + values[4] =
> >> UInt32GetDatum(beentry->st_progress_param[2]);
> >> + values[5] =
> >> UInt32GetDatum(beentry->st_progress_param[3]);
> >> + values[6] =
> >> UInt32GetDatum(beentry->st_progress_param[4]);
> >> + values[7] =
> >> UInt32GetDatum(beentry->st_progress_param[5]);
> >> + if (beentry->st_progress_param[1] != 0)
> >> + values[8] =
> >> Float8GetDatum(beentry->st_progress_param[2] * 100 /
> >> beentry->st_progress_param[1]);
> >> + else
> >> + nulls[8] = true;
> >> + }
> >> + else
> >> + {
> >> + nulls[2] = true;
> >> + nulls[3] = true;
> >> + nulls[4] = true;
> >> + nulls[5] = true;
> >> + nulls[6] = true;
> >> + nulls[7] = true;
> >> + nulls[8] = true;
> >> + }
> >> + }
> >>
> >> How about doing this in a separate function which takes the command id as
> >> parameter and returns an array of values and the number of values (per
> >> command id). pg_stat_get_progress_info() then creates values[] and nulls[]
> >> arrays from that and returns that as result set. It will be a cleaner
> >> separation of activities, perhaps.
> >>
> >> +1

Accessing an element out of array safely be NULL and the caller
should know the number of elements, so I prefer one integer (or
bigint?) array to be returned. Or anyway the internal array has
finite number of elements, the function may return an array
exactly reflects the internal.

Last, I found one small bug mentioned above.

+ if (beentry->st_progress_param[1] != 0)
+ values[8] = Float8GetDatum(beentry->st_progress_param[2] * 100 / beentry->st_progress_param[1]);

Float8GetDatum(int/int) cannot have decimal places.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center