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