Re: postgres_fdw join pushdown (was Re: Custom/Foreign-Join-APIs)

On 2015/12/08 17:27, Ashutosh Bapat wrote:
> On Thu, Dec 3, 2015 at 12:36 PM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@lab.ntt.co.jp>> wrote:

>         Generating paths
>         ================

>         A join between two foreign relations is considered safe to push
>         down if

>         4. The join conditions (e.g. conditions in ON clause) are all
>         safe to
>         push down.
>              This is important for OUTER joins as pushing down join clauses
>         partially and
>              applying rest locally changes the result. There are ways [1] by
>         which partial
>              OUTER join can be completed by applying unpushable clauses
>         locally
>         and then
>              nullifying the nullable side and eliminating duplicate
>         non-nullable side
>              rows. But that's again out of scope of first version of
>         postgres_fdw
>         join
>              pushdown.

>     As for 4, as commented in the patch, we could relax the requirement
>     that all the join conditions (given by JoinPathExtraData's
>     restrictlist) need to be safe to push down to the remote server;
>     * In case of inner join, all the conditions would not need to be safe.
>     * In case of outer join, all the "otherclauses" would not need to be
>     safe, while I think all the "joinclauses" need to be safe to get the
>     right results (where "joinclauses" and "otherclauses" are defined by
>     extract_actual_join_clauses).  And I think we should do this
>     relaxation to some extent for 9.6, to allow more joins to be pushed
>     down.

> agreed. I will work on those.

Great!

>         Generating plan
>         ===============

>         Rest of this section describes the logic to construct
>         the SQL
>         for join; the logic is implemented as function
>         deparseSelectSqlForRel().
>
>         deparseSelectSqlForRel() builds the SQL for given joinrel (and
>         now for
>         baserel
>         asd well) recursively.
>         For joinrels
>         1. it constructs SQL representing either side of join, by
>         calling itself
>              in recursive fashion.
>         2. These SQLs are converted into subqueries and become part of
>         the FROM
>         clause
>              with appropriate JOIN type and clauses. The left and right
>         subqueries are
>              given aliases "l" and "r" respectively. The columns in each
>         subquery are
>              aliased as "a1", "a2", "a3" and so on. Thus the third
>         column on left
>         side can
>              be referenced as "l.a3" at any recursion level.
>         3. Targetlist is added representing the columns in the join result
>         expected at
>              that level.
>         4. The join clauses are added as part of ON clause
>         5. Any clauses that planner has deemed fit to be evaluated at
>         that level
>         of join
>              are added as part of WHERE clause.

>     Honestly, I'm not sure that that is a good idea.  One reason for
>     that is that a query string constructed by the procedure is
>     difficult to read especially when the procedure is applied
>     recursively.  So, I'm thinking to revise the procedure so as to
>     construct a query string with a flattened FROM clause, as discussed
>     in eg, [2].

> Just to confirm, the hook discussed in [2] is not in place right? I can
> find only one hook for foreign join
>   50 typedef void (*GetForeignJoinPaths_function) (PlannerInfo *root,
>   51
> RelOptInfo *joinrel,
>   52                                                         RelOptInfo
> *outerrel,
>   53                                                         RelOptInfo
> *innerrel,
>   54                                                           JoinType
> jointype,
>   55
> JoinPathExtraData *extra);
> This hook takes an inner and outer relation, so can not be used for
> N-way join as discussed in that thread.
>
> Are you suggesting that we should add that hook before we implement join
> pushdown in postgres_fdw? Am I missing something?

I don't mean it.  I'm thinking that I'll just revise the procedure so as 
to generate a FROM clause that is something like "from c left join d on 
(...) full join e on (...)" based on the existing hook you mentioned.

>         TODOs
>         =====

>         In another thread Robert, Fujita-san and Kaigai-san are
>         discussing about
>         EvalPlanQual support for foreign joins. Corresponding changes to
>         postgres_fdw
>         will need to be added once those changes get committed.

>     Yeah, we would need those changes including helper functions to
>     create a local join execution plan for that support.  I'd like to
>     add those changes to your updated patch if it's okay.

> Right now, we do not have any support for postgres_fdw join pushdown. I
> was thinking of adding at least minimal support for the same using this
> patch, may be by preventing join pushdown in case there are row marks
> for now. That way, we at least have some way to play with postgres_fdw
> join pushdown. Once we have that, we can work on remaining items listed
> for 9.6 and also you can add suport for row marks with fix for
> EvalPlanQual independently. This will keep the first patch smaller. Do
> you agree or you want to see EvalPlanQual fix to be in the first patch
> itself?

IMO I want to see the EvalPlanQual fix in the first version for 9.6.

Best regards,
Etsuro Fujita