On Tue, Dec 8, 2015 at 5:49 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: > On 2015/12/08 3:06, Tom Lane wrote: >> Robert Haas <robertmh...@gmail.com> writes: >>> I think the core system likely needs visibility into where paths and >>> plans are present in node trees, and putting them somewhere inside >>> fdw_private would be going in the opposite direction. > >> Absolutely. You don't really want FDWs having to take responsibility >> for setrefs.c processing of their node trees, for example. This is why >> e.g. ForeignScan has both fdw_exprs and fdw_private. >> >> I'm not too concerned about whether we have to adjust FDW-related APIs >> as we go along. It's been clear from the beginning that we'd have to >> do that, and we are nowhere near a point where we should promise that >> we're done doing so. > > OK, I'd vote for Robert's idea, then. I'd like to discuss the next > thing about his patch. As I mentioned in [1], the following change in > the patch will break the EXPLAIN output. > > @@ -205,6 +218,11 @@ ExecInitForeignScan(ForeignScan *node, EState > *estate, int eflags) > scanstate->fdwroutine = fdwroutine; > scanstate->fdw_state = NULL; > > + /* Initialize any outer plan. */ > + if (outerPlanState(scanstate)) > + outerPlanState(scanstate) = > + ExecInitNode(outerPlan(node), estate, eflags); > + > > As pointed out by Horiguchi-san, that's not correct, though; we should > initialize the outer plan if outerPlan(node) != NULL, not > outerPlanState(scanstate) != NULL. Attached is an updated version of > his patch.
Oops, good catch. > I'm also attaching an updated version of the postgres_fdw > join pushdown patch. Is that based on Ashutosh's version of the patch, or are the two of you developing independent of each other? We should avoid dueling patches if possible. > You can find the breaking examples by doing the > regression tests in the postgres_fdw patch. Please apply the patches in > the following order: > > epq-recheck-v6-efujita (attached) > usermapping_matching.patch in [2] > add_GetUserMappingById.patch in [2] > foreign_join_v16_efujita2.patch (attached) > > As I proposed upthread, I think we could fix that by handling the outer > plan as in the patch [3]; a) the core initializes the outer plan and > stores it into somewhere in the ForeignScanState node, not the lefttree > of the ForeignScanState node, during ExecInitForeignScan, and b) when > the RecheckForeignScan routine gets called, the FDW extracts the plan > from the given ForeignScanState node and executes it. What do you think > about that? I think the actual regression test outputs are fine, and that your desire to suppress part of the plan tree from showing up in the EXPLAIN output is misguided. I like it just the way it is. To prevent user confusion, I think that when we add support to postgres_fdw for this we might also want to add some documentation explaining how to interpret this EXPLAIN output, but I don't think there's any problem with the output itself. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
