On 2015/12/02 1:53, Robert Haas wrote:
On Fri, Nov 27, 2015 at 1:33 AM, Etsuro Fujita
<fujita.ets...@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.
IIUC, I think the EXPLAIN output for eg,
select localtab.* from localtab, ft1, ft2 where localtab.a = ft1.a and
ft1.a = ft2.a for update
would be something like this:
LockRows
-> Nested Loop
Join Filter: (ft1.a = localtab.a)
-> Seq Scan on localtab
-> Foreign Scan on ft1/ft2-foreign-join
-> Nested Loop
Join Filter: (ft1.a = ft2.a)
-> Foreign Scan on ft1
-> Foreign Scan on ft2
The subplan below the Foreign Scan on the foreign-join seems odd to me.
One option to avoid that is to handle the subplan as in my patch [2],
which I created to address your comment that we should not break the
equivalence discussed below. I'm not sure that the patch's handling of
chgParam for the subplan is a good idea, though.
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.
Agreed. Thanks for the explanation!
Best regards,
Etsuro Fujita
[2] 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
