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

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?

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?

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.



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

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.

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.

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

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