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, variables scanned_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 need to save that information.
This has been corrected in the attached.

>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 
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 you need 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 time of 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 depending on 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.

Attachment: Vacuum_progress_checker_v4.patch
Description: Vacuum_progress_checker_v4.patch

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to