Re: doc review for parallel vacuum

On Tue, Apr 7, 2020 at 10:25 AM Justin Pryzby <pryzby(at)telsasoft(dot)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(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.
>
> Probably it should say "index vacuum and index cleanup", which is also what the
> comment in vacuumlazy.c says.
>

Okay, that makes sense.

>
> > > - * 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" ?
>

It means the "at the time when" worker performed any I/O. This has
been used at some other place in code as well.

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

- The <option>PARALLEL</option> option is used only for vacuum purpose.
- Even if this option is specified with <option>ANALYZE</option> option
- it does not affect <option>ANALYZE</option>.
+ The <option>PARALLEL</option> option is used only for vacuum operations.
+ If specified along with <option>ANALYZE</option>, the behavior during
+ <literal>ANALYZE</literal> is unchanged.

I think the proposed text makes the above text unclear especially "the
behavior during ANALYZE is unchanged.". Basically this option has
nothing to do with the behavior of vacuum or analyze. I think we
should be more specific as the current text.

* Copy the index bulk-deletion result returned from ambulkdelete and
- * amvacuumcleanup to the DSM segment if it's the first time to get it
- * from them, because they allocate it locally and it's possible that an
- * index will be vacuumed by the different vacuum process at the next
- * time. The copying of the result normally happens only after the first
- * time of index vacuuming. From the second time, we pass the result on
- * the DSM segment so that they then update it directly.
+ * amvacuumcleanup to the DSM segment if it's the first time to get a result
+ * from a worker, because workers allocate BulkDeleteResults locally,
and it's possible that an
+ * index will be vacuumed by a different vacuum process the next
+ * time.

This can be done by the leader backend as well, so we can't use
workers terminology here. Also, I don't see the need to mention
BulkDeleteResults. I will include some changes from this text.

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