RE: parallel vacuum comments

On Tues, Nov 16, 2021 1:53 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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