Re: [HACKERS] Block level parallel vacuum

On Tue, Dec 31, 2019 at 9:09 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Dec 30, 2019 at 6:46 PM Tomas Vondra
> <tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
> >
> > On Mon, Dec 30, 2019 at 08:25:28AM +0530, Amit Kapila wrote:
> > >On Mon, Dec 30, 2019 at 2:53 AM Tomas Vondra
> > ><tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
> > >> I think there's another question we need to ask - why to we introduce a
> > >> bitmask, instead of using regular boolean struct members? Until now, the
> > >> IndexAmRoutine struct had simple boolean members describing capabilities
> > >> of the AM implementation. Why shouldn't this patch do the same thing,
> > >> i.e. add one boolean flag for each AM feature?
> > >>
> > >
> > >This structure member describes mostly one property of index which is
> > >about a parallel vacuum which I am not sure is true for other members.
> > >Now, we can use separate bool variables for it which we were initially
> > >using in the patch but that seems to be taking more space in a
> > >structure without any advantage. Also, using one variable makes a
> > >code bit better because otherwise, in many places we need to check and
> > >set four variables instead of one. This is also the reason we used
> > >parallel in its name (we also use *parallel* for parallel index scan
> > >related things). Having said that, we can remove parallel from its
> > >name if we want to extend/use it for something other than a parallel
> > >vacuum. I think we might need to add a flag or two for parallelizing
> > >heap scan of vacuum when we enhance this feature, so keeping it for
> > >just a parallel vacuum is not completely insane.
> > >
> > >I think keeping amusemaintenanceworkmem separate from this variable
> > >seems to me like a better idea as it doesn't describe whether IndexAM
> > >can participate in a parallel vacuum or not. You can see more
> > >discussion about that variable in the thread [1].
> > >
> >
> > I don't know, but IMHO it's somewhat easier to work with separate flags.
> > Bitmasks make sense when space usage matters a lot, e.g. for on-disk
> > representation, but that doesn't seem to be the case here I think (if it
> > was, we'd probably use bitmasks already).
> >
> > It seems like we're mixing two ways to design the struct unnecessarily,
> > but I'm not going to nag about this any further.
> >
>
> Fair enough. I see your point and as mentioned earlier that we
> started with the approach of separate booleans, but later found that
> this is a better way as it was easier to set and check the different
> parallel options for a parallel vacuum. I think we can go back to
> the individual booleans if we want but I am not sure if that is a
> better approach for this usage. Sawada-San, others, do you have any
> opinion here?
IMHO, having multiple bools will be confusing compared to what we have
now because these are all related to enabling parallelism for
different phases of the vacuum. So it makes more sense to keep it as
a single variable with multiple options.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com