On Tue, Jun 14, 2016 at 4:48 PM, Amit Kapila <amit.kapil...@gmail.com> wrote:
> On Mon, Jun 13, 2016 at 8:11 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: > > > I do > > not share your confidence that using apply_projection_to_path within > > create_grouping_paths is free of such a hazard. > > > > Fair enough, let me try to explain the problem and someways to solve it in > some more detail. The main thing that got missed by me in the patch > related to commit-04ae11f62 is that the partial path list of rel also needs > to have a scanjoin_target. I was under assumption that > create_grouping_paths will take care of assigning appropriate Path targets > for the parallel paths generated by it. If we see, create_grouping_paths() > do take care of adding the appropriate path targets for the paths generated > by that function. However, it doesn't do anything for partial paths. The > patch sent by me yesterday [1] which was trying to assign > partial_grouping_target to partial paths won't be the right fix, because > (a) partial_grouping_target includes Aggregates (refer > make_partialgroup_input_target) which we don't want for partial paths; (b) > it is formed using grouping_target passed in function > create_grouping_paths() whereas we need the path target formed from > final_target for partial paths (as partial paths are scanjoin relations) > as is done for main path list (in grouping_planner(), /* Forcibly apply > that target to all the Paths for the scan/join rel.*/). Now, I think we > have following ways to solve it: > > (a) check whether the scanjoin_target contains any parallel-restricted > clause before applying the same to partial path list in grouping_planner. > However it could lead to duplicate checks in some cases for > parallel-restricted clause, once in apply_projection_to_path() for main > pathlist and then again before applying to partial paths. I think we can > avoid that by having an additional parameter in apply_projection_to_path() > which can indicate if the check for parallel-safety is done inside that > function and what is the result of same. > > (b) generate the appropriate scanjoin_target for partial path list in > create_grouping_paths using final_target. However I think this might lead > to some duplicate code in create_grouping_paths() as we might have to some > thing similar to what we have done in grouping_planner for generating such > a target. I think if we want we can pass scanjoin_target to > create_grouping_paths() as well. Again, we have to check for > parallel-safety for scanjoin_target before applying it to partial paths, > but we need to do that only when grouped_rel is considered parallel-safe. > > One more idea could be to have a flag (say parallel_safe) in PathTarget and set it once we have ensured that the expressions in target doesn't contain any parallel-restricted entry. In create_pathtarget()/set_pathtarget_cost_width(), if the parallelmodeOK flag is set, then we can evaluate target expressions for parallel-restricted expressions and set the flag accordingly. Now, this has the benefit that we don't need to evaluate the expressions of targets for parallel-restricted clauses again and again. I think this way if the flag is set once for final_target in grouping_planner, we don't need to evaluate it again for other targets (grouping_target, scanjoin_target, etc.) as those are derived from final_target. Similarly, I think we need to set ReplOptInfo->reltarget and others as required. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
