On 2015/07/02 23:13, Kouhei Kaigai wrote:
>> To be honest, ISTM that it's difficult to do that simply and efficiently
>> for the foreign/custom-join-pushdown API that we have for 9.5. It's a
>> little late, but what I started thinking is to redesign that API so that
>> that API is called at standard_join_search, as discussed in [2]; (1) to
>> place that API call *after* the set_cheapest call and (2) to place
>> another set_cheapest call after that API call for each joinrel. By the
>> first set_cheapest call, I think we could probably save an alternative
>> path that we need in "cheapest_builtin_path". By the second
>> set_cheapest call following that API call, we could consider
>> foreign/custom-join-pushdown paths also. What do you think about this idea?
> Disadvantage is larger than advantage, sorry.
> The reason why we put foreign/custom-join hook on add_paths_to_joinrel()
> is that the source relations (inner/outer) were not obvious, thus,
> we cannot reproduce which relations are the source of this join.
> So, I had to throw a spoon when I tried this approach before.
Maybe I'm missing something, but my image about this approach is that if
base relations for a given joinrel are all foreign tables and belong to
the same foreign server, then by calling that API there, we consider the
remote join over all the foreign tables, and that if not, we give up to
consider the remote join.
> My idea is that we save the cheapest_total_path of RelOptInfo onto the
> new cheapest_builtin_path just before the GetForeignJoinPaths() hook.
> Why? It should be a built-in join logic, never be a foreign/custom-join,
> because of the hook location; only built-in logic shall be added here.
My concern about your idea is that since that (a) add_paths_to_joinrel
is called multiple times per joinrel and that (b) repetitive add_path
calls through GetForeignJoinPaths in add_paths_to_joinrel might remove
old paths that are builtin, it's possible to save a path that is not
builtin onto the cheapest_total_path and thus to save that path wrongly
onto the cheapest_builtin_path. There might be a good way to cope with
that, though.
> Regarding to the development timeline, I prefer to put something
> workaround not to kick Assert() on ExecScanFetch().
> We may add a warning in the documentation not to replace built-in
> join if either/both of sub-trees are target of UPDATE/DELETE or
> FOR SHARE/UPDATE.
I'm not sure that that is a good idea, but anyway, I think we need to
hurry fixing this issue.
Best regards,
Etsuro Fujita