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

Reply via email to