From 9fa82c980935ef4aee18fabe8da20ae2198b052a Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 25 Apr 2012 20:20:33 -0400 Subject: [PATCH] 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 | 75 ++++++++++++++++++++----- src/include/optimizer/planmain.h | 4 -- src/test/regress/expected/with.out | 42 ++++++++++++++ src/test/regress/sql/with.sql | 6 ++ 6 files changed, 116 insertions(+), 50 deletions(-) diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index de2779a1a2..c34b9b8c38 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -4587,16 +4587,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 14b251037c..0b1ee971df 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -529,22 +529,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, - parse->returningList, - plan, - parse->resultRelation); - returningLists = list_make1(rlist); - } + returningLists = list_make1(parse->returningList); else returningLists = NIL; @@ -889,15 +877,8 @@ inheritance_planner(PlannerInfo *root) /* Build list of per-relation RETURNING targetlists */ if (parse->returningList) - { - List *rlist; - - rlist = set_returning_clause_references(&subroot, - 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 9e347ce736..db301e6c59 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -121,6 +121,11 @@ static Node *fix_upper_expr(PlannerInfo *root, int rtoffset); static Node *fix_upper_expr_mutator(Node *node, fix_upper_expr_context *context); +static List *set_returning_clause_references(PlannerInfo *root, + 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, PlannerInfo *context); @@ -548,13 +553,50 @@ set_plan_refs(PlannerInfo *root, 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(root, + 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; @@ -1532,6 +1574,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; @@ -1691,25 +1734,31 @@ 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_VAR. + * adjusted RETURNING list, result-table Vars will have their original + * varno (plus rtoffset), but Vars for other rels will have varno OUTER_VAR. * * We also must perform opcode lookup and add regclass OIDs to * root->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: the given 'root' is for the parent query level, not the 'topplan'. + * This does not matter currently since we only access the dependency-item + * lists in root->glob, but it would need some hacking if we wanted a root + * that actually matches the subplan. + * + * Note: resultRelation is not yet adjusted by rtoffset. */ -List * +static List * set_returning_clause_references(PlannerInfo *root, List *rlist, Plan *topplan, - Index resultRelation) + Index resultRelation, + int rtoffset) { indexed_tlist *itlist; @@ -1734,7 +1783,7 @@ set_returning_clause_references(PlannerInfo *root, itlist, NULL, resultRelation, - 0); + rtoffset); pfree(itlist); diff --git a/src/include/optimizer/planmain.h b/src/include/optimizer/planmain.h index 47cc39cf1d..5a9e677f94 100644 --- a/src/include/optimizer/planmain.h +++ b/src/include/optimizer/planmain.h @@ -120,10 +120,6 @@ extern List *remove_useless_joins(PlannerInfo *root, List *joinlist); * prototypes for plan/setrefs.c */ extern Plan *set_plan_references(PlannerInfo *root, Plan *plan); -extern List *set_returning_clause_references(PlannerInfo *root, - 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 fae92cd37b..57e178afa9 100644 --- a/src/test/regress/expected/with.out +++ b/src/test/regress/expected/with.out @@ -1857,6 +1857,48 @@ 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 +(34 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 1479422c9c..9c6732b961 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 -- GitLab