Re: Foreign join pushdown vs EvalPlanQual

On 2015/12/02 1:53, Robert Haas wrote:
> On Fri, Nov 27, 2015 at 1:33 AM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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.10202@lab.ntt.co.jp