RE: parallel vacuum comments

On Tues, Nov 16, 2021 1:53 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> I've incorporated these comments and attached an updated patch.

Thanks for updating the patch.
I read the latest patch and have few comments.

1)
+/*
+ *    lazy_vacuum_one_index() -- vacuum index relation.
...
+IndexBulkDeleteResult *
+vacuum_one_index(IndexVacuumInfo *ivinfo, IndexBulkDeleteResult *istat,


+ *    vac_cleanup_one_index() -- do post-vacuum cleanup for index relation.
...
+IndexBulkDeleteResult *
+cleanup_one_index(IndexVacuumInfo *ivinfo, IndexBulkDeleteResult *istat)

The above function names seem different from the name mentioned in the function
header.

2)
 static void vacuum_error_callback(void *arg);

I noticed the patch changed the parallel worker's error callback function to
parallel_index_vacuum_error_callback(). The error message in new callback
function seems a little different from the old one, was it intentional ?


3)

+    /*
+     * Reset all index status back to invalid (while checking that we have
+     * processed all indexes).
+     */
+    for (int i = 0; i < pvs->nindexes; i++)
+    {
+        PVIndStats *stats = &(pvs->indstats[i]);
+
+        Assert(stats->status == INDVAC_STATUS_COMPLETED);
+        stats->status = INDVAC_STATUS_INITIAL;
+    }

Would it be safer if we report an error if any index's status is not
INDVAC_STATUS_COMPLETED ?

4)

Just a personal suggestion for the parallel related function name. Since Andres
wanted a uniform naming pattern. Mabe we can rename the following functions:

end|begin_parallel_vacuum => parallel_vacuum_end|begin
perform_parallel_index_bulkdel|cleanup => parallel_vacuum_index_bulkdel|cleanup

So that all the parallel related functions' name is like parallel_vacuum_xxx.

Best regards,
Hou zj