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.
>
How to use create_plan_recurse by extension? It is a static function.

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

> > 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.)
>
It should be Assert(). The node with scanrelid==0 never happen
unless FDW driver does not add such a path explicitly.

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

> >> 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.
>
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>