Re: Foreign join pushdown vs EvalPlanQual

On 2015/12/02 1:41, Robert Haas wrote:
> On Thu, Nov 26, 2015 at 7:59 AM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp> wrote:
>>> The attached patch adds: Path *fdw_outerpath field to ForeignPath node.
>>> FDW driver can set arbitrary but one path-node here.
>>> After that, this path-node shall be transformed to plan-node by
>>> createplan.c, then passed to FDW driver using GetForeignPlan callback.

>> I understand this, as I also did the same thing in my patches, but actually,
>> that seems a bit complicated to me.  Instead, could we keep the
>> fdw_outerpath in the fdw_private of a ForeignPath node when creating the
>> path node during GetForeignPaths, and then create an outerplan accordingly
>> from the fdw_outerpath stored into the fdw_private during GetForeignPlan, by
>> using create_plan_recurse there?  I think that that would make the core
>> involvment much simpler.

> I can't see how it's going to get much simpler than this.  The core
> core is well under a hundred lines, and it all looks pretty
> straightforward to me.  All of our existing path and plan types keep
> lists of paths and plans separate from other kinds of data, and I
> don't think we're going to win any awards for deviating from that
> principle here.

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
asfollows:
 

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

>> @@ -85,6 +86,18 @@ ForeignRecheck(ForeignScanState *node, TupleTableSlot
>> *slot)
>>
>>          ResetExprContext(econtext);
>>
>> +       /*
>> +        * FDW driver has to recheck visibility of EPQ tuple towards
>> +        * the scan qualifiers once it gets pushed down.
>> +        * In addition, if this node represents a join sub-tree, not
>> +        * a scan, FDW driver is also responsible to reconstruct
>> +        * a joined tuple according to the primitive EPQ tuples.
>> +        */
>> +       if (fdwroutine->RecheckForeignScan)
>> +       {
>> +               if (!fdwroutine->RecheckForeignScan(node, slot))
>> +                       return false;
>> +       }
>>
>> Maybe I'm missing something, but I think we should let FDW do the work if
>> scanrelid==0, not just if fdwroutine->RecheckForeignScan is given. (And if
>> scanrelid==0 and fdwroutine->RecheckForeignScan is not given, we should
>> abort the transaction.)

> That would be unnecessarily restrictive.  On the one hand, even if
> scanrelid != 0, the FDW can decide that it prefers to do the rechecks
> using RecheckForeignScan rather than fdw_recheck_quals.  For most
> FDWs, I expect using fdw_recheck_quals to be more convenient, but
> there may be cases where somebody prefers to use RecheckForeignScan,
> and allowing that costs nothing.

I suppose that the flexibility would probably be a good thing, but I'm a 
little bit concerned that that might be rather confusing to FDW authors.  Maybe I'm missing something, though.

Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/5624D583.10202@lab.ntt.co.jp