From: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Peter Geoghegan <pg(at)bowt(dot)ie>, Andres Freund <andres(at)anarazel(dot)de> |
Subject: | Re: parallel vacuum comments |
Date: | 2021-11-24 02:12:57 |
Message-ID: | CAD21AoCaH=Da0HjqzYioSy3yqic-mpCqnvSRjUUZCrn6iricSA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, Nov 22, 2021 at 1:48 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, Nov 19, 2021 at 7:55 AM houzj(dot)fnst(at)fujitsu(dot)com
> <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> >
> > On Tues, Nov 16, 2021 1:53 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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/
From | Date | Subject | |
---|---|---|---|
Next Message | Mark Dilger | 2021-11-24 02:14:54 | Re: Granting SET and ALTER SYSTE privileges for GUCs |
Previous Message | Andrew Dunstan | 2021-11-24 02:00:53 | Re: Mop-up from Test::More version change patch |