(2019/03/04 16:46), Antonin Houska wrote:
Etsuro Fujita<fujita.ets...@lab.ntt.co.jp> wrote:
(2019/03/01 20:00), Antonin Houska 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.
I have to admit that my proposal makes estimate_path_cost_size()
complicated to a certain extent, but I don't think it changes the
meaning of the foreignrel; even in my proposal, the foreignrel should be
considered as an output relation rather than an input relation, because
we create a foreign upper path with the foreignrel being the parent
RelOptInfo for the foreign upper path in add_foreign_ordered_paths() or
add_foreign_final_paths().
Perhaps both input and output relation should be passed to the function now,
and maybe also UpperRelationKind of the output relation.
My concern is: that would make it inconvenient to call that function.
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.
I 100% agree with you on that point.
Best regards,
Etsuro Fujita
