Re: [PROPOSAL] VACUUM Progress Checker. - Mailing list pgsql-hackers

Hello Fujii-san,

>Here are another review comments
Thank you for review. Please find attached an updated patch.

> You removed some empty lines, for example, in vacuum.h.
>Which seems useless to me.
Has been corrected in the attached.

>Why did you use an array to store the progress information of VACUUM?
>I think that it's better to use separate specific variables for them for better code readability, for example,
variablesscanned_pages, heap_total_pages, etc. 
It is designed this way to keep it generic for all the commands which can store different progress parameters in shared
memory.

>Currently only progress_param_float[0] is used. So there is no need to use an array here.
Same as before . This is for later use by other commands.

>progress_param_float[0] saves the percetage of VACUUM progress.
>But why do we need to save that information into shared memory?
>We can just calculate the percentage whenever pg_stat_get_vacuum_progress() is executed, instead. There seems to be no
needto save that information. 
This has been corrected in the attached.

>char progress_message[PROGRESS_MESSAGE_LENGTH][N_PROGRESS_PARAM];
>As Sawada pointed out, there is no user of this variable.
Have used it to store table name in the updated patch. It can also be used to display index names, current phase of
VACUUM.  
This has not been included in the patch yet to avoid cluttering the display with too much information.

>For example, st_activity of autovacuum worker displays "autovacuum ...".
>So as Sawada reported, he could not find any entry for autovacuum in pg_stat_vacuum_progress.
In the attached patch , I have performed check for autovacuum also.

>I think that you should add the flag or something which indicates whether this backend is running VACUUM or not, into
PgBackendStatus.
>pg_stat_vacuum_progress should display the entries of only backends with that flag set true. This design means that
youneed to set the flag to true when starting VACUUM and reset at the end of VACUUM progressing. 
This design seems better in the sense that we don’t rely on st_activity entry to display progress values.
A variable which stores flags for running commands can be created in PgBackendStatus. These flags can be used at the
timeof display of progress of particular command.  

>That is, non-superuser should not be allowed to see the pg_stat_vacuum_progress entry of superuser running VACUUM?
This has been included in the updated patch.

>This code may cause total_pages and total_heap_pages to decrease while VACUUM is running.
Yes. This is because the initial count of total pages to be vacuumed and the pages which are actually vacuumed can vary
dependingon visibility of tuples. 
The pages which are all visible are skipped and hence have been subtracted from total page count.


Thank you,
Rahila Syed

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.