Re: doc review for parallel vacuum - Mailing list pgsql-hackers

On Mon, Mar 23, 2020 at 10:34 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sun, Mar 22, 2020 at 7:48 AM Justin Pryzby <pryzby@telsasoft.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