Re: [PROPOSAL] VACUUM Progress Checker. - Mailing list 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