Re: parallel vacuum comments

Masahiko Sawada <sawada.mshk@gmail.com>

From: Masahiko Sawada <sawada.mshk@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>, Peter Geoghegan <pg@bowt.ie>, Andres Freund <andres@anarazel.de>
Date: 2021-11-29T03:38:08Z
Lists: pgsql-hackers

Attachments

On Wed, Nov 24, 2021 at 5:54 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Nov 24, 2021 at 12:16 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Wed, Nov 24, 2021 at 1:34 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, Nov 24, 2021 at 7:43 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > 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:
> > > > >
> > > > > >
> > > > > > 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.
> > > >
> > >
> > > Okay, that makes sense. However, I am still not very comfortable with
> > > the function naming suggested by Hou-San, do you have any thoughts on
> > > that?
> >
> > I personally don't disagree with the names starting with
> > "parallel_vacuum_*".
> >
>
> I don't have any strong opinion here but I prefer the name which makes
> more sense in the context it is being used. OTOH, I see there is an
> argument that it will be easier to follow and might appear consistent
> if we use parallel_vacuum_*.

Maybe we can start with using parallel_vacuum_*. We can change them
later if there is an argument.

I've attached an updated patch. I don't update the terminology in
vacuum that we're discussing on another thread[1].

Regards,

[1] https://www.postgresql.org/message-id/flat/CAH2-WzktGBg4si6DEdmq3q6SoXSDqNi6MtmB8CmmTmvhsxDTLA%40mail.gmail.com

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

Commits

  1. Move parallel vacuum code to vacuumparallel.c.

  2. Move index vacuum routines to vacuum.c.

  3. Improve parallel vacuum implementation.

  4. Fix parallel amvacuumcleanup safety bug.

  5. Simplify state managed by VACUUM.