Re: Foreign join pushdown vs EvalPlanQual

On Tue, Dec 1, 2015 at 10:20 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> One thing I can think of is that we can keep both the structure of a
> ForeignPath node and the API of create_foreignscan_path as-is.  The latter
> is a good thing for FDW authors.  And IIUC the patch you posted today, I
> think we could make create_foreignscan_plan a bit simpler too.  Ie, in your
> patch, you modified that function as follows:
>
> @@ -2129,7 +2134,9 @@ create_foreignscan_plan(PlannerInfo *root, ForeignPath
> *best_path,
>          */
>         scan_plan = rel->fdwroutine->GetForeignPlan(root, rel, rel_oid,
>
> best_path,
> -
> tlist, scan_clauses);
> +
> tlist,
> +
> scan_clauses);
> +       outerPlan(scan_plan) = fdw_outerplan;
>
> I think that would be OK, but I think we would have to do a bit more here
> about the fdw_outerplan's targetlist and qual; I think that the targetlist
> needs to be changed to fdw_scan_tlist, as in the patch [1], and that it'd be
> better to change the qual to remote conditions, ie, quals not in the
> scan_plan's scan.plan.qual, to avoid duplicate evaluation of local
> conditions.  (In the patch [1], I didn't do anything about the qual because
> the current postgres_fdw join pushdown patch assumes that all the the
> scan_plan's scan.plan.qual are pushed down.) Or, FDW authors might want to
> do something about fdw_recheck_quals for a foreign-join while creating the
> fdw_outerplan.  So if we do that during GetForeignPlan, I think we could
> make create_foreignscan_plan a bit simpler, or provide flexibility to FDW
> authors.

It's certainly true that we need the alternative plan's tlist to match
that of the main plan; otherwise, it's going to be difficult for the
FDW to make use of that alternative subplan to fill its slot, which is
kinda the point of all this.  However, I'm quite reluctant to
introduce code into create_foreignscan_plan() that forces the
subplan's tlist to match that of the main plan.  For one thing, that
would likely foreclose the possibility of an FDW ever using the outer
plan for any purpose other than EPQ rechecks.  It may be hard to
imagine what else you'd do with the outer plan as things are today,
but right now the two haves of the patch - letting FDWs have an outer
subplan, and providing them with a way of overriding the EPQ recheck
behavior - are technically independent.  Putting tlist-altering
behavior into create_foreignscan_plan() ties those two things together
irrevocably.

Instead, I think we should go the opposite direction and pass the
outerplan to GetForeignPlan after all.  I was lulled into a full sense
of security by the realization that every FDW that uses this feature
MUST want to do outerPlan(scan_plan) = fdw_outerplan.  That's true,
but irrelevant.  The point is that the FDW might want to do something
additional, like frob the outer plan's tlist, and it can't do that if
we don't pass it fdw_outerplan.  So we should do that, after all.

Updated patch attached.  This fixes a couple of whitespace issues that
were pointed out, also.

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