Re: Foreign join pushdown vs EvalPlanQual

From: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com>, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>
Subject: Re: Foreign join pushdown vs EvalPlanQual
Date: 2015-12-08 10:49:03
Message-ID: 5666B59F.6010701@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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. I'm also attaching an updated version of the postgres_fdw
join pushdown patch. 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?

Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/565EA539.1080703@lab.ntt.co.jp
[2]
http://www.postgresql.org/message-id/CAEZqfEe9KGy=1_waGh2rgZPg0o4pqgD+iauYaj8wTze+CYJUHg@mail.gmail.com
[3] http://www.postgresql.org/message-id/5624D583.10202@lab.ntt.co.jp

Attachment Content-Type Size
epq-recheck-v6-efujita.patch application/x-patch 16.5 KB
foreign_join_v16_efujita2.patch application/x-patch 118.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Etsuro Fujita 2015-12-08 11:16:24 Minor comment update in setrefs.c
Previous Message FattahRozzaq 2015-12-08 10:33:26 HELP!!! The WAL Archive is taking up all space