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

On Fri, Jan 29, 2016 at 9:51 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Thu, Jan 28, 2016 at 11:26 AM, Ashutosh Bapat
> <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
> > 2. pg_fdw_join_v3.patch: changes to postgres_fdw - more description below
>
> This patch no longer quite applies because of conflicts with one of
> your other patches that I applied today (cf. commit
> fbe5a3fb73102c2cfec11aaaa4a67943f4474383).
>
> And then I broke it some more by committing a patch to extract
> deparseLockingClause from postgresGetForeignPlan and move it to
> deparse.c, but that should pretty directly reduce the size of this
> patch. I wonder if there are any other bits of refactoring of that
> sort that we can do in advance of landing the main patch, just to
> simplify review. But I'm not sure there are: this patch removes very
> little existing code; it just adds a ton of new stuff.
>

PFA patch to move code to deparse SELECT statement into a function
deparseSelectStmtForRel(). This code is duplicated in
estimate_path_cost_size() and postgresGetForeignPlan(), so moving it into a
function avoids that duplication. As a side note, estimate_path_cost_size()
doesn't add FOR SHARE/UPDATE clause to the statement being EXPLAINed, even
if the actual statement during execution would have it. This difference
looks unintentional to me. This patch corrects it as well.
appendOrderByClause and appendWhereClause both create a context within
themselves and pass it to deparseExpr. This patch creates the context once
in deparseSelectStmtForRel() and then passes it to the other deparse
functions avoiding repeated context creation.

> copy_path_for_epq_recheck() and friends are really ugly. Should we
> consider just adding copyObject() support for those node types
> instead?
>

the note in copyfuncs.c says
* We also do not support copying Path trees, mainly
* because the circular linkages between RelOptInfo and Path nodes can't
* be handled easily in a simple depth-first traversal.

We may avoid that by just copying the pointer to RelOptInfo and not copy
the whole RelOptInfo. The other problem is paths in epq_paths will be
copied as many times as the number of 2-way joins pushed down. Let me give
it a try and produce patch with that.

I'm sure there's more -- this is a huge patch and I don't fully
> understand it yet -- but I'm out of energy for tonight.
>
>
Thanks a lot for your comments and moving this patch forward.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company