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

On Mon, Feb 1, 2016 at 8:27 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> Here are patches rebased on recent commit
> cc592c48c58d9c1920f8e2063756dcbcce79e4dd. Renamed original deparseSelectSql
> as deparseSelectSqlForBaseRel and added deparseSelectSqlForJoinRel to
> construct SELECT and FROM clauses for base and join relations.
>
> pg_fdw_core_v5.patch GetUserMappingById changes
> pg_fdw_join_v5.patch: postgres_fdw changes for join pushdown including
> suggestions as described below
> pg_join_pd_v5.patch: combined patch for ease of testing.
>
> The patches also have following changes along with the changes described in
> my last mail.
> 1. Revised the way targetlists are handled. For a bare base relation the
> SELECT clause is deparsed from fpinfo->attrs_used but for a base relation
> which is part of join relation, the expected targetlist is passed down to
> deparseSelectSqlForBaseRel(). This change removed 75 odd lines in
> build_tlist_to_deparse() which were very similar to
> deparseTargetListFromAttrsUsed() in the previous patch.

Nice!

> 2. Refactored postgresGetForeignJoinPaths to be more readable moving the
> code to assess safety of join pushdown into a separate function.

That looks good.  But maybe call the function foreign_join_ok() or
something like that?  is_foreign_join() isn't terrible but it sounds a
little odd to me.

The path-copying stuff in get_path_for_epq_recheck() looks a lot
better now, but you neglected to add a comment explaining why you did
it that way (e.g. "Make a shallow copy of the join path, because the
planner might free the original structure after a future add_path().
We don't need to copy the substructure, though; that won't get freed."I would forget about setting
merge_path->materialize_inner= false;
 
that doesn't seem essential.  Also, I would arrange things so that if
you hit an unrecognized path type (like a custom join, or a gather)
you skip that particular path instead of erroring out.  I think this
whole function should be moved to core, and I think the argument
should be a RelOptInfo * rather than a List *.

+     * We can't know VERBOSE option is specified or not, so always add shcema

We can't know "whether" VERBOSE...
shcema -> schema

+     * the join relaiton is already considered, so that we won't waste time in

Typo.

+     * judging safety of join pushdow and adding the same paths again if found

Typo.

More when I have a bit more time to look at this...

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company