> On 2015/12/02 1:41, Robert Haas wrote: > > On Thu, Nov 26, 2015 at 7:59 AM, Etsuro Fujita > > <fujita.ets...@lab.ntt.co.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 <kai...@ak.jp.nec.com> Maybe I'm missing something, though. > > Best regards, > Etsuro Fujita > > [1] http://www.postgresql.org/message-id/5624d583.10...@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