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"

  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.

Regards,
Masanori Ohyama
NTT Open Source Software Center

2016年2月27日(土) 13:54 Vinayak Pokale <vinpok...@gmail.com>:

> Hello,
>
> On Fri, Feb 26, 2016 at 6:19 PM, Amit Langote <
> langote_amit...@lab.ntt.co.jp> wrote:
>
>>
>> Hi Vinayak,
>>
>> Thanks for updating the patch! A quick comment:
>>
>> On 2016/02/26 17:28, poku...@pm.nttdata.co.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
>
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Reply via email to