Обсуждение: Query::targetList and RETURNING

Hi,

While working on writeable CTEs, I noticed I have to special-case the
output of a Query node frequently because in INSERT/UPDATE/DELETE query
targetList is the target list which is used for modifying the result
relation and returningList is the output of that Query.  However, this
is different from SELECT where targetList actually is the output of that
Query node.  Attached is a patch which avoids this special-casing by
making Query's targetList always be the output target list.  The target
list for the result relation is stored separately.  The patch needs a
bit more work but I'll be glad to do it if people think this is useful.

Thoughts?


Regards,
Marko Tiikkaja

*** a/src/backend/executor/functions.c
--- b/src/backend/executor/functions.c
***************
*** 1070,1078 **** check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
               (parse->commandType == CMD_INSERT ||
                parse->commandType == CMD_UPDATE ||
                parse->commandType == CMD_DELETE) &&
!              parse->returningList)
      {
!         tlist = parse->returningList;
      }
      else
      {
--- 1070,1078 ----
               (parse->commandType == CMD_INSERT ||
                parse->commandType == CMD_UPDATE ||
                parse->commandType == CMD_DELETE) &&
!              parse->hasReturning)
      {
!         tlist = parse->targetList;
      }
      else
      {
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
***************
*** 2225,2235 **** _copyQuery(Query *from)
      COPY_SCALAR_FIELD(hasDistinctOn);
      COPY_SCALAR_FIELD(hasRecursive);
      COPY_SCALAR_FIELD(hasForUpdate);
      COPY_NODE_FIELD(cteList);
      COPY_NODE_FIELD(rtable);
      COPY_NODE_FIELD(jointree);
      COPY_NODE_FIELD(targetList);
!     COPY_NODE_FIELD(returningList);
      COPY_NODE_FIELD(groupClause);
      COPY_NODE_FIELD(havingQual);
      COPY_NODE_FIELD(windowClause);
--- 2225,2236 ----
      COPY_SCALAR_FIELD(hasDistinctOn);
      COPY_SCALAR_FIELD(hasRecursive);
      COPY_SCALAR_FIELD(hasForUpdate);
+     COPY_SCALAR_FIELD(hasReturning);
      COPY_NODE_FIELD(cteList);
      COPY_NODE_FIELD(rtable);
      COPY_NODE_FIELD(jointree);
      COPY_NODE_FIELD(targetList);
!     COPY_NODE_FIELD(resultTargetList);
      COPY_NODE_FIELD(groupClause);
      COPY_NODE_FIELD(havingQual);
      COPY_NODE_FIELD(windowClause);
*** a/src/backend/nodes/equalfuncs.c
--- b/src/backend/nodes/equalfuncs.c
***************
*** 861,871 **** _equalQuery(Query *a, Query *b)
      COMPARE_SCALAR_FIELD(hasDistinctOn);
      COMPARE_SCALAR_FIELD(hasRecursive);
      COMPARE_SCALAR_FIELD(hasForUpdate);
      COMPARE_NODE_FIELD(cteList);
      COMPARE_NODE_FIELD(rtable);
      COMPARE_NODE_FIELD(jointree);
      COMPARE_NODE_FIELD(targetList);
!     COMPARE_NODE_FIELD(returningList);
      COMPARE_NODE_FIELD(groupClause);
      COMPARE_NODE_FIELD(havingQual);
      COMPARE_NODE_FIELD(windowClause);
--- 861,872 ----
      COMPARE_SCALAR_FIELD(hasDistinctOn);
      COMPARE_SCALAR_FIELD(hasRecursive);
      COMPARE_SCALAR_FIELD(hasForUpdate);
+     COMPARE_SCALAR_FIELD(hasReturning);
      COMPARE_NODE_FIELD(cteList);
      COMPARE_NODE_FIELD(rtable);
      COMPARE_NODE_FIELD(jointree);
      COMPARE_NODE_FIELD(targetList);
!     COMPARE_NODE_FIELD(resultTargetList);
      COMPARE_NODE_FIELD(groupClause);
      COMPARE_NODE_FIELD(havingQual);
      COMPARE_NODE_FIELD(windowClause);
*** a/src/backend/nodes/nodeFuncs.c
--- b/src/backend/nodes/nodeFuncs.c
***************
*** 1392,1400 **** query_tree_walker(Query *query,
  {
      Assert(query != NULL && IsA(query, Query));

!     if (walker((Node *) query->targetList, context))
          return true;
!     if (walker((Node *) query->returningList, context))
          return true;
      if (walker((Node *) query->jointree, context))
          return true;
--- 1392,1400 ----
  {
      Assert(query != NULL && IsA(query, Query));

!     if (walker((Node *) query->resultTargetList, context))
          return true;
!     if (walker((Node *) query->targetList, context))
          return true;
      if (walker((Node *) query->jointree, context))
          return true;
***************
*** 2090,2097 **** query_tree_mutator(Query *query,
          query = newquery;
      }

      MUTATE(query->targetList, query->targetList, List *);
-     MUTATE(query->returningList, query->returningList, List *);
      MUTATE(query->jointree, query->jointree, FromExpr *);
      MUTATE(query->setOperations, query->setOperations, Node *);
      MUTATE(query->havingQual, query->havingQual, Node *);
--- 2090,2097 ----
          query = newquery;
      }

+     MUTATE(query->resultTargetList, query->resultTargetList, List *);
      MUTATE(query->targetList, query->targetList, List *);
      MUTATE(query->jointree, query->jointree, FromExpr *);
      MUTATE(query->setOperations, query->setOperations, Node *);
      MUTATE(query->havingQual, query->havingQual, Node *);
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
***************
*** 1988,1998 **** _outQuery(StringInfo str, Query *node)
      WRITE_BOOL_FIELD(hasDistinctOn);
      WRITE_BOOL_FIELD(hasRecursive);
      WRITE_BOOL_FIELD(hasForUpdate);
      WRITE_NODE_FIELD(cteList);
      WRITE_NODE_FIELD(rtable);
      WRITE_NODE_FIELD(jointree);
      WRITE_NODE_FIELD(targetList);
!     WRITE_NODE_FIELD(returningList);
      WRITE_NODE_FIELD(groupClause);
      WRITE_NODE_FIELD(havingQual);
      WRITE_NODE_FIELD(windowClause);
--- 1988,1999 ----
      WRITE_BOOL_FIELD(hasDistinctOn);
      WRITE_BOOL_FIELD(hasRecursive);
      WRITE_BOOL_FIELD(hasForUpdate);
+     WRITE_BOOL_FIELD(hasReturning);
      WRITE_NODE_FIELD(cteList);
      WRITE_NODE_FIELD(rtable);
      WRITE_NODE_FIELD(jointree);
      WRITE_NODE_FIELD(targetList);
!     WRITE_NODE_FIELD(resultTargetList);
      WRITE_NODE_FIELD(groupClause);
      WRITE_NODE_FIELD(havingQual);
      WRITE_NODE_FIELD(windowClause);
*** a/src/backend/nodes/readfuncs.c
--- b/src/backend/nodes/readfuncs.c
***************
*** 204,214 **** _readQuery(void)
      READ_BOOL_FIELD(hasDistinctOn);
      READ_BOOL_FIELD(hasRecursive);
      READ_BOOL_FIELD(hasForUpdate);
      READ_NODE_FIELD(cteList);
      READ_NODE_FIELD(rtable);
      READ_NODE_FIELD(jointree);
      READ_NODE_FIELD(targetList);
!     READ_NODE_FIELD(returningList);
      READ_NODE_FIELD(groupClause);
      READ_NODE_FIELD(havingQual);
      READ_NODE_FIELD(windowClause);
--- 204,215 ----
      READ_BOOL_FIELD(hasDistinctOn);
      READ_BOOL_FIELD(hasRecursive);
      READ_BOOL_FIELD(hasForUpdate);
+     READ_BOOL_FIELD(hasReturning);
      READ_NODE_FIELD(cteList);
      READ_NODE_FIELD(rtable);
      READ_NODE_FIELD(jointree);
      READ_NODE_FIELD(targetList);
!     READ_NODE_FIELD(resultTargetList);
      READ_NODE_FIELD(groupClause);
      READ_NODE_FIELD(havingQual);
      READ_NODE_FIELD(windowClause);
*** a/src/backend/optimizer/plan/planagg.c
--- b/src/backend/optimizer/plan/planagg.c
***************
*** 488,494 **** make_agg_subplan(PlannerInfo *root, MinMaxAggInfo *info)
      subroot.parse = subparse = (Query *) copyObject(root->parse);
      subparse->commandType = CMD_SELECT;
      subparse->resultRelation = 0;
!     subparse->returningList = NIL;
      subparse->utilityStmt = NULL;
      subparse->intoClause = NULL;
      subparse->hasAggs = false;
--- 488,495 ----
      subroot.parse = subparse = (Query *) copyObject(root->parse);
      subparse->commandType = CMD_SELECT;
      subparse->resultRelation = 0;
!     subparse->hasReturning = false;
!     subparse->resultTargetList = NIL;
      subparse->utilityStmt = NULL;
      subparse->intoClause = NULL;
      subparse->hasAggs = false;
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
***************
*** 232,238 **** standard_planner(Query *parse, int cursorOptions, ParamListInfo boundParams)
      result = makeNode(PlannedStmt);

      result->commandType = parse->commandType;
!     result->hasReturning = (parse->returningList != NIL);
      result->canSetTag = parse->canSetTag;
      result->transientPlan = glob->transientPlan;
      result->planTree = top_plan;
--- 232,238 ----
      result = makeNode(PlannedStmt);

      result->commandType = parse->commandType;
!     result->hasReturning = parse->hasReturning;
      result->canSetTag = parse->canSetTag;
      result->transientPlan = glob->transientPlan;
      result->planTree = top_plan;
***************
*** 400,407 **** subquery_planner(PlannerGlobal *glob, Query *parse,
          preprocess_expression(root, (Node *) parse->targetList,
                                EXPRKIND_TARGET);

!     parse->returningList = (List *)
!         preprocess_expression(root, (Node *) parse->returningList,
                                EXPRKIND_TARGET);

      preprocess_qual_conditions(root, (Node *) parse->jointree);
--- 400,407 ----
          preprocess_expression(root, (Node *) parse->targetList,
                                EXPRKIND_TARGET);

!     parse->resultTargetList = (List *)
!         preprocess_expression(root, (Node *) parse->resultTargetList,
                                EXPRKIND_TARGET);

      preprocess_qual_conditions(root, (Node *) parse->jointree);
***************
*** 516,528 **** subquery_planner(PlannerGlobal *glob, Query *parse,
               * level (if we waited, handling inherited UPDATE/DELETE would be
               * much harder).
               */
!             if (parse->returningList)
              {
                  List       *rlist;

                  Assert(parse->resultRelation);
                  rlist = set_returning_clause_references(root->glob,
!                                                         parse->returningList,
                                                          plan,
                                                          parse->resultRelation);
                  returningLists = list_make1(rlist);
--- 516,528 ----
               * level (if we waited, handling inherited UPDATE/DELETE would be
               * much harder).
               */
!             if (parse->hasReturning)
              {
                  List       *rlist;

                  Assert(parse->resultRelation);
                  rlist = set_returning_clause_references(root->glob,
!                                                         parse->targetList,
                                                          plan,
                                                          parse->resultRelation);
                  returningLists = list_make1(rlist);
***************
*** 760,771 **** inheritance_planner(PlannerInfo *root)
          resultRelations = lappend_int(resultRelations, appinfo->child_relid);

          /* 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);
--- 760,771 ----
          resultRelations = lappend_int(resultRelations, appinfo->child_relid);

          /* Build list of per-relation RETURNING targetlists */
!         if (parse->hasReturning)
          {
              List       *rlist;

              rlist = set_returning_clause_references(root->glob,
!                                                     subroot.parse->targetList,
                                                      subplan,
                                                      appinfo->child_relid);
              returningLists = lappend(returningLists, rlist);
***************
*** 848,854 **** static Plan *
  grouping_planner(PlannerInfo *root, double tuple_fraction)
  {
      Query       *parse = root->parse;
!     List       *tlist = parse->targetList;
      int64        offset_est = 0;
      int64        count_est = 0;
      double        limit_tuples = -1.0;
--- 848,854 ----
  grouping_planner(PlannerInfo *root, double tuple_fraction)
  {
      Query       *parse = root->parse;
!     List       *tlist = (parse->commandType == CMD_SELECT ? parse->targetList : parse->resultTargetList);
      int64        offset_est = 0;
      int64        count_est = 0;
      double        limit_tuples = -1.0;
*** a/src/backend/optimizer/prep/prepjointree.c
--- b/src/backend/optimizer/prep/prepjointree.c
***************
*** 734,741 **** pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
       */
      parse->targetList = (List *)
          pullup_replace_vars((Node *) parse->targetList, &rvcontext);
!     parse->returningList = (List *)
!         pullup_replace_vars((Node *) parse->returningList, &rvcontext);
      replace_vars_in_jointree((Node *) parse->jointree, &rvcontext,
                               lowest_outer_join);
      Assert(parse->setOperations == NULL);
--- 734,741 ----
       */
      parse->targetList = (List *)
          pullup_replace_vars((Node *) parse->targetList, &rvcontext);
!     parse->resultTargetList = (List *)
!         pullup_replace_vars((Node *) parse->resultTargetList, &rvcontext);
      replace_vars_in_jointree((Node *) parse->jointree, &rvcontext,
                               lowest_outer_join);
      Assert(parse->setOperations == NULL);
*** a/src/backend/optimizer/prep/preptlist.c
--- b/src/backend/optimizer/prep/preptlist.c
***************
*** 193,204 **** preprocess_targetlist(PlannerInfo *root, List *tlist)
       * belong to the result rel don't need to be added, because they will be
       * made to refer to the actual heap tuple.
       */
!     if (parse->returningList && list_length(parse->rtable) > 1)
      {
          List       *vars;
          ListCell   *l;

!         vars = pull_var_clause((Node *) parse->returningList,
                                 PVC_INCLUDE_PLACEHOLDERS);
          foreach(l, vars)
          {
--- 193,204 ----
       * belong to the result rel don't need to be added, because they will be
       * made to refer to the actual heap tuple.
       */
!     if (parse->hasReturning && list_length(parse->rtable) > 1)
      {
          List       *vars;
          ListCell   *l;

!         vars = pull_var_clause((Node *) parse->targetList,
                                 PVC_INCLUDE_PLACEHOLDERS);
          foreach(l, vars)
          {
*** a/src/backend/optimizer/prep/prepunion.c
--- b/src/backend/optimizer/prep/prepunion.c
***************
*** 1507,1514 **** adjust_appendrel_attrs(Node *node, AppendRelInfo *appinfo)
              newnode->resultRelation = appinfo->child_relid;
              /* Fix tlist resnos too, if it's inherited UPDATE */
              if (newnode->commandType == CMD_UPDATE)
!                 newnode->targetList =
!                     adjust_inherited_tlist(newnode->targetList,
                                             appinfo);
          }
          result = (Node *) newnode;
--- 1507,1514 ----
              newnode->resultRelation = appinfo->child_relid;
              /* Fix tlist resnos too, if it's inherited UPDATE */
              if (newnode->commandType == CMD_UPDATE)
!                 newnode->resultTargetList =
!                     adjust_inherited_tlist(newnode->resultTargetList,
                                             appinfo);
          }
          result = (Node *) newnode;
*** a/src/backend/parser/analyze.c
--- b/src/backend/parser/analyze.c
***************
*** 304,310 **** transformDeleteStmt(ParseState *pstate, DeleteStmt *stmt)

      qual = transformWhereClause(pstate, stmt->whereClause, "WHERE");

!     qry->returningList = transformReturningList(pstate, stmt->returningList);

      /* done building the range table and jointree */
      qry->rtable = pstate->p_rtable;
--- 304,319 ----

      qual = transformWhereClause(pstate, stmt->whereClause, "WHERE");

!     if (stmt->returningList)
!     {
!         qry->targetList = transformReturningList(pstate, stmt->returningList);
!         qry->hasReturning = true;
!     }
!     else
!     {
!         qry->targetList = NIL;
!         qry->hasReturning = false;
!     }

      /* done building the range table and jointree */
      qry->rtable = pstate->p_rtable;
***************
*** 638,644 **** transformInsertStmt(ParseState *pstate, InsertStmt *stmt)
       * Also, mark all the target columns as needing insert permissions.
       */
      rte = pstate->p_target_rangetblentry;
!     qry->targetList = NIL;
      icols = list_head(icolumns);
      attnos = list_head(attrnos);
      foreach(lc, exprList)
--- 647,653 ----
       * Also, mark all the target columns as needing insert permissions.
       */
      rte = pstate->p_target_rangetblentry;
!     qry->resultTargetList = NIL;
      icols = list_head(icolumns);
      attnos = list_head(attrnos);
      foreach(lc, exprList)
***************
*** 656,662 **** transformInsertStmt(ParseState *pstate, InsertStmt *stmt)
                                attr_num,
                                col->name,
                                false);
!         qry->targetList = lappend(qry->targetList, tle);

          rte->modifiedCols = bms_add_member(rte->modifiedCols,
                                attr_num - FirstLowInvalidHeapAttributeNumber);
--- 665,671 ----
                                attr_num,
                                col->name,
                                false);
!         qry->resultTargetList = lappend(qry->resultTargetList, tle);

          rte->modifiedCols = bms_add_member(rte->modifiedCols,
                                attr_num - FirstLowInvalidHeapAttributeNumber);
***************
*** 677,684 **** transformInsertStmt(ParseState *pstate, InsertStmt *stmt)
          pstate->p_varnamespace = NIL;
          addRTEtoQuery(pstate, pstate->p_target_rangetblentry,
                        false, true, true);
!         qry->returningList = transformReturningList(pstate,
                                                      stmt->returningList);
      }

      /* done building the range table and jointree */
--- 686,699 ----
          pstate->p_varnamespace = NIL;
          addRTEtoQuery(pstate, pstate->p_target_rangetblentry,
                        false, true, true);
!         qry->targetList = transformReturningList(pstate,
                                                      stmt->returningList);
+         qry->hasReturning = true;
+     }
+     else
+     {
+         qry->hasReturning = false;
+         qry->targetList = NIL;
      }

      /* done building the range table and jointree */
***************
*** 1727,1737 **** transformUpdateStmt(ParseState *pstate, UpdateStmt *stmt)
       */
      transformFromClause(pstate, stmt->fromClause);

!     qry->targetList = transformTargetList(pstate, stmt->targetList);

      qual = transformWhereClause(pstate, stmt->whereClause, "WHERE");

!     qry->returningList = transformReturningList(pstate, stmt->returningList);

      qry->rtable = pstate->p_rtable;
      qry->jointree = makeFromExpr(pstate->p_joinlist, qual);
--- 1742,1761 ----
       */
      transformFromClause(pstate, stmt->fromClause);

!     qry->resultTargetList = transformTargetList(pstate, stmt->targetList);

      qual = transformWhereClause(pstate, stmt->whereClause, "WHERE");

!     if (stmt->returningList)
!     {
!         qry->hasReturning = true;
!         qry->targetList = transformReturningList(pstate, stmt->returningList);
!     }
!     else
!     {
!         qry->hasReturning = false;
!         qry->targetList = NIL;
!     }

      qry->rtable = pstate->p_rtable;
      qry->jointree = makeFromExpr(pstate->p_joinlist, qual);
***************
*** 1769,1775 **** transformUpdateStmt(ParseState *pstate, UpdateStmt *stmt)
      target_rte = pstate->p_target_rangetblentry;
      origTargetList = list_head(stmt->targetList);

!     foreach(tl, qry->targetList)
      {
          TargetEntry *tle = (TargetEntry *) lfirst(tl);
          ResTarget  *origTarget;
--- 1793,1799 ----
      target_rte = pstate->p_target_rangetblentry;
      origTargetList = list_head(stmt->targetList);

!     foreach(tl, qry->resultTargetList)
      {
          TargetEntry *tle = (TargetEntry *) lfirst(tl);
          ResTarget  *origTarget;
*** a/src/backend/rewrite/rewriteDefine.c
--- b/src/backend/rewrite/rewriteDefine.c
***************
*** 444,450 **** DefineQueryRewrite(char *rulename,
          {
              query = (Query *) lfirst(l);

!             if (!query->returningList)
                  continue;
              if (haveReturning)
                  ereport(ERROR,
--- 444,450 ----
          {
              query = (Query *) lfirst(l);

!             if (!query->hasReturning)
                  continue;
              if (haveReturning)
                  ereport(ERROR,
***************
*** 459,465 **** DefineQueryRewrite(char *rulename,
                  ereport(ERROR,
                          (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                           errmsg("RETURNING lists are not supported in non-INSTEAD rules")));
!             checkRuleResultList(query->returningList,
                                  RelationGetDescr(event_relation),
                                  false);
          }
--- 459,465 ----
                  ereport(ERROR,
                          (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                           errmsg("RETURNING lists are not supported in non-INSTEAD rules")));
!             checkRuleResultList(query->targetList,
                                  RelationGetDescr(event_relation),
                                  false);
          }
*** a/src/backend/rewrite/rewriteHandler.c
--- b/src/backend/rewrite/rewriteHandler.c
***************
*** 469,475 **** rewriteRuleAction(Query *parsetree,
                                            0,
                                            rt_fetch(new_varno,
                                                     sub_action->rtable),
!                                           parsetree->targetList,
                                            event,
                                            current_varno,
                                            NULL);
--- 469,475 ----
                                            0,
                                            rt_fetch(new_varno,
                                                     sub_action->rtable),
!                                           parsetree->resultTargetList,
                                            event,
                                            current_varno,
                                            NULL);
***************
*** 485,506 **** rewriteRuleAction(Query *parsetree,
       * the triggering query's RETURNING clause asks for.  Throw an error if
       * more than one rule has a RETURNING clause.
       */
!     if (!parsetree->returningList)
!         rule_action->returningList = NIL;
!     else if (rule_action->returningList)
      {
          if (*returning_flag)
              ereport(ERROR,
                      (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                     errmsg("cannot have RETURNING lists in multiple rules")));
          *returning_flag = true;
!         rule_action->returningList = (List *)
!             ResolveNew((Node *) parsetree->returningList,
                         parsetree->resultRelation,
                         0,
                         rt_fetch(parsetree->resultRelation,
                                  parsetree->rtable),
!                        rule_action->returningList,
                         CMD_SELECT,
                         0,
                         &rule_action->hasSubLinks);
--- 485,509 ----
       * the triggering query's RETURNING clause asks for.  Throw an error if
       * more than one rule has a RETURNING clause.
       */
!     if (!parsetree->hasReturning)
!     {
!         rule_action->hasReturning = false;
!         rule_action->targetList = NIL;
!     }
!     else if (rule_action->hasReturning)
      {
          if (*returning_flag)
              ereport(ERROR,
                      (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                     errmsg("cannot have RETURNING lists in multiple rules")));
          *returning_flag = true;
!         rule_action->targetList = (List *)
!             ResolveNew((Node *) parsetree->targetList,
                         parsetree->resultRelation,
                         0,
                         rt_fetch(parsetree->resultRelation,
                                  parsetree->rtable),
!                        rule_action->targetList,
                         CMD_SELECT,
                         0,
                         &rule_action->hasSubLinks);
***************
*** 511,517 **** rewriteRuleAction(Query *parsetree,
           */
          if (parsetree->hasSubLinks && !rule_action->hasSubLinks)
              rule_action->hasSubLinks =
!                 checkExprHasSubLink((Node *) rule_action->returningList);
      }

      return rule_action;
--- 514,520 ----
           */
          if (parsetree->hasSubLinks && !rule_action->hasSubLinks)
              rule_action->hasSubLinks =
!                 checkExprHasSubLink((Node *) rule_action->targetList);
      }

      return rule_action;
***************
*** 554,560 **** adjustJoinTreeList(Query *parsetree, bool removert, int rt_index)


  /*
!  * rewriteTargetList - rewrite INSERT/UPDATE targetlist into standard form
   *
   * This has the following responsibilities:
   *
--- 557,563 ----


  /*
!  * rewriteTargetList - rewrite INSERT/UPDATE result targetlist into standard form
   *
   * This has the following responsibilities:
   *
***************
*** 601,606 **** rewriteTargetList(Query *parsetree, Relation target_relation,
--- 604,611 ----
                  numattrs;
      ListCell   *temp;

+     Assert(commandType == CMD_INSERT || commandType == CMD_UPDATE);
+
      if (attrno_list)            /* initialize optional result list */
          *attrno_list = NIL;

***************
*** 617,623 **** rewriteTargetList(Query *parsetree, Relation target_relation,
      new_tles = (TargetEntry **) palloc0(numattrs * sizeof(TargetEntry *));
      next_junk_attrno = numattrs + 1;

!     foreach(temp, parsetree->targetList)
      {
          TargetEntry *old_tle = (TargetEntry *) lfirst(temp);

--- 622,628 ----
      new_tles = (TargetEntry **) palloc0(numattrs * sizeof(TargetEntry *));
      next_junk_attrno = numattrs + 1;

!     foreach(temp, parsetree->resultTargetList)
      {
          TargetEntry *old_tle = (TargetEntry *) lfirst(temp);

***************
*** 730,736 **** rewriteTargetList(Query *parsetree, Relation target_relation,

      pfree(new_tles);

!     parsetree->targetList = list_concat(new_tlist, junk_tlist);
  }


--- 735,741 ----

      pfree(new_tles);

!     parsetree->resultTargetList = list_concat(new_tlist, junk_tlist);
  }


***************
*** 1496,1502 **** CopyAndAddInvertedQual(Query *parsetree,
                                PRS2_NEW_VARNO,
                                0,
                                rt_fetch(rt_index, parsetree->rtable),
!                               parsetree->targetList,
                                event,
                                rt_index,
                                &parsetree->hasSubLinks);
--- 1501,1507 ----
                                PRS2_NEW_VARNO,
                                0,
                                rt_fetch(rt_index, parsetree->rtable),
!                               parsetree->resultTargetList,
                                event,
                                rt_index,
                                &parsetree->hasSubLinks);
***************
*** 1767,1773 **** RewriteQuery(Query *parsetree, List *rewrite_events)
           * will actually be executed --- it must be.)
           */
          if ((instead || qual_product != NULL) &&
!             parsetree->returningList &&
              !returning)
          {
              switch (event)
--- 1772,1778 ----
           * will actually be executed --- it must be.)
           */
          if ((instead || qual_product != NULL) &&
!             parsetree->hasReturning &&
              !returning)
          {
              switch (event)
*** a/src/backend/tcop/pquery.c
--- b/src/backend/tcop/pquery.c
***************
*** 326,332 **** ChoosePortalStrategy(List *stmts)
              {
                  if (++nSetTag > 1)
                      return PORTAL_MULTI_QUERY;    /* no need to look further */
!                 if (query->returningList == NIL)
                      return PORTAL_MULTI_QUERY;    /* no need to look further */
              }
          }
--- 326,332 ----
              {
                  if (++nSetTag > 1)
                      return PORTAL_MULTI_QUERY;    /* no need to look further */
!                 if (!query->hasReturning)
                      return PORTAL_MULTI_QUERY;    /* no need to look further */
              }
          }
***************
*** 401,408 **** FetchStatementTargetList(Node *stmt)
                  query->utilityStmt == NULL &&
                  query->intoClause == NULL)
                  return query->targetList;
!             if (query->returningList)
!                 return query->returningList;
              return NIL;
          }
      }
--- 401,408 ----
                  query->utilityStmt == NULL &&
                  query->intoClause == NULL)
                  return query->targetList;
!             if (query->hasReturning)
!                 return query->targetList;
              return NIL;
          }
      }
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
***************
*** 3125,3131 **** get_insert_query_def(Query *query, deparse_context *context)
          values_cell = NULL;
      strippedexprs = NIL;
      sep = "";
!     foreach(l, query->targetList)
      {
          TargetEntry *tle = (TargetEntry *) lfirst(l);

--- 3125,3131 ----
          values_cell = NULL;
      strippedexprs = NIL;
      sep = "";
!     foreach(l, query->resultTargetList)
      {
          TargetEntry *tle = (TargetEntry *) lfirst(l);

***************
*** 3188,3198 **** get_insert_query_def(Query *query, deparse_context *context)
      }

      /* Add RETURNING if present */
!     if (query->returningList)
      {
          appendContextKeyword(context, " RETURNING",
                               -PRETTYINDENT_STD, PRETTYINDENT_STD, 1);
!         get_target_list(query->returningList, context, NULL);
      }
  }

--- 3188,3198 ----
      }

      /* Add RETURNING if present */
!     if (query->hasReturning)
      {
          appendContextKeyword(context, " RETURNING",
                               -PRETTYINDENT_STD, PRETTYINDENT_STD, 1);
!         get_target_list(query->targetList, context, NULL);
      }
  }

***************
*** 3229,3235 **** get_update_query_def(Query *query, deparse_context *context)

      /* Add the comma separated list of 'attname = value' */
      sep = "";
!     foreach(l, query->targetList)
      {
          TargetEntry *tle = (TargetEntry *) lfirst(l);
          Node       *expr;
--- 3229,3235 ----

      /* Add the comma separated list of 'attname = value' */
      sep = "";
!     foreach(l, query->resultTargetList)
      {
          TargetEntry *tle = (TargetEntry *) lfirst(l);
          Node       *expr;
***************
*** 3271,3281 **** get_update_query_def(Query *query, deparse_context *context)
      }

      /* Add RETURNING if present */
!     if (query->returningList)
      {
          appendContextKeyword(context, " RETURNING",
                               -PRETTYINDENT_STD, PRETTYINDENT_STD, 1);
!         get_target_list(query->returningList, context, NULL);
      }
  }

--- 3271,3281 ----
      }

      /* Add RETURNING if present */
!     if (query->hasReturning)
      {
          appendContextKeyword(context, " RETURNING",
                               -PRETTYINDENT_STD, PRETTYINDENT_STD, 1);
!         get_target_list(query->targetList, context, NULL);
      }
  }

***************
*** 3319,3329 **** get_delete_query_def(Query *query, deparse_context *context)
      }

      /* Add RETURNING if present */
!     if (query->returningList)
      {
          appendContextKeyword(context, " RETURNING",
                               -PRETTYINDENT_STD, PRETTYINDENT_STD, 1);
!         get_target_list(query->returningList, context, NULL);
      }
  }

--- 3319,3329 ----
      }

      /* Add RETURNING if present */
!     if (query->hasReturning)
      {
          appendContextKeyword(context, " RETURNING",
                               -PRETTYINDENT_STD, PRETTYINDENT_STD, 1);
!         get_target_list(query->targetList, context, NULL);
      }
  }

