On 2015/12/05 5:15, Robert Haas wrote:
On Tue, Dec 1, 2015 at 10:20 PM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp> wrote:
One thing I can think of is that we can keep both the structure of a
ForeignPath node and the API of create_foreignscan_path as-is. The latter
is a good thing for FDW authors. And IIUC the patch you posted today, I
think we could make create_foreignscan_plan a bit simpler too. Ie, in your
patch, you modified that function as follows:
@@ -2129,7 +2134,9 @@ create_foreignscan_plan(PlannerInfo *root, ForeignPath
*best_path,
*/
scan_plan = rel->fdwroutine->GetForeignPlan(root, rel, rel_oid,
best_path,
-
tlist, scan_clauses);
+
tlist,
+
scan_clauses);
+ outerPlan(scan_plan) = fdw_outerplan;
I think that would be OK, but I think we would have to do a bit more here
about the fdw_outerplan's targetlist and qual; I think that the targetlist
needs to be changed to fdw_scan_tlist, as in the patch [1], and that it'd be
better to change the qual to remote conditions, ie, quals not in the
scan_plan's scan.plan.qual, to avoid duplicate evaluation of local
conditions. (In the patch [1], I didn't do anything about the qual because
the current postgres_fdw join pushdown patch assumes that all the the
scan_plan's scan.plan.qual are pushed down.) Or, FDW authors might want to
do something about fdw_recheck_quals for a foreign-join while creating the
fdw_outerplan. So if we do that during GetForeignPlan, I think we could
make create_foreignscan_plan a bit simpler, or provide flexibility to FDW
authors.
It's certainly true that we need the alternative plan's tlist to match
that of the main plan; otherwise, it's going to be difficult for the
FDW to make use of that alternative subplan to fill its slot, which is
kinda the point of all this.
OK.
However, I'm quite reluctant to
introduce code into create_foreignscan_plan() that forces the
subplan's tlist to match that of the main plan. For one thing, that
would likely foreclose the possibility of an FDW ever using the outer
plan for any purpose other than EPQ rechecks. It may be hard to
imagine what else you'd do with the outer plan as things are today,
but right now the two haves of the patch - letting FDWs have an outer
subplan, and providing them with a way of overriding the EPQ recheck
behavior - are technically independent. Putting tlist-altering
behavior into create_foreignscan_plan() ties those two things together
irrevocably.
Agreed.
Instead, I think we should go the opposite direction and pass the
outerplan to GetForeignPlan after all. I was lulled into a full sense
of security by the realization that every FDW that uses this feature
MUST want to do outerPlan(scan_plan) = fdw_outerplan. That's true,
but irrelevant. The point is that the FDW might want to do something
additional, like frob the outer plan's tlist, and it can't do that if
we don't pass it fdw_outerplan. So we should do that, after all.
As I proposed upthread, another idea would be to 1) to store an
fdw_outerpath in the fdw_private list of a ForeignPath node, and then 2)
to create an fdw_outerplan from *the fdw_outerpath stored into
the fdw_private* in GetForeignPlan. One good thing for this is that we
keep the API of create_foreignscan_path as-is. What do you think about
that?
Updated patch attached. This fixes a couple of whitespace issues that
were pointed out, also.
Thanks for updating the patch!
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
