Hi Vinayak,
Thanks for updating the patch, a couple of comments:
On 2016/02/05 17:15, poku...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