*** a/src/backend/utils/cache/plancache.c
--- b/src/backend/utils/cache/plancache.c
***************
*** 892,899 **** PlanCacheComputeResultDesc(List *stmt_list)
              if (IsA(node, Query))
              {
                  query = (Query *) node;
!                 Assert(query->returningList);
!                 return ExecCleanTypeFromTL(query->returningList, false);
              }
              if (IsA(node, PlannedStmt))
              {
--- 892,899 ----
              if (IsA(node, Query))
              {
                  query = (Query *) node;
!                 Assert(query->hasReturning && query->targetList);
!                 return ExecCleanTypeFromTL(query->targetList, false);
              }
              if (IsA(node, PlannedStmt))
              {
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
***************
*** 119,124 **** typedef struct Query
--- 119,125 ----
      bool        hasDistinctOn;    /* distinctClause is from DISTINCT ON */
      bool        hasRecursive;    /* WITH RECURSIVE was specified */
      bool        hasForUpdate;    /* FOR UPDATE or FOR SHARE was specified */
+     bool        hasReturning;    /* targetList is from RETURNING clause */

      List       *cteList;        /* WITH list (of CommonTableExpr's) */

***************
*** 127,133 **** typedef struct Query

      List       *targetList;        /* target list (of TargetEntry) */

!     List       *returningList;    /* return-values list (of TargetEntry) */

      List       *groupClause;    /* a list of SortGroupClause's */

--- 128,134 ----

      List       *targetList;        /* target list (of TargetEntry) */

!     List       *resultTargetList; /* result relation target list (of TargetEntry) */

      List       *groupClause;    /* a list of SortGroupClause's */

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
> While working on writeable CTEs, I noticed I have to special-case the
> output of a Query node frequently because in INSERT/UPDATE/DELETE query
> targetList is the target list which is used for modifying the result
> relation and returningList is the output of that Query.  However, this
> is different from SELECT where targetList actually is the output of that
> Query node.  Attached is a patch which avoids this special-casing by
> making Query's targetList always be the output target list.  The target
> list for the result relation is stored separately.  The patch needs a
> bit more work but I'll be glad to do it if people think this is useful.

This doesn't really seem like a good idea from here.  You're changing
a decision that has something like twenty years' standing in the code,
for no real gain.  AFAICS this is just going to move the special cases
from point A to point B.
        regards, tom lane

(Sorry, forgot to CC the list)

Tom Lane wrote:
> This doesn't really seem like a good idea from here.  You're changing
> a decision that has something like twenty years' standing in the code,
> for no real gain.  AFAICS this is just going to move the special cases
> from point A to point B.

Right, but this way you only have to special-case in grouping_planner(),
and targetList always means the same thing.


Regards,
Marko Tiikkaja

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
> Tom Lane wrote:
>> This doesn't really seem like a good idea from here.  You're changing
>> a decision that has something like twenty years' standing in the code,
>> for no real gain.  AFAICS this is just going to move the special cases
>> from point A to point B.

> Right, but this way you only have to special-case in grouping_planner(),
> and targetList always means the same thing.

If you think that, it just means you have not found all the places you
need to special-case ;-).  One really obvious example is ruleutils.c,
and I rather imagine there are multiple places in the parser and
rewriter that would need attention, quite aside from whatever it does
to the planner.

If there were a clear net benefit, I'd be for changing, but I think
it's going to end up being roughly a wash.  And if it's a wash we
should not change it, because when you consider the follow-on costs
(patches not back-patching, third-party code breaking, etc) that
means we'd come out way behind.
        regards, tom lane

On Tue, Nov 10, 2009 at 2:56 PM, Marko Tiikkaja
<marko.tiikkaja@cs.helsinki.fi> wrote:
> (Sorry, forgot to CC the list)
>
> Tom Lane wrote:
>>
>> This doesn't really seem like a good idea from here.  You're changing
>> a decision that has something like twenty years' standing in the code,
>> for no real gain.  AFAICS this is just going to move the special cases
>> from point A to point B.
>
> Right, but this way you only have to special-case in grouping_planner(),
> and targetList always means the same thing.

I haven't read through the patch but I can say the existing
arrangement certainly seemed strange to me when I first saw it.

--
greg

Tom Lane wrote:
> Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
>> Tom Lane wrote:
>>> This doesn't really seem like a good idea from here.  You're changing
>>> a decision that has something like twenty years' standing in the code,
>>> for no real gain.  AFAICS this is just going to move the special cases
>>> from point A to point B.
> 
>> Right, but this way you only have to special-case in grouping_planner(),
>> and targetList always means the same thing.
> 
> If you think that, it just means you have not found all the places you
> need to special-case ;-).  One really obvious example is ruleutils.c,
> and I rather imagine there are multiple places in the parser and
> rewriter that would need attention, quite aside from whatever it does
> to the planner.

