Re: Foreign join pushdown vs EvalPlanQual

On Tue, Dec 8, 2015 at 5:49 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> On 2015/12/08 3:06, Tom Lane wrote:
>> Robert Haas <robertmhaas@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