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
