Hi Amit,
(2018/05/11 16:12), Amit Langote wrote:
> On 2018/05/10 21:41, Etsuro Fujita wrote:
>> I think the reason for that is: in that case we try to find the target
>> foreign-join RelOptInfo using find_join_rel in postgresPlanDirectModify,
>> but can't find it, because the given root is the *parent* root and
>> doesn't have join RelOptInfos with it. To fix this, I'd like to propose
>> to modify make_modifytable so that in case of an inherited
>> UPDATE/DELETE, it passes to PlanDirectModify the per-child modified
>> subroot, not the parent root, for the FDW to get the foreign-join
>> RelOptInfo from the given root in PlanDirectModify. I think that that
>> would be more consistent with the non-inherited UPDATE/DELETE case in
>> that the FDW can look at any join RelOptInfos in the given root in
>> PlanDirectModify, which I think would be a good thing because some FDWs
>> might need to do that for some reason. For the same reason, I'd also
>> like to propose to pass to PlanForeignModify the per-child modified
>> subroot, not the parent root, as for PlanDirectModify. Attached is a
>> proposed patch for that. I'll add this to the open items list for PG11.
>
> So IIUC, we must pass the per-child PlannerInfo here, because that's what
> would have been used for the planning join between the child (ft1 in your
> example) and the other table (ft2 in your example). So that's where the
> RelOptInfo's corresponding to planning for the child, including that for
> the pushed-down join, would be stored.
Yeah, I think so too.
> Anyway, the patch and tests it adds look good.
Thanks for the review!
I added an assertion test to make_modifytable to match that in
create_modifytable_path. Attached is an updated version of the patch.
Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***************
*** 7370,7375 **** drop table bar cascade;
--- 7370,7450 ----
NOTICE: drop cascades to foreign table bar2
drop table loct1;
drop table loct2;
+ -- Test pushing down UPDATE/DELETE joins to the remote server
+ create table parent (a int, b text);
+ create table loct1 (a int, b text);
+ create table loct2 (a int, b text);
+ create foreign table remt1 (a int, b text)
+ server loopback options (table_name 'loct1');
+ create foreign table remt2 (a int, b text)
+ server loopback options (table_name 'loct2');
+ alter foreign table remt1 inherit parent;
+ insert into remt1 values (1, 'foo');
+ insert into remt1 values (2, 'bar');
+ insert into remt2 values (1, 'foo');
+ insert into remt2 values (2, 'bar');
+ analyze remt1;
+ analyze remt2;
+ explain (verbose, costs off)
+ update parent set b = parent.b || remt2.b from remt2 where parent.a = remt2.a returning *;
+ QUERY PLAN
+ -----------------------------------------------------------------------------------------------------------------------------------------------
+ Update on public.parent
+ Output: parent.a, parent.b, remt2.a, remt2.b
+ Update on public.parent
+ Foreign Update on public.remt1
+ -> Nested Loop
+ Output: parent.a, (parent.b || remt2.b), parent.ctid, remt2.*, remt2.a, remt2.b
+ Join Filter: (parent.a = remt2.a)
+ -> Seq Scan on public.parent
+ Output: parent.a, parent.b, parent.ctid
+ -> Foreign Scan on public.remt2
+ Output: remt2.b, remt2.*, remt2.a
+ Remote SQL: SELECT a, b FROM public.loct2
+ -> Foreign Update
+ Remote SQL: UPDATE public.loct1 r4 SET b = (r4.b || r2.b) FROM public.loct2 r2 WHERE ((r4.a = r2.a)) RETURNING r4.a, r4.b, r2.a, r2.b
+ (14 rows)
+
+ update parent set b = parent.b || remt2.b from remt2 where parent.a = remt2.a returning *;
+ a | b | a | b
+ ---+--------+---+-----
+ 1 | foofoo | 1 | foo
+ 2 | barbar | 2 | bar
+ (2 rows)
+
+ explain (verbose, costs off)
+ delete from parent using remt2 where parent.a = remt2.a returning parent;
+ QUERY PLAN
+ ------------------------------------------------------------------------------------------------------------------
+ Delete on public.parent
+ Output: parent.*
+ Delete on public.parent
+ Foreign Delete on public.remt1
+ -> Nested Loop
+ Output: parent.ctid, remt2.*
+ Join Filter: (parent.a = remt2.a)
+ -> Seq Scan on public.parent
+ Output: parent.ctid, parent.a
+ -> Foreign Scan on public.remt2
+ Output: remt2.*, remt2.a
+ Remote SQL: SELECT a, b FROM public.loct2
+ -> Foreign Delete
+ Remote SQL: DELETE FROM public.loct1 r4 USING public.loct2 r2 WHERE ((r4.a = r2.a)) RETURNING r4.a, r4.b
+ (14 rows)
+
+ delete from parent using remt2 where parent.a = remt2.a returning parent;
+ parent
+ ------------
+ (1,foofoo)
+ (2,barbar)
+ (2 rows)
+
+ -- cleanup
+ drop foreign table remt1;
+ drop foreign table remt2;
+ drop table loct1;
+ drop table loct2;
+ drop table parent;
-- ===================================================================
-- test tuple routing for foreign-table partitions
-- ===================================================================
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***************
*** 1767,1772 **** drop table bar cascade;
--- 1767,1804 ----
drop table loct1;
drop table loct2;
+ -- Test pushing down UPDATE/DELETE joins to the remote server
+ create table parent (a int, b text);
+ create table loct1 (a int, b text);
+ create table loct2 (a int, b text);
+ create foreign table remt1 (a int, b text)
+ server loopback options (table_name 'loct1');
+ create foreign table remt2 (a int, b text)
+ server loopback options (table_name 'loct2');
+ alter foreign table remt1 inherit parent;
+
+ insert into remt1 values (1, 'foo');
+ insert into remt1 values (2, 'bar');
+ insert into remt2 values (1, 'foo');
+ insert into remt2 values (2, 'bar');
+
+ analyze remt1;
+ analyze remt2;
+
+ explain (verbose, costs off)
+ update parent set b = parent.b || remt2.b from remt2 where parent.a = remt2.a returning *;
+ update parent set b = parent.b || remt2.b from remt2 where parent.a = remt2.a returning *;
+ explain (verbose, costs off)
+ delete from parent using remt2 where parent.a = remt2.a returning parent;
+ delete from parent using remt2 where parent.a = remt2.a returning parent;
+
+ -- cleanup
+ drop foreign table remt1;
+ drop foreign table remt2;
+ drop table loct1;
+ drop table loct2;
+ drop table parent;
+
-- ===================================================================
-- test tuple routing for foreign-table partitions
-- ===================================================================
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***************
*** 289,295 **** static ModifyTable *make_modifytable(PlannerInfo *root,
CmdType operation, bool canSetTag,
Index nominalRelation, List *partitioned_rels,
bool partColsUpdated,
! List *resultRelations, List *subplans,
List *withCheckOptionLists, List *returningLists,
List *rowMarks, OnConflictExpr *onconflict, int epqParam);
static GatherMerge *create_gather_merge_plan(PlannerInfo *root,
--- 289,295 ----
CmdType operation, bool canSetTag,
Index nominalRelation, List *partitioned_rels,
bool partColsUpdated,
! List *resultRelations, List *subplans, List *subroots,
List *withCheckOptionLists, List *returningLists,
List *rowMarks, OnConflictExpr *onconflict, int epqParam);
static GatherMerge *create_gather_merge_plan(PlannerInfo *root,
***************
*** 2484,2489 **** create_modifytable_plan(PlannerInfo *root, ModifyTablePath *best_path)
--- 2484,2490 ----
best_path->partColsUpdated,
best_path->resultRelations,
subplans,
+ best_path->subroots,
best_path->withCheckOptionLists,
best_path->returningLists,
best_path->rowMarks,
***************
*** 6558,6564 **** make_modifytable(PlannerInfo *root,
CmdType operation, bool canSetTag,
Index nominalRelation, List *partitioned_rels,
bool partColsUpdated,
! List *resultRelations, List *subplans,
List *withCheckOptionLists, List *returningLists,
List *rowMarks, OnConflictExpr *onconflict, int epqParam)
{
--- 6559,6565 ----
CmdType operation, bool canSetTag,
Index nominalRelation, List *partitioned_rels,
bool partColsUpdated,
! List *resultRelations, List *subplans, List *subroots,
List *withCheckOptionLists, List *returningLists,
List *rowMarks, OnConflictExpr *onconflict, int epqParam)
{
***************
*** 6566,6574 **** make_modifytable(PlannerInfo *root,
--- 6567,6577 ----
List *fdw_private_list;
Bitmapset *direct_modify_plans;
ListCell *lc;
+ ListCell *lc2;
int i;
Assert(list_length(resultRelations) == list_length(subplans));
+ Assert(list_length(resultRelations) == list_length(subroots));
Assert(withCheckOptionLists == NIL ||
list_length(resultRelations) == list_length(withCheckOptionLists));
Assert(returningLists == NIL ||
***************
*** 6627,6635 **** make_modifytable(PlannerInfo *root,
fdw_private_list = NIL;
direct_modify_plans = NULL;
i = 0;
! foreach(lc, resultRelations)
{
Index rti = lfirst_int(lc);
FdwRoutine *fdwroutine;
List *fdw_private;
bool direct_modify;
--- 6630,6639 ----
fdw_private_list = NIL;
direct_modify_plans = NULL;
i = 0;
! forboth(lc, resultRelations, lc2, subroots)
{
Index rti = lfirst_int(lc);
+ PlannerInfo *subroot = lfirst_node(PlannerInfo, lc2);
FdwRoutine *fdwroutine;
List *fdw_private;
bool direct_modify;
***************
*** 6641,6656 **** make_modifytable(PlannerInfo *root,
* so it's not a baserel; and there are also corner cases for
* updatable views where the target rel isn't a baserel.)
*/
! if (rti < root->simple_rel_array_size &&
! root->simple_rel_array[rti] != NULL)
{
! RelOptInfo *resultRel = root->simple_rel_array[rti];
fdwroutine = resultRel->fdwroutine;
}
else
{
! RangeTblEntry *rte = planner_rt_fetch(rti, root);
Assert(rte->rtekind == RTE_RELATION);
if (rte->relkind == RELKIND_FOREIGN_TABLE)
--- 6645,6660 ----
* so it's not a baserel; and there are also corner cases for
* updatable views where the target rel isn't a baserel.)
*/
! if (rti < subroot->simple_rel_array_size &&
! subroot->simple_rel_array[rti] != NULL)
{
! RelOptInfo *resultRel = subroot->simple_rel_array[rti];
fdwroutine = resultRel->fdwroutine;
}
else
{
! RangeTblEntry *rte = planner_rt_fetch(rti, subroot);
Assert(rte->rtekind == RTE_RELATION);
if (rte->relkind == RELKIND_FOREIGN_TABLE)
***************
*** 6672,6686 **** make_modifytable(PlannerInfo *root,
fdwroutine->IterateDirectModify != NULL &&
fdwroutine->EndDirectModify != NULL &&
withCheckOptionLists == NIL &&
! !has_row_triggers(root, rti, operation))
! direct_modify = fdwroutine->PlanDirectModify(root, node, rti, i);
if (direct_modify)
direct_modify_plans = bms_add_member(direct_modify_plans, i);
if (!direct_modify &&
fdwroutine != NULL &&
fdwroutine->PlanForeignModify != NULL)
! fdw_private = fdwroutine->PlanForeignModify(root, node, rti, i);
else
fdw_private = NIL;
fdw_private_list = lappend(fdw_private_list, fdw_private);
--- 6676,6690 ----
fdwroutine->IterateDirectModify != NULL &&
fdwroutine->EndDirectModify != NULL &&
withCheckOptionLists == NIL &&
! !has_row_triggers(subroot, rti, operation))
! direct_modify = fdwroutine->PlanDirectModify(subroot, node, rti, i);
if (direct_modify)
direct_modify_plans = bms_add_member(direct_modify_plans, i);
if (!direct_modify &&
fdwroutine != NULL &&
fdwroutine->PlanForeignModify != NULL)
! fdw_private = fdwroutine->PlanForeignModify(subroot, node, rti, i);
else
fdw_private = NIL;
fdw_private_list = lappend(fdw_private_list, fdw_private);
