Re: [HACKERS] Block level parallel vacuum

On Tue, 31 Dec 2019 at 12:39, 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?

If we go back to the individual booleans we would end up with having
three booleans: bulkdelete, cleanup and conditional cleanup. I think
making the bulkdelete option to a boolean makes sense but having two
booleans for cleanup and conditional cleanup might be slightly odd
because these options are exclusive. If we don't stick to have only
booleans the having a ternary value for cleanup might be
understandable but I'm not sure it's better to have it for only vacuum
purpose.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services