Re: Foreign join pushdown vs EvalPlanQual

On 2015/11/26 14:04, Kouhei Kaigai wrote:
>> On 2015/11/24 2:41, Robert Haas wrote:
>>> On Fri, Nov 20, 2015 at 12:11 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>>>> One subplan means FDW driver run an entire join sub-tree with local
>>>> alternative sub-plan; that is my expectation for the majority case.

>>> What I'm imagining is that we'd add handling that allows the
>>> ForeignScan to have inner and outer children. If the FDW wants to
>>> delegate the EvalPlanQual handling to a local plan, it can use the
>>> outer child for that. Or the inner one, if it likes. The other one
>>> is available for some other purposes which we can't imagine yet. If
>>> this is too weird, we can only add handling for an outer subplan and
>>> forget about having an inner subplan for now. I just thought to make
>>> it symmetric, since outer and inner subplans are pretty deeply baked
>>> into the structure of the system.

>> I'd vote for only allowing an outer subplan.

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

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

> Remaining portions are as previous version. ExecScanFetch is revised
> to call recheckMtd always when scanrelid==0, then FDW driver can get
> control using RecheckForeignScan callback.
> It allows FDW driver to handle (1) EPQ recheck on underlying scan nodes,
> (2) reconstruction of joined tuple, and (3) EPQ recheck on join clauses,
> by its preferable implementation - including execution of an alternative
> sub-plan.

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

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

>> There seems to be no changes to make_foreignscan. Is that OK?

> create_foreignscan_path(), not only make_foreignscan().

OK

> This patch is not tested by actual FDW extensions, so it is helpful
> to enhance postgres_fdw to run the alternative sub-plan on EPQ recheck.

Will do.

Best regards,
Etsuro Fujita