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....@lab.ntt.co.jp
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
