Re: parallel vacuum comments

On Mon, Nov 22, 2021 at 1:48 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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?

Agreed.

>
> >
> > 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()?

The reason why I declare these two functions are: (1) the fields of
ParallelVacuumState are not exposed and (2) bulk-deletion and cleanup
require different arguments (estimated_count is required only by
cleanup).  So if we expose the fields of ParallelVacuumState, the
caller can do those assignments and directly call
parallel_vacuum_all_indexes(). But I'm not sure it's good if those
assignments are the caller's responsibility.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/