Of course there were multiple places that needed attention, but those
don't look like special-casing to me, they just have to make sure to do
what they need to on the correct list of those two, which is what they
did before.

I wouldn't care for this at all, but with things the way they are right
now, the writeable CTE patch has to do quite a few of these:

List *cteList;
if (query->cmdType != CMD_SELECT)cteList = query->returningList;
elsecteList = query->targetList;

/* do something with cteList */


With this patch, they could use targetList directly without paying any
attention to the type of the Query inside them.


Regards,
Marko Tiikkaja

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
> I wouldn't care for this at all, but with things the way they are right
> now, the writeable CTE patch has to do quite a few of these:

[ shrug... ]  How many is "quite a few"?  In a quick search for existing
references to targetList in the planner, it looked to me like the
majority were places that wouldn't be relevant for writable CTEs anyway.
For example, none of the references in allpaths.c are, because they have
to do with deciding whether quals can be pushed down into the subquery.
And the answer to that, for a non-SELECT CTE, is always "no".

> if (query->cmdType != CMD_SELECT)
>     cteList = query->returningList;
> else
>     cteList = query->targetList;

Just a thought ... where you do need this, would it be better to phrase
it as

if (query->returningList)cteList = query->returningList;
elsecteList = query->targetList;

?  I'm not sure myself, but it's something to consider.
        regards, tom lane

