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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
