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