Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: > (2019/02/08 21:35), Etsuro Fujita wrote: > > (2019/02/08 2:04), Antonin Houska wrote: > >> * add_foreign_ordered_paths() > >> > >> Exprssion root->upper_targets[UPPERREL_FINAL] is used to access the > >> target. > >> > >> I think create_ordered_paths() should actually set the target so that > >> postgresGetForeignUpperPaths() can simply find it in > >> output_rel->reltarget. This is how postgres_fdw already retrieves the > >> target > >> for grouped paths. Small patch is attached that makes this possible. > > > > Seems like a good idea. Thanks for the patch! Will review. > > Here are my review comments: > > root->upper_targets[UPPERREL_FINAL] = final_target; > + root->upper_targets[UPPERREL_ORDERED] = final_target; > root->upper_targets[UPPERREL_WINDOW] = sort_input_target; > root->upper_targets[UPPERREL_GROUP_AGG] = grouping_target; > > I think that's a good idea. I think we'll soon need the PathTarget for > UPPERREL_DISTINCT, so how about saving that as well like this? > > root->upper_targets[UPPERREL_FINAL] = final_target; > + root->upper_targets[UPPERREL_ORDERED] = final_target; > + root->upper_targets[UPPERREL_DISTINCT] = sort_input_target; > root->upper_targets[UPPERREL_WINDOW] = sort_input_target; > root->upper_targets[UPPERREL_GROUP_AGG] = grouping_target;
I see nothing wrong about this. > Another is about this: > > /* > + * Set reltarget so that it's consistent with the paths. Also it's more > + * convenient for FDW to find the target here. > + */ > + ordered_rel->reltarget = target; > > I think this is correct if there are no SRFs in the targetlist, but otherwise > I'm not sure this is really correct because the relation's reltarget would not > match the target of paths added to the relation after adjust_paths_for_srfs(). I wouldn't expect problem here. create_grouping_paths() also sets the reltarget although adjust_paths_for_srfs() can be alled on grouped_rel afterwards. -- Antonin Houska Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26, A-2700 Wiener Neustadt Web: https://www.cybertec-postgresql.com
