Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: > (2019/03/01 20:00), Antonin Houska wrote: > > Etsuro Fujita<fujita.ets...@lab.ntt.co.jp> wrote:
> > It's still unclear to me why add_foreign_ordered_paths() passes the input > > relation (input_rel) to estimate_path_cost_size(). If it passed the output > > rel > > (i.e. ordered_rel in this case) like add_foreign_grouping_paths() does, then > > foreignrel->reltarget should contain the random() function. (What I see now > > is > > that create_ordered_paths() leaves ordered_rel->reltarget empty, but that's > > another problem to be fixed.) > > > > Do you still see a reason to call estimate_path_cost_size() this way? > > Yeah, the reason for that is because we can estimate the costs of sorting for > the final scan/join relation using the existing code as-is that estimates the > costs of sorting for base or join relations, except for tlist eval cost > adjustment. As you mentioned, we could pass ordered_rel to > estimate_path_cost_size(), but if so, I think we would need to get input_rel > (ie, the final scan/join relation) from ordered_rel within > estimate_path_cost_size(), to use that code, which would not be great. After some more thought I suggest that estimate_path_cost_size() is redesigned before your patch gets applied. The function is quite hard to read if - with your patch applied - the "foreignrel" argument sometimes means the output relation and other times the input one. I takes some effort to "switch context" between these two perspectives when I read the code. Perhaps both input and output relation should be passed to the function now, and maybe also UpperRelationKind of the output relation. And maybe it's even worth moving some code into a subroutine that handles only the upper relation. Originally the function could only handle base relations, then join relation was added, then one kind of upper relation (UPPERREL_GROUP_AGG) was added and eventually the function will need to handle multiple kinds of upper relation. It should be no surprise if such an amount of changes makes the original signature insufficient. -- Antonin Houska https://www.cybertec-postgresql.com
