Re: [HACKERS] Block level parallel vacuum

g_indg_On Mon, 23 Dec 2019 at 16:11, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
>
> On Fri, Dec 20, 2019 at 12:13 PM Masahiko Sawada
> <masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:
> >
> > I've attached the updated version patch that incorporated the all
> > review comments I go so far.
> >
>
> I have further edited the first two patches posted by you. The
> changes include (a) changed tests to reset the guc, (b) removing some
> stuff which is not required in this version, (c) moving some variables
> around to make them in better order, (d) changed comments and few
> other cosmetic things and (e) commit messages for first two patches.
>
> I think the first two patches attached in this email are in good shape
> and we can commit those unless you or someone has more comments on
> them, the main parallel vacuum patch can still be improved by some
> more test/polish/review. I am planning to push the first two patches
> next week after another pass. The first two patches are explained in
> brief as below:
>
> 1. v4-0001-Delete-empty-pages-in-each-pass-during-GIST-VACUUM: It
> allows us to delete empty pages in each pass during GIST VACUUM.
> Earlier, we use to postpone deleting empty pages till the second stage
> of vacuum to amortize the cost of scanning internal pages. However,
> that can sometimes (say vacuum is canceled or errored between first
> and second stage) delay the pages to be recycled. Another thing is
> that to facilitate deleting empty pages in the second stage, we need
> to share the information of internal and empty pages between different
> stages of vacuum. It will be quite tricky to share this information
> via DSM which is required for the main parallel vacuum patch. Also,
> it will bring the logic to reclaim deleted pages closer to nbtree
> where we delete empty pages in each pass. Overall, the advantages of
> deleting empty pages in each pass outweigh the advantages of
> postponing the same. This patch is discussed in detail in a separate
> thread [1].
>
> 2. v39-0001-Introduce-IndexAM-fields-for-parallel-vacuum.patch:
> Introduce new fields amusemaintenanceworkmem and
> amparallelvacuumoptions in IndexAmRoutine for parallel vacuum. The
> amusemaintenanceworkmem tells whether a particular IndexAM uses
> maintenance_work_mem or not. This will help in controlling the memory
> used by individual workers as otherwise, each worker can consume
> memory equal to maintenance_work_mem. This has been discussed in
> detail in a separate thread as well [2]. The amparallelvacuumoptions
> tell whether a particular IndexAM participates in a parallel vacuum
> and if so in which phase (bulkdelete, vacuumcleanup) of vacuum.
>
>
> [1] -
https://www.postgresql.org/message-id/CAA4eK1LGr%2BMN0xHZpJ2dfS8QNQ1a_aROKowZB%2BMPNep8FVtwAA%40mail.gmail.com
> [2] -
https://www.postgresql.org/message-id/CAA4eK1LmcD5aPogzwim5Nn58Ki+74a6Edghx4Wd8hAskvHaq5A@mail.gmail.com
>

Hi,
I reviewed v39 patch set. Below are the some minor review comments:

1.
+ * memory equal to maitenance_work_mem, the new maitenance_work_mem for

maitenance_work_mem should be replaced by maintenance_work_mem.

2.
+ * The number of workers can vary between and bulkdelete and cleanup

I think, grammatically above sentence is not correct. "and" is extra in
above sentence.

3.
+ /*
+ * Open table. The lock mode is the same as the leader process. It's
+ * okay because The lockmode does not conflict among the parallel workers.
+ */

I think, "lock mode" and "lockmode", both should be same.(means extra space
should be removed from "lock mode"). In "The", "T" should be small case
letter.

4.
+ /* We don't support parallel vacuum for autovacuum for now */

I think, above sentence should be like "As of now, we don't support
parallel vacuum for autovacuum"

5. I am not sure that I am right but I can see that we are not consistent
while ending the single line comments.

I think, if single line comment is started with "upper case letter", then
we should not put period(dot) at the end of comment, but if comment started
with "lower case letter", then we should put period(dot) at the end of
comment.

a)
+ /* parallel vacuum must be active */
I think. we should end above comment with dot or we should make "p" of
parallel as upper case letter.

b)
+ /* At least count itself */
I think, above is correct.

If my understanding is correct, then please let me know so that I can make
these changes on the top of v39 patch set.

6.
+ bool amusemaintenanceworkmem;

I think, we haven't ran pgindent.

Thanks and Regards
Mahendra Thalor
EnterpriseDB: http://www.enterprisedb.com