On Sat, Jan 27, 2018 at 2:50 AM, Robert Haas <robertmh...@gmail.com> wrote: > On Tue, Jan 2, 2018 at 6:38 AM, Amit Kapila <amit.kapil...@gmail.com> wrote: >> [ new patch ] > > I think that grouping_planner() could benefit from a slightly more > extensive rearrangement. With your patch applied, the order of > operations is: > > 1. compute the scan/join target > 2. apply the scan/join target to all paths in current_rel's pathlist > 3. generate gather paths, possibly adding more stuff to current_rel's pathlist > 4. rerun set_cheapest > 5. apply the scan/join target, if parallel safe, to all paths in the > current rel's partial_pathlist, for the benefit of upper planning > steps > 6. clear the partial pathlist if the target list is not parallel safe > > I at first thought this was outright broken because step #3 imposes > the scan/join target without testing it for parallel-safety, but then > I realized that generate_gather_paths will apply that target list by > using apply_projection_to_path, which makes an is_parallel_safe test > of its own. But it doesn't seem good for step 3 to test the > parallel-safety of the target list separately for each path and then > have grouping_planner do it one more time for the benefit of upper > planning steps. Instead, I suggest that we try to get rid of the > logic in apply_projection_to_path that knows about Gather and Gather > Merge specifically. I think we can do that if grouping_planner does > this: > > 1. compute the scan/join target > 2. apply the scan/join target, if parallel safe, to all paths in the > current rel's partial_pathlist > 3. generate gather paths > 4. clear the partial pathlist if the target list is not parallel safe > 5. apply the scan/join target to all paths in current_rel's pathlist > 6. rerun set_cheapest > > That seems like a considerably more logical order of operations. It > avoids not only the expense of testing the scanjoin_target for > parallel-safety multiple times, but the ugliness of having > apply_projection_to_path know about Gather and Gather Merge as a > special case. >
If we want to get rid of Gather (Merge) checks in apply_projection_to_path(), then we need some way to add a projection path to the subpath of gather node even if that is capable of projection as we do now. I think changing the order of applying scan/join target won't address that unless we decide to do it for every partial path. Another way could be that we handle that in generate_gather_paths, but I think that won't be the idle place to add projection. If we want, we can compute the parallel-safety of scan/join target once in grouping_planner and then pass it in apply_projection_to_path to address your main concern. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
