Re: [HACKERS] Block level parallel vacuum

On Sat, Jan 11, 2020 at 9:23 AM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Fri, 10 Jan 2020 at 20:54, Mahendra Singh Thalor <mahi6run@gmail.com> wrote:
> >
> > On Fri, 10 Jan 2020 at 15:51, Sergei Kornilov <sk@zsrv.org> wrote:
> > >
> > > Hi
> > > Thank you for update! I looked again
> > >
> > > (vacuum_indexes_leader)
> > > +               /* Skip the indexes that can be processed by parallel workers */
> > > +               if (!skip_index)
> > > +                       continue;
> > >
> > > Does the variable name skip_index not confuse here? Maybe rename to something like can_parallel?
> >
> > I also agree with your point.
>
> I don't think the change is a good idea.
>
> -               bool            skip_index = (get_indstats(lps->lvshared, i) == NULL ||
> -                                                                 skip_parallel_vacuum_index(Irel[i],
lps->lvshared));
> +               bool            can_parallel = (get_indstats(lps->lvshared, i) == NULL ||
> +                                                                       skip_parallel_vacuum_index(Irel[i],
> +
    lps->lvshared)); 
>
> The above condition is true when the index can *not* do parallel index vacuum. How about changing it to skipped_index
andchange the comment to something like “We are interested in only index skipped parallel vacuum”? 
>

Hmm, I find the current code and comment better than what you or
Sergei are proposing.  I am not sure what is the point of confusion in
the current code?

> >
> > >
> > > Another question about behavior on temporary tables. Use case: the user commands just "vacuum;" to vacuum entire
database(and has enough maintenance workers). Vacuum starts fine in parallel, but on first temporary table we hit: 
> > >
> > > +       if (RelationUsesLocalBuffers(onerel) && params->nworkers >= 0)
> > > +       {
> > > +               ereport(WARNING,
> > > +                               (errmsg("disabling parallel option of vacuum on \"%s\" --- cannot vacuum
temporarytables in parallel", 
> > > +                                               RelationGetRelationName(onerel))));
> > > +               params->nworkers = -1;
> > > +       }
> > >
> > > And therefore we turn off the parallel vacuum for the remaining tables... Can we improve this case?
> >
> > Good point.
> > Yes, we should improve this. I tried to fix this.
>
> +1
>

Yeah, we can improve the situation here.  I think we don't need to
change the value of params->nworkers at first place if allow
lazy_scan_heap to take care of this.  Also, I think we shouldn't
display warning unless the user has explicitly asked for parallel
option.  See the fix in the attached patch.

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