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