Re: Foreign join pushdown vs EvalPlanQual

On 2015/11/27 0:14, Kouhei Kaigai wrote:
>> On 2015/11/26 14:04, Kouhei Kaigai 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.

> How to use create_plan_recurse by extension? It is a static function.

I was just thinking a change to make that function extern.

>>> We expect FDW driver set this plan-node on lefttree (a.k.a outerPlan).
>>> The Plan->outerPlan is a common field, so patch size become relatively
>>> small. FDW driver can initialize this plan at BeginForeignScan, then
>>> execute this sub-plan-tree on demand.

>> Another idea would be to add the core support for
>> initializing/closing/rescanning the outerplan tree when the tree is given.

> No. Please don't repeat same discussion again.

IIUC, I think your point is to allow FDWs to do something else, instead 
of performing a local join execution plan, during RecheckForeignScan. 
So, what's wrong with the core doing that support in that case?

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

> It should be Assert(). The node with scanrelid==0 never happen
> unless FDW driver does not add such a path explicitly.

That's an idea.  But the abort seems to me more consistent with other 
places (see eg, RefetchForeignRow in EvalPlanQualFetchRowMarks).

>> Another thing I'm concerned about is
>>
>> @@ -347,8 +355,26 @@ ExecScanReScan(ScanState *node)
>>        {
>>            Index        scanrelid = ((Scan *)
>> node->ps.plan)->scanrelid;
>>
>> -        Assert(scanrelid > 0);
>> +        if (scanrelid > 0)
>> +            estate->es_epqScanDone[scanrelid - 1] = false;
>> +        else
>> +        {
>> +            Bitmapset  *relids;
>> +            int            rtindex = -1;
>> +
>> +            if (IsA(node->ps.plan, ForeignScan))
>> +                relids = ((ForeignScan *)
>> node->ps.plan)->fs_relids;
>> +            else if (IsA(node->ps.plan, CustomScan))
>> +                relids = ((CustomScan *)
>> node->ps.plan)->custom_relids;
>> +            else
>> +                elog(ERROR, "unexpected scan node: %d",
>> +                     (int)nodeTag(node->ps.plan));
>>
>> -        estate->es_epqScanDone[scanrelid - 1] = false;
>> +            while ((rtindex = bms_next_member(relids, rtindex)) >=
>> 0)
>> +            {
>> +                Assert(rtindex > 0);
>> +                estate->es_epqScanDone[rtindex - 1] = false;
>> +            }
>> +        }
>>        }
>>
>> That seems the outerplan's business to me, so I think it'd be better to
>> just return, right before the assertion, as I said before.  Seen from
>> another angle, ISTM that FDWs that don't use a local join execution plan
>> wouldn't need to be aware of handling the es_epqScanDone flags.  (Do you
>> think that such FDWs should do something like what ExecScanFtch is doing
>> about the flags, in their RecheckForeignScans?  If so, I think we need
>> docs for that.)

> Execution of alternative local subplan (outerplan) is discretional.
> We have to pay attention FDW drivers which handles EPQ recheck by
> itself. Even though you argue callback can violate state of
> es_epqScanDone flags, it is safe to follow the existing behavior.

So, I think the documentation needs more work.

Yet another thing that I'm concerned about is

@@ -3747,7 +3754,8 @@ make_foreignscan(List *qptlist,                  List *fdw_exprs,                  List
*fdw_private,                 List *fdw_scan_tlist,
 
-                 List *fdw_recheck_quals)
+                 List *fdw_recheck_quals,
+                 Plan *fdw_outerplan) {     ForeignScan *node = makeNode(ForeignScan);     Plan       *plan =
&node->scan.plan;
@@ -3755,7 +3763,7 @@ make_foreignscan(List *qptlist,     /* cost will be filled in by create_foreignscan_plan */
plan->targetlist= qptlist;     plan->qual = qpqual;
 
-    plan->lefttree = NULL;
+    plan->lefttree = fdw_outerplan;     plan->righttree = NULL;     node->scan.scanrelid = scanrelid;

I think that that would break the EXPLAIN output.  One option to avoid 
that is to set the fdw_outerplan in ExecInitForeignScan as in my patch 
[1], or BeginForeignScan as you proposed.  That breaks the equivalence 
that the Plan tree and the PlanState tree should be mirror images of 
each other, but I think that that break would be harmless.

Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/55DEF5F0.308@lab.ntt.co.jp