On Fri, Nov 27, 2015 at 1:33 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote: Plan *plan =
&node->scan.plan;
> @@ -3755,7 +3763,7 @@ make_foreignscan(List *qptlist,
> /* cost will be filled in by create_foreignscan_plan */
> plan->targetlist = qptlist;
> plan->qual = qpqual;
> - plan->lefttree = NULL;
> + plan->lefttree = fdw_outerplan;
> plan->righttree = NULL;
> node->scan.scanrelid = scanrelid;
>
> I think that that would break the EXPLAIN output.
In what way? EXPLAIN recurses into the left and right trees of every
plan node regardless of what type it is, so superficially I feel like
this ought to just work. What problem do you foresee?
I do think that ExecInitForeignScan ought to be changed to
ExecInitNode on it's outer plan if present rather than leaving that to
the FDW's BeginForeignScan method.
> One option to avoid that
> is to set the fdw_outerplan in ExecInitForeignScan as in my patch [1], or
> BeginForeignScan as you proposed. That breaks the equivalence that the Plan
> tree and the PlanState tree should be mirror images of each other, but I
> think that that break would be harmless.
I'm not sure how many times I have to say this, but we are not doing
that. I will not commit any patch that does that, and I will
vigorously argue against anyone else committing such a patch either.
That *would* break EXPLAIN, because EXPLAIN relies on being able to
walk the PlanState tree and find all the Plan nodes from the
corresponding PlanState nodes. Now you might think that it would be
OK to omit a plan node that we decided we weren't ever going to
execute, but today we don't do that, and I don't think we should. I
think it could be very confusing if EXPLAIN and EXPLAIN ANALYZE show
different sets of plan nodes for the same query. Quite apart from
EXPLAIN, there are numerous other places that assume that they can
walk the PlanState tree and find all the Plan nodes. Breaking that
assumption would be bad news.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
