Re: doc review for parallel vacuum

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.

> 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.

> 3.
> ..
> - vacuum launches before starting each phase and exit at the end of
> + vacuum are launched before the start of each phase and
> terminate at the end of
>
> It is better to use 'exit' instead of 'ternimate' as we are not
> forcing the workers to end rather we wait for them to exit.
>

I have used 'exit' instead of 'terminate' here.

>
>
> 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.
>
> 2.
> - * Since writes are not allowed during the parallel mode, so we copy the
> + * Since writes are not allowed during parallel mode, copy the
>
> Similar to above, I think here also we should have a determiner before
> 'parallel mode'.
>

Changed as per your suggestion.

> 3.
> - * The basic idea of a cost-based vacuum delay for parallel vacuum is to allow
> - * each worker to sleep proportional to the work done by it. We achieve this
> + * The basic idea of a cost-based delay for parallel vacuum is to force
> + * each worker to sleep in proportion to the share of work it's done.
> We achieve this
>
> I am not sure if it is an improvement to use 'to force' instead of 'to
> allow' because there is another criteria as well which decides whether
> the worker will sleep or not. I am also not sure if the second change
> (share of work it's) in this sentence is a clear improvement.
>

I have used 'to allow' in above text, otherwise, acceptted your suggestions.

> 4.
> - * 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.

> 5.
> - * We allow any worker to sleep only if it has performed the I/O above a
> + * We force a worker to sleep only if it has performed I/O above a
> * certain threshold
>
> Hmm, again I am not sure if we should use 'force' here instead of 'allow'.
>

Kept the usage of 'allow'.

I have combined both of your patches. Let me know if you have any
more suggestions, otherwise, I am planning to push this tomorrow.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com