From: | Justin Pryzby <pryzby(at)telsasoft(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com>, Mahendra Singh Thalor <mahi6run(at)gmail(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Sergei Kornilov <sk(at)zsrv(dot)org>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: doc review for parallel vacuum |
Date: | 2020-04-07 04:55:51 |
Message-ID: | 20200407045551.GI2228@telsasoft.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Apr 07, 2020 at 09:57:46AM +0530, Amit Kapila wrote:
> On Mon, Mar 23, 2020 at 10:34 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > On Sun, Mar 22, 2020 at 7:48 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> > >
> > > Original, long thread
> > > https://www.postgresql.org/message-id/flat/CAA4eK1%2Bnw1FBK3_sDnW%2B7kB%2Bx4qbDJqetgqwYW8k2xv82RZ%2BKw%40mail.gmail.com#b1745ee853b137043e584b500b41300f
> > >
> >
> > I have comments/questions on the patches:
> > doc changes
> > -------------------------
> > 1.
> > <para>
> > - Perform vacuum index and cleanup index phases of <command>VACUUM</command>
> > + Perform vacuum index and index cleanup phases of <command>VACUUM</command>
> >
> > Why is the proposed text improvement over what is already there?
>
> I have kept the existing text as it is for this case.
Probably it should say "index vacuum and index cleanup", which is also what the
comment in vacuumlazy.c says.
> > 2.
> > If the
> > - <literal>PARALLEL</literal> option is omitted, then
> > - <command>VACUUM</command> decides the number of workers based on number
> > - of indexes that support parallel vacuum operation on the relation which
> > - is further limited by <xref
> > linkend="guc-max-parallel-workers-maintenance"/>.
> > - The index can participate in a parallel vacuum if and only if the size
> > + <literal>PARALLEL</literal> option is omitted, then the number of workers
> > + is determined based on the number of indexes that support parallel vacuum
> > + operation on the relation, and is further limited by <xref
> > + linkend="guc-max-parallel-workers-maintenance"/>.
> > + An index can participate in parallel vacuum if and only if the size
> > of the index is more than <xref
> > linkend="guc-min-parallel-index-scan-size"/>.
> >
> > Here, it is not clear to me why the proposed text is better than what
> > we already have?
> Changed as per your suggestion.
To answer your question:
"VACUUM decides" doesn't sound consistent with docs.
"based on {+the+} number"
=> here, you're veritably missing an article...
"relation which" technically means that the *relation* is "is further limited
by"...
> > Patch changes
> > -------------------------
> > 1.
> > - * and index cleanup with parallel workers unless the parallel vacuum is
> > + * and index cleanup with parallel workers unless parallel vacuum is
> >
> > As per my understanding 'parallel vacuum' is a noun phrase, so we
> > should have a determiner before it.
>
> Changed as per your suggestion.
Thanks (I think you meant an "article").
> > - * We allow each worker to update it as and when it has incurred any cost and
> > + * We allow each worker to update it AS AND WHEN it has incurred any cost and
> >
> > I don't see why it is necessary to make this part bold? We are using
> > it at one other place in the code in the way it is used here.
> >
>
> Kept the existing text as it is.
I meant this as a question. I'm not sure what "as and when means" ? "If and
when" ?
> I have combined both of your patches. Let me know if you have any
> more suggestions, otherwise, I am planning to push this tomorrow.
In the meantime, I found some more issues, so I rebased on top of your patch so
you can review it.
--
Justin
Attachment | Content-Type | Size |
---|---|---|
v3-0001-Comments-and-doc-fixes-for-commit-40d964ec99.patch | text/x-diff | 12.1 KB |
v3-0002-docs-comments-review-for-parallel-vacuum.patch | text/x-diff | 6.2 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | vignesh C | 2020-04-07 05:24:28 | Re: Parallel copy |
Previous Message | Tom Lane | 2020-04-07 04:37:24 | Re: backup manifests and contemporaneous buildfarm failures |