Re: [PROPOSAL] VACUUM Progress Checker.

Hi Vinayak,

Thanks for updating the patch, a couple of comments:

On 2016/02/05 17:15, pokurev(at)pm(dot)nttdata(dot)co(dot)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