Tom Lane wrote:
> Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
>> I wouldn't care for this at all, but with things the way they are right
>> now, the writeable CTE patch has to do quite a few of these:
> 
> [ shrug... ]  How many is "quite a few"?  In a quick search for existing
> references to targetList in the planner, it looked to me like the
> majority were places that wouldn't be relevant for writable CTEs anyway.
> For example, none of the references in allpaths.c are, because they have
> to do with deciding whether quals can be pushed down into the subquery.
> And the answer to that, for a non-SELECT CTE, is always "no".

It appears we have four of those at the moment (hmm.. I thought there
were more).

> Just a thought ... where you do need this, would it be better to phrase
> it as
> 
> if (query->returningList)
>     cteList = query->returningList;
> else
>     cteList = query->targetList;
> 
> ?  I'm not sure myself, but it's something to consider.

My initial thought is that this won't work because there might not be a
RETURNING clause, but I'm not sure.


Regards,
Marko Tiikkaja

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
> Tom Lane wrote:
>> if (query->returningList)
>>    cteList = query->returningList;
>> else
>>    cteList = query->targetList;

> My initial thought is that this won't work because there might not be a
> RETURNING clause, but I'm not sure.

Hm, would we allow DML without RETURNING in CTEs?  I'm not sure I see
the point of that.  But in any case, the other way is fine.
        regards, tom lane