On Fri, Nov 19, 2021 at 7:55 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> 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.
>
>
> 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 ?
>
One more point related to this is that it seems a new callback will be
invoked only by parallel workers, so the context displayed during
parallel vacuum will be different based on if the error happens during
processing by leader or worker. I think if done correctly this would
be an improvement over what we have now but isn't it better to do this
change as a separate patch?
>
> 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.
>
BTW, do we really need functions
perform_parallel_index_bulkdel|cleanup? Both do some minimal
assignments and then call parallel_vacuum_all_indexes() and there is
just one caller of each. Isn't it better to just do those assignments
in the caller and directly call parallel_vacuum_all_indexes()?
In general, we are not following the convention to start the function
names with parallel_* at other places so I think we should consider
such a convention on case to case basis. In this case, if we can get
rid of perform_parallel_index_bulkdel|cleanup then we probably don't
need such a renaming.
--
With Regards,
Amit Kapila.