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
- parallel_vacuum_refactor_v4.patch (application/octet-stream) patch v4
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
-
Move parallel vacuum code to vacuumparallel.c.
- 8e1fae193864 15.0 landed
-
Move index vacuum routines to vacuum.c.
- cc8b25712b5e 15.0 landed
-
Improve parallel vacuum implementation.
- 22bd3cbe0c28 15.0 landed
-
Fix parallel amvacuumcleanup safety bug.
- c59278a1aa5e 15.0 landed
- f6162c020c88 14.1 landed
-
Simplify state managed by VACUUM.
- b4af70cb2103 14.0 cited