On 2015/12/02 1:41, Robert Haas wrote:
On Thu, Nov 26, 2015 at 7:59 AM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp> wrote:
The attached patch adds: Path *fdw_outerpath field to ForeignPath node.
FDW driver can set arbitrary but one path-node here.
After that, this path-node shall be transformed to plan-node by
createplan.c, then passed to FDW driver using GetForeignPlan callback.
I understand this, as I also did the same thing in my patches, but actually,
that seems a bit complicated to me. Instead, could we keep the
fdw_outerpath in the fdw_private of a ForeignPath node when creating the
path node during GetForeignPaths, and then create an outerplan accordingly
from the fdw_outerpath stored into the fdw_private during GetForeignPlan, by
using create_plan_recurse there? I think that that would make the core
involvment much simpler.
I can't see how it's going to get much simpler than this. The core
core is well under a hundred lines, and it all looks pretty
straightforward to me. All of our existing path and plan types keep
lists of paths and plans separate from other kinds of data, and I
don't think we're going to win any awards for deviating from that
principle here.
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.
@@ -85,6 +86,18 @@ ForeignRecheck(ForeignScanState *node, TupleTableSlot
*slot)
ResetExprContext(econtext);
+ /*
+ * FDW driver has to recheck visibility of EPQ tuple towards
+ * the scan qualifiers once it gets pushed down.
+ * In addition, if this node represents a join sub-tree, not
+ * a scan, FDW driver is also responsible to reconstruct
+ * a joined tuple according to the primitive EPQ tuples.
+ */
+ if (fdwroutine->RecheckForeignScan)
+ {
+ if (!fdwroutine->RecheckForeignScan(node, slot))
+ return false;
+ }
Maybe I'm missing something, but I think we should let FDW do the work if
scanrelid==0, not just if fdwroutine->RecheckForeignScan is given. (And if
scanrelid==0 and fdwroutine->RecheckForeignScan is not given, we should
abort the transaction.)
That would be unnecessarily restrictive. On the one hand, even if
scanrelid != 0, the FDW can decide that it prefers to do the rechecks
using RecheckForeignScan rather than fdw_recheck_quals. For most
FDWs, I expect using fdw_recheck_quals to be more convenient, but
there may be cases where somebody prefers to use RecheckForeignScan,
and allowing that costs nothing.
I suppose that the flexibility would probably be a good thing, but I'm a
little bit concerned that that might be rather confusing to FDW authors.
Maybe I'm missing something, though.
Best regards,
Etsuro Fujita
[1] http://www.postgresql.org/message-id/5624d583.10...@lab.ntt.co.jp
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
