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.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.

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         ->
ForeignScan 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
subplanas 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