Re: [PROPOSAL] VACUUM Progress Checker.
От | Amit Langote |
---|---|
Тема | Re: [PROPOSAL] VACUUM Progress Checker. |
Дата | |
Msg-id | 56B7FF5D.7030108@lab.ntt.co.jp обсуждение исходный текст |
Ответ на | Re: [PROPOSAL] VACUUM Progress Checker. (<pokurev@pm.nttdata.co.jp>) |
Ответы |
Re: [PROPOSAL] VACUUM Progress Checker.
(Alvaro Herrera <alvherre@2ndquadrant.com>)
Re: [PROPOSAL] VACUUM Progress Checker. (Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>) |
Список | pgsql-hackers |
Hi Vinayak, Thanks for updating the patch, a couple of comments: On 2016/02/05 17:15, pokurev@pm.nttdata.co.jp wrote: > Hello, > > Please find attached updated patch. >> The point of having pgstat_report_progress_update_counter() is so that >> you can efficiently update a single counter without having to update >> everything, when only one counter has changed. But here you are >> calling this function a whole bunch of times in a row, which >> completely misses the point - if you are updating all the counters, >> it's more efficient to use an interface that does them all at once >> instead of one at a time. > > The pgstat_report_progress_update_counter() is called at appropriate places in the attached patch. + char progress_message[N_PROGRESS_PARAM][PROGRESS_MESSAGE_LENGTH]; [ ... ] + snprintf(progress_message[0], PROGRESS_MESSAGE_LENGTH, "%s", phase1); + pgstat_report_progress_update_message(0, progress_message); [ ... ] + snprintf(progress_message[0], PROGRESS_MESSAGE_LENGTH, "%s", phase2); + pgstat_report_progress_update_message(0, progress_message); Instead of passing the array of char *'s, why not just pass a single char *, because that's what it's doing - updating a single message. So, something like: + char progress_message[PROGRESS_MESSAGE_LENGTH]; [ ... ] + snprintf(progress_message, PROGRESS_MESSAGE_LENGTH, "%s", phase1); + pgstat_report_progress_update_message(0, progress_message); [ ... ] + snprintf(progress_message, PROGRESS_MESSAGE_LENGTH, "%s", phase2); + pgstat_report_progress_update_message(0, progress_message); And also: +/*----------- + * pgstat_report_progress_update_message()- + * + *Called to update phase of VACUUM progress + *----------- + */ +void +pgstat_report_progress_update_message(int index, char *msg) +{ [ ... ] + pgstat_increment_changecount_before(beentry); + strncpy((char *)beentry->st_progress_message[index], msg, PROGRESS_MESSAGE_LENGTH); + pgstat_increment_changecount_after(beentry); One more comment: @@ -1120,14 +1157,23 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats, /* Log cleanup info before we touch indexes */ vacuum_log_cleanup_info(onerel, vacrelstats); + snprintf(progress_message[0], PROGRESS_MESSAGE_LENGTH, "%s", phase2); + pgstat_report_progress_update_message(0, progress_message); /* Remove index entries */ for (i = 0;i < nindexes; i++) + { lazy_vacuum_index(Irel[i], &indstats[i], vacrelstats); + scanned_index_pages += RelationGetNumberOfBlocks(Irel[i]); + /* Update the scanned index pages and number of index scan */ + pgstat_report_progress_update_counter(3, scanned_index_pages); + pgstat_report_progress_update_counter(4, vacrelstats->num_index_scans + 1); + } /* Remove tuples from heap */ lazy_vacuum_heap(onerel, vacrelstats); vacrelstats->num_index_scans++; + scanned_index_pages = 0; I guess num_index_scans could better be reported after all the indexes are done, that is, after the for loop ends. Thanks, Amit
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Filip RembiałkowskiДата:
Сообщение: Re: proposal: make NOTIFY list de-duplication optional
Следующее
От: Kouhei KaigaiДата:
Сообщение: Re: Way to check whether a particular block is on the shared_buffer?