Re: Foreign join pushdown vs EvalPlanQual

On 2015/12/05 5:15, Robert Haas wrote:
> 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.

OK.

> 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.

Agreed.

> 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.

As I proposed upthread, another idea would be to 1) to store an 
fdw_outerpath in the fdw_private list of a ForeignPath node, and then 2) 
to create an fdw_outerplan from *the fdw_outerpath stored into
the fdw_private* in GetForeignPlan.  One good thing for this is that we 
keep the API of create_foreignscan_path as-is.  What do you think about 
that?

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

Thanks for updating the patch!

Best regards,
Etsuro Fujita