Re: Foreign join pushdown vs EvalPlanQual

> On 2015/12/02 1:41, Robert Haas wrote:
> > On Thu, Nov 26, 2015 at 7:59 AM, Etsuro Fujita
> > <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> 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.
>
> > I can't see how it's going to get much simpler than this. The core
> > core is well under a hundred lines, and it all looks pretty
> > straightforward to me. All of our existing path and plan types keep
> > lists of paths and plans separate from other kinds of data, and I
> > don't think we're going to win any awards for deviating from that
> > principle here.
>
> One thing I can think of is that we can keep both the structure of a
> ForeignPath node and the API of create_foreignscan_path as-is. The
> latter is a good thing for FDW authors. And IIUC the patch you posted
> today, I think we could make create_foreignscan_plan a bit simpler too.
> Ie, in your patch, you modified that function as follows:
>
> @@ -2129,7 +2134,9 @@ create_foreignscan_plan(PlannerInfo *root,
> ForeignPath *best_path,
> */
> scan_plan = rel->fdwroutine->GetForeignPlan(root, rel, rel_oid,
>
> best_path,
> -
> tlist, scan_clauses);
> +
> tlist,
> +
> scan_clauses);
> + outerPlan(scan_plan) = fdw_outerplan;
>
> I think that would be OK, but I think we would have to do a bit more
> here about the fdw_outerplan's targetlist and qual; I think that the
> targetlist needs to be changed to fdw_scan_tlist, as in the patch [1],
>
Hmm... you are right. The sub-plan shall generate a tuple according to
the fdw_scan_tlist, if valid. Do you think the surgical operation is best
to apply alternative target-list than build_path_tlist()?

> and that it'd be better to change the qual to remote conditions, ie,
> quals not in the scan_plan's scan.plan.qual, to avoid duplicate
> evaluation of local conditions. (In the patch [1], I didn't do anything
> about the qual because the current postgres_fdw join pushdown patch
> assumes that all the the scan_plan's scan.plan.qual are pushed down.)
> Or, FDW authors might want to do something about fdw_recheck_quals for a
> foreign-join while creating the fdw_outerplan. So if we do that during
> GetForeignPlan, I think we could make create_foreignscan_plan a bit
> simpler, or provide flexibility to FDW authors.
>
So, you suggest it is better to pass fdw_outerplan on the GetForeignPlan
callback, to allow FDW to adjust target-list and quals of sub-plans.
I think it is reasonable argue. Only FDW knows which qualifiers are
executable on remote side, so it is not easy to remove qualifiers to be
executed on host-side only, from the sub-plan tree.

> >> @@ -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.)
>
> > That would be unnecessarily restrictive. On the one hand, even if
> > scanrelid != 0, the FDW can decide that it prefers to do the rechecks
> > using RecheckForeignScan rather than fdw_recheck_quals. For most
> > FDWs, I expect using fdw_recheck_quals to be more convenient, but
> > there may be cases where somebody prefers to use RecheckForeignScan,
> > and allowing that costs nothing.
>
> I suppose that the flexibility would probably be a good thing, but I'm a
> little bit concerned that that might be rather confusing to FDW authors.
>
We expect FDW authors, like Hanada-san, have deep knowledge about PostgreSQL
internal. It is not a feature for SQL newbie.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

Maybe I'm missing something, though.
>
> Best regards,
> Etsuro Fujita
>
> [1] http://www.postgresql.org/message-id/5624D583.10202@lab.ntt.co.jp
>