Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:
> (2019/03/01 20:00), Antonin Houska wrote:
> > Etsuro Fujita<fujita.etsuro@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
