Re: [HACKERS] Block level parallel vacuum

On Mon, Dec 30, 2019 at 2:53 AM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
>
> On Sun, Dec 29, 2019 at 10:06:23PM +0900, Masahiko Sawada wrote:
> >> v40-0001-Introduce-IndexAM-fields-for-parallel-vacuum.patch
> >> -----------------------------------------------------------
> >>
> >> I wonder if 'amparallelvacuumoptions' is unnecessarily specific. Maybe
> >> it should be called just 'amvacuumoptions' or something like that? The
> >> 'parallel' part is actually encoded in names of the options.
> >>
> >
> >amvacuumoptions seems good to me.
> >
> >> Also, why do we need a separate amusemaintenanceworkmem option? Why
> >> don't we simply track it using a separate flag in 'amvacuumoptions'
> >> (or whatever we end up calling it)?
> >>
> >
> >It also seems like a good idea.
> >
>
> 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].

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

[1] - https://www.postgresql.org/message-id/CAA4eK1LmcD5aPogzwim5Nn58Ki+74a6Edghx4Wd8hAskvHaq5A@mail.gmail.com

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