Re: [HACKERS] Block level parallel vacuum

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

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.

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


regards

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