Re: Foreign join pushdown vs EvalPlanQual

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