On Wed, Jan 3, 2018 at 9:11 AM, Peter Geoghegan <p...@bowt.ie> wrote:
> On Tue, Jan 2, 2018 at 1:38 AM, Rushabh Lathia <rushabh.lat...@gmail.com> > wrote: > > Need to do after the indexRelation build. So I added after update of > > pg_index, > > as indexRelation needed for plan_create_index_worders(). > > > > Attaching the separate patch the same. > > This made it so that REINDEX and CREATE INDEX CONCURRENTLY no longer > used parallelism. I think we need to do this very late, just before > nbtree's ambuild() routine is called from index.c. > > Ahh right. We should move the plan_create_index_workers() call to index_build() before the ambuild(). > > So you suggesting that need to do adjustment with the output of > > compute_parallel_worker() by considering parallel_leader_participation? > > We know for sure that there is no reason to not use the leader process > as a worker process in the case of parallel CREATE INDEX. So we must > not have the number of participants (i.e. worker Tuplesortstates) vary > based on the current parallel_leader_participation setting. While > parallel_leader_participation can affect the number of worker > processes requested, that's a different thing. There is no question > about parallel_leader_participation ever being relevant to performance > -- it's strictly a testing option for us. > > Even after parallel_leader_participation was added, > compute_parallel_worker() still assumes that the sequential scan > leader is always too busy to help. compute_parallel_worker() seems to > think that that's something that the leader does in "rare" cases not > worth considering -- cases where it has no worker tuples to consume > (maybe I'm reading too much into it not caring about > parallel_leader_participation, but I don't think so). If > compute_parallel_worker()'s assumption was questionable before, it's > completely wrong for parallel CREATE INDEX. I think > plan_create_index_workers() needs to count the leader-as-worker as an > ordinary worker, not special in any way by deducting one worker from > what compute_parallel_worker() returns. (This only happens when it's > necessary to compensate -- when leader-as-worker participation is > going to go ahead.) > > Yes, event with parallel_leader_participation - compute_parallel_worker() doesn't take that into consideration. Or may be the assumption is that launch the number of workers return by the compute_parallel_worker(), irrespective of whether leader is going to participate in a scan or not. I agree that plan_create_index_workers() needs to count the leader as a normal worker for the CREATE INDEX. So what you proposing is - when parallel_leader_participation is true launch (return value of compute_parallel_worker() - 1) workers. true ? I'm working on fixing up what you posted. I'm probably not more than a > week away from posting a patch that I'm going to mark "ready for > committer". I've already made the change above, and once I spend time > on trying to break the few small changes needed within buffile.c I'll > have taken it as far as I can, most likely. > > Okay, once you submit the patch with changes - I will do one round of review for the changes. Thanks, Rushabh Lathia www.EnterpriseDB.com
