On Tue, 7 Apr 2020 at 13:55, Justin Pryzby <pry...@telsasoft.com> wrote: > > 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.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. > > 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. >
I don't have comments on your change other than the comments Amit already sent. Thank you for reviewing this part! Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
