On Fri, Nov 19, 2021 at 7:55 AM houzj.f...@fujitsu.com <houzj.f...@fujitsu.com> wrote: > > On Tues, Nov 16, 2021 1:53 PM Masahiko Sawada <sawada.m...@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.
