From dc7521dcb9410167a4748d9702bfe05746d7d27a Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 25 Apr 2012 20:20:43 -0400 Subject: Fix planner's handling of RETURNING lists in writable CTEs. setrefs.c failed to do "rtoffset" adjustment of Vars in RETURNING lists, which meant they were left with the wrong varnos when the RETURNING list was in a subquery. That was never possible before writable CTEs, of course, but now it's broken. The executor fails to notice any problem because ExecEvalVar just references the ecxt_scantuple for any normal varno; but EXPLAIN breaks when the varno is wrong, as illustrated in a recent complaint from Bartosz Dmytrak. Since the eventual rtoffset of the subquery is not known at the time we are preparing its plan node, the previous scheme of executing set_returning_clause_references() at that time cannot handle this adjustment. Fortunately, it turns out that we don't really need to do it that way, because all the needed information is available during normal setrefs.c execution; we just have to dig it out of the ModifyTable node. So, do that, and get rid of the kluge of early setrefs processing of RETURNING lists. (This is a little bit of a cheat in the case of inherited UPDATE/DELETE, because we are not passing a "root" struct that corresponds exactly to what the subplan was built with. But that doesn't matter, and anyway this is less ugly than early setrefs processing was.) Back-patch to 9.1, where the problem became possible to hit. --- src/backend/optimizer/plan/createplan.c | 12 +----- src/backend/optimizer/plan/planner.c | 27 ++----------- src/backend/optimizer/plan/setrefs.c | 70 +++++++++++++++++++++++++++------ src/include/optimizer/planmain.h | 4 -- src/test/regress/expected/with.out | 49 +++++++++++++++++++++++ src/test/regress/sql/with.sql | 6 +++ 6 files changed, 118 insertions(+), 50 deletions(-) (limited to 'src') diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index f8a22a1c5e9..affcf264c35 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -4416,16 +4416,8 @@ make_modifytable(CmdType operation, bool canSetTag, node->plan.lefttree = NULL; node->plan.righttree = NULL; node->plan.qual = NIL; - - /* - * Set up the visible plan targetlist as being the same as the first - * RETURNING list. This is for the use of EXPLAIN; the executor won't pay - * any attention to the targetlist. - */ - if (returningLists) - node->plan.targetlist = copyObject(linitial(returningLists)); - else - node->plan.targetlist = NIL; + /* setrefs.c will fill in the targetlist, if needed */ + node->plan.targetlist = NIL; node->operation = operation; node->canSetTag = canSetTag; diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index b7e02132ce5..5b4ce491a5c 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -544,22 +544,10 @@ subquery_planner(PlannerGlobal *glob, Query *parse, List *rowMarks; /* - * Deal with the RETURNING clause if any. It's convenient to pass - * the returningList through setrefs.c now rather than at top - * level (if we waited, handling inherited UPDATE/DELETE would be - * much harder). + * Set up the RETURNING list-of-lists, if needed. */ if (parse->returningList) - { - List *rlist; - - Assert(parse->resultRelation); - rlist = set_returning_clause_references(root->glob, - parse->returningList, - plan, - parse->resultRelation); - returningLists = list_make1(rlist); - } + returningLists = list_make1(parse->returningList); else returningLists = NIL; @@ -795,15 +783,8 @@ inheritance_planner(PlannerInfo *root) /* Build list of per-relation RETURNING targetlists */ if (parse->returningList) - { - List *rlist; - - rlist = set_returning_clause_references(root->glob, - subroot.parse->returningList, - subplan, - appinfo->child_relid); - returningLists = lappend(returningLists, rlist); - } + returningLists = lappend(returningLists, + subroot.parse->returningList); } /* Mark result as unordered (probably unnecessary) */ diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c index 01d2cec562f..da5a3fadf45 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -116,6 +116,11 @@ static Node *fix_upper_expr(PlannerGlobal *glob, int rtoffset); static Node *fix_upper_expr_mutator(Node *node, fix_upper_expr_context *context); +static List *set_returning_clause_references(PlannerGlobal *glob, + List *rlist, + Plan *topplan, + Index resultRelation, + int rtoffset); static bool fix_opfuncids_walker(Node *node, void *context); static bool extract_query_dependencies_walker(Node *node, PlannerGlobal *context); @@ -530,13 +535,50 @@ set_plan_refs(PlannerGlobal *glob, Plan *plan, int rtoffset) { ModifyTable *splan = (ModifyTable *) plan; - /* - * planner.c already called set_returning_clause_references, - * so we should not process either the targetlist or the - * returningLists. - */ + Assert(splan->plan.targetlist == NIL); Assert(splan->plan.qual == NIL); + if (splan->returningLists) + { + List *newRL = NIL; + ListCell *lcrl, + *lcrr, + *lcp; + + /* + * Pass each per-subplan returningList through + * set_returning_clause_references(). + */ + Assert(list_length(splan->returningLists) == list_length(splan->resultRelations)); + Assert(list_length(splan->returningLists) == list_length(splan->plans)); + forthree(lcrl, splan->returningLists, + lcrr, splan->resultRelations, + lcp, splan->plans) + { + List *rlist = (List *) lfirst(lcrl); + Index resultrel = lfirst_int(lcrr); + Plan *subplan = (Plan *) lfirst(lcp); + + rlist = set_returning_clause_references(glob, + rlist, + subplan, + resultrel, + rtoffset); + newRL = lappend(newRL, rlist); + } + splan->returningLists = newRL; + + /* + * Set up the visible plan targetlist as being the same as + * the first RETURNING list. This is for the use of + * EXPLAIN; the executor won't pay any attention to the + * targetlist. We postpone this step until here so that + * we don't have to do set_returning_clause_references() + * twice on identical targetlists. + */ + splan->plan.targetlist = copyObject(linitial(newRL)); + } + foreach(l, splan->resultRelations) { lfirst_int(l) += rtoffset; @@ -1463,6 +1505,7 @@ fix_join_expr_mutator(Node *node, fix_join_expr_context *context) if (var->varno == context->acceptable_rel) { var = copyVar(var); + var->varno += context->rtoffset; if (var->varnoold > 0) var->varnoold += context->rtoffset; return (Node *) var; @@ -1619,25 +1662,26 @@ fix_upper_expr_mutator(Node *node, fix_upper_expr_context *context) * entries in the top subplan's targetlist. Vars referencing the result * table should be left alone, however (the executor will evaluate them * using the actual heap tuple, after firing triggers if any). In the - * adjusted RETURNING list, result-table Vars will still have their - * original varno, but Vars for other rels will have varno OUTER. + * adjusted RETURNING list, result-table Vars will have their original + * varno (plus rtoffset), but Vars for other rels will have varno OUTER. * * We also must perform opcode lookup and add regclass OIDs to * glob->relationOids. * * 'rlist': the RETURNING targetlist to be fixed * 'topplan': the top subplan node that will be just below the ModifyTable - * node (note it's not yet passed through set_plan_references) + * node (note it's not yet passed through set_plan_refs) * 'resultRelation': RT index of the associated result relation + * 'rtoffset': how much to increment varnos by * - * Note: we assume that result relations will have rtoffset zero, that is, - * they are not coming from a subplan. + * Note: resultRelation is not yet adjusted by rtoffset. */ -List * +static List * set_returning_clause_references(PlannerGlobal *glob, List *rlist, Plan *topplan, - Index resultRelation) + Index resultRelation, + int rtoffset) { indexed_tlist *itlist; @@ -1662,7 +1706,7 @@ set_returning_clause_references(PlannerGlobal *glob, itlist, NULL, resultRelation, - 0); + rtoffset); pfree(itlist); diff --git a/src/include/optimizer/planmain.h b/src/include/optimizer/planmain.h index 69ba6b6923f..2f9bc032c35 100644 --- a/src/include/optimizer/planmain.h +++ b/src/include/optimizer/planmain.h @@ -122,10 +122,6 @@ extern Plan *set_plan_references(PlannerGlobal *glob, Plan *plan, List *rtable, List *rowmarks); -extern List *set_returning_clause_references(PlannerGlobal *glob, - List *rlist, - Plan *topplan, - Index resultRelation); extern void fix_opfuncids(Node *node); extern void set_opfuncid(OpExpr *opexpr); extern void set_sa_opfuncid(ScalarArrayOpExpr *opexpr); diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.out index 148ddcb25e9..d8c42896cd0 100644 --- a/src/test/regress/expected/with.out +++ b/src/test/regress/expected/with.out @@ -1856,6 +1856,55 @@ SELECT * FROM parent; 42 | new2 (5 rows) +-- check EXPLAIN VERBOSE for a wCTE with RETURNING +EXPLAIN (VERBOSE, COSTS OFF) +WITH wcte AS ( INSERT INTO int8_tbl VALUES ( 42, 47 ) RETURNING q2 ) +DELETE FROM a USING wcte WHERE aa = q2; + QUERY PLAN +-------------------------------------------------- + Delete on public.a + CTE wcte + -> Insert on public.int8_tbl + Output: int8_tbl.q2 + -> Result + Output: 42::bigint, 47::bigint + -> Nested Loop + Output: public.a.ctid, wcte.* + Join Filter: (public.a.aa = wcte.q2) + -> Seq Scan on public.a + Output: public.a.ctid, public.a.aa + -> CTE Scan on wcte + Output: wcte.*, wcte.q2 + -> Nested Loop + Output: public.a.ctid, wcte.* + Join Filter: (public.a.aa = wcte.q2) + -> Seq Scan on public.b a + Output: public.a.ctid, public.a.aa + -> CTE Scan on wcte + Output: wcte.*, wcte.q2 + -> Nested Loop + Output: public.a.ctid, wcte.* + Join Filter: (public.a.aa = wcte.q2) + -> Seq Scan on public.c a + Output: public.a.ctid, public.a.aa + -> CTE Scan on wcte + Output: wcte.*, wcte.q2 + -> Nested Loop + Output: public.a.ctid, wcte.* + Join Filter: (public.a.aa = wcte.q2) + -> Seq Scan on public.d a + Output: public.a.ctid, public.a.aa + -> CTE Scan on wcte + Output: wcte.*, wcte.q2 + -> Nested Loop + Output: public.a.ctid, wcte.* + Join Filter: (public.a.aa = wcte.q2) + -> Seq Scan on public.inhe a + Output: public.a.ctid, public.a.aa + -> CTE Scan on wcte + Output: wcte.*, wcte.q2 +(41 rows) + -- error cases -- data-modifying WITH tries to use its own output WITH RECURSIVE t AS ( diff --git a/src/test/regress/sql/with.sql b/src/test/regress/sql/with.sql index 1479422c9c6..9c6732b9611 100644 --- a/src/test/regress/sql/with.sql +++ b/src/test/regress/sql/with.sql @@ -791,6 +791,12 @@ DELETE FROM parent USING wcte WHERE id = newid; SELECT * FROM parent; +-- check EXPLAIN VERBOSE for a wCTE with RETURNING + +EXPLAIN (VERBOSE, COSTS OFF) +WITH wcte AS ( INSERT INTO int8_tbl VALUES ( 42, 47 ) RETURNING q2 ) +DELETE FROM a USING wcte WHERE aa = q2; + -- error cases -- data-modifying WITH tries to use its own output -- cgit v1.2.3