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.

Author: tglsfdc

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;

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) */

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);
 

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);

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 (

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