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. >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 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.
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: http://www.postgresql.org/mailpref/pgsql-hackers
