Re: Foreign join pushdown vs EvalPlanQual
От | Etsuro Fujita |
---|---|
Тема | Re: Foreign join pushdown vs EvalPlanQual |
Дата | |
Msg-id | 565EA913.7040201@lab.ntt.co.jp обсуждение исходный текст |
Ответ на | Re: Foreign join pushdown vs EvalPlanQual (Kouhei Kaigai <kaigai@ak.jp.nec.com>) |
Список | pgsql-hackers |
On 2015/12/02 14:54, Kouhei Kaigai wrote: >> On 2015/12/02 1:41, Robert Haas wrote: >>> On Thu, Nov 26, 2015 at 7:59 AM, Etsuro Fujita >>> <fujita.etsuro@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()? Sorry, I'm not sure about that. I thought changing it to fdw_scan_tlist just because that's simple. >> 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 that is one option for us. Another option, which I proposed above, is 1) store an fdw_outerpath in the fdw_private when creating the ForeignPath node in GetForeignPaths, and then 2) create an fdw_outerplan from the fdw_outerpath stored into the fdw_private when creating the ForeignScan node in GetForeignPlan, by using create_plan_recurse in GetForeignPlan. (To do so, I was thinking to make that function extern.) One good point about that is that we can keep the API of create_foreignscan_path as-is, which I think would be a good thing for FDW authors that don't care about join pushdown. > 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. Yeah, we could provide the flexibility to FDW authors. >>>> @@ -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. That's right! Best regards, Etsuro Fujita
В списке pgsql-hackers по дате отправления: