Re: [HACKERS] Block level parallel vacuum

On Mon, Dec 30, 2019 at 6:46 PM Tomas Vondra
<tomas.vondra@2ndquadrant.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.vondra@2ndquadrant.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?

> >> >>
> >> >>
> >> >> v40-0004-Add-ability-to-disable-leader-participation-in-p.patch
> >> >> ---------------------------------------------------------------
> >> >>
> >> >> IMHO this should be simply merged into 0002.
> >> >
> >> >We discussed it's still unclear whether we really want to commit this
> >> >code and therefore it's separated from the main part. Please see more
> >> >details here[2].
> >> >
> >>
> >> IMO there's not much reason for the leader not to participate.
> >>
> >
> >The only reason for this is just a debugging/testing aid because
> >during the development of other parallel features we required such a
> >knob.  The other way could be to have something similar to
> >force_parallel_mode and there is some discussion about that as well on
> >this thread but we haven't concluded which is better.  So, we decided
> >to keep it as a separate patch which we can use to test this feature
> >during development and decide later whether we really need to commit
> >it.  BTW, we have found few bugs by using this knob in the patch.
> >
>
> OK, understood. Then why not just use force_parallel_mode?
>

Because we are not sure what should be its behavior under different
modes especially what should we do when user set force_parallel_mode =
on.  We can even consider introducing new guc specific for this, but
as of now, I am not convinced that is required.  See some more
discussion around this parameter in emails [1][2].  I think we can
decide on this later (probably once the main patch is committed) as we
already have one way to test the patch.

[1] - https://www.postgresql.org/message-id/CAFiTN-sUuLASVXm2qOjufVH3tBZHPLdujMJ0RHr47Tnctjk9YA%40mail.gmail.com
[2] -
https://www.postgresql.org/message-id/CA%2Bfd4k6VgA_DG%3D8%3Dui7UvHhqx9VbQ-%2B72X%3D_GdTzh%3DJ_xN%2BVEg%40mail.gmail.com

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