On Mon, Mar 23, 2020 at 10:34 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Sun, Mar 22, 2020 at 7:48 AM Justin Pryzby <pry...@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
0001-Comments-and-doc-fixes-for-commit-40d964ec99.patch
Description: Binary data