Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables - Mailing list pgsql-hackers

Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
> [ fix-rewrite-tlist-v4.patch ]

I started reviewing this patch.  I did not much like the fact that it
effectively moved rewriteTargetListUD to a different file and renamed it.
That seems like unnecessary code churn, plus it breaks the analogy with
rewriteTargetListIU, plus it will make back-patching harder (since that
code isn't exactly the same in back branches).  I see little reason why
we can't leave it where it is and just make it non-static.  It's not like
there's no other parts of the rewriter that the planner calls.

I revised the patch along that line, and while at it, refactored
preptlist.c a bit to eliminate repeated heap_opens of the target
relation.  I've not really reviewed any other aspects of the patch
yet, but in the meantime, does anyone object to proceeding this way?

            regards, tom lane

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 4339bbf..f90a6cf 100644
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
*************** update bar set f2 = f2 + 100 returning *
*** 7022,7027 ****
--- 7022,7086 ----
    7 | 277
  (6 rows)

+ -- Test that UPDATE/DELETE with inherited target works with row-level triggers
+ CREATE TRIGGER trig_row_before
+ BEFORE UPDATE OR DELETE ON bar2
+ FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
+ CREATE TRIGGER trig_row_after
+ AFTER UPDATE OR DELETE ON bar2
+ FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
+ explain (verbose, costs off)
+ update bar set f2 = f2 + 100;
+                                       QUERY PLAN
+ --------------------------------------------------------------------------------------
+  Update on public.bar
+    Update on public.bar
+    Foreign Update on public.bar2
+      Remote SQL: UPDATE public.loct2 SET f2 = $2 WHERE ctid = $1 RETURNING f1, f2, f3
+    ->  Seq Scan on public.bar
+          Output: bar.f1, (bar.f2 + 100), bar.ctid
+    ->  Foreign Scan on public.bar2
+          Output: bar2.f1, (bar2.f2 + 100), bar2.f3, bar2.ctid, bar2.*
+          Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR UPDATE
+ (9 rows)
+
+ update bar set f2 = f2 + 100;
+ NOTICE:  trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2
+ NOTICE:  OLD: (3,333,33),NEW: (3,433,33)
+ NOTICE:  trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2
+ NOTICE:  OLD: (4,344,44),NEW: (4,444,44)
+ NOTICE:  trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2
+ NOTICE:  OLD: (7,277,77),NEW: (7,377,77)
+ NOTICE:  trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2
+ NOTICE:  OLD: (3,333,33),NEW: (3,433,33)
+ NOTICE:  trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2
+ NOTICE:  OLD: (4,344,44),NEW: (4,444,44)
+ NOTICE:  trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2
+ NOTICE:  OLD: (7,277,77),NEW: (7,377,77)
+ explain (verbose, costs off)
+ delete from bar where f2 < 400;
+                                          QUERY PLAN
+ ---------------------------------------------------------------------------------------------
+  Delete on public.bar
+    Delete on public.bar
+    Foreign Delete on public.bar2
+      Remote SQL: DELETE FROM public.loct2 WHERE ctid = $1 RETURNING f1, f2, f3
+    ->  Seq Scan on public.bar
+          Output: bar.ctid
+          Filter: (bar.f2 < 400)
+    ->  Foreign Scan on public.bar2
+          Output: bar2.ctid, bar2.*
+          Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 WHERE ((f2 < 400)) FOR UPDATE
+ (10 rows)
+
+ delete from bar where f2 < 400;
+ NOTICE:  trig_row_before(23, skidoo) BEFORE ROW DELETE ON bar2
+ NOTICE:  OLD: (7,377,77)
+ NOTICE:  trig_row_after(23, skidoo) AFTER ROW DELETE ON bar2
+ NOTICE:  OLD: (7,377,77)
+ -- cleanup
+ DROP TRIGGER trig_row_before ON bar2;
+ DROP TRIGGER trig_row_after ON bar2;
  drop table foo cascade;
  NOTICE:  drop cascades to foreign table foo2
  drop table bar cascade;
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index ddfec79..8e1bd89 100644
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
*************** explain (verbose, costs off)
*** 1656,1661 ****
--- 1656,1681 ----
  update bar set f2 = f2 + 100 returning *;
  update bar set f2 = f2 + 100 returning *;

+ -- Test that UPDATE/DELETE with inherited target works with row-level triggers
+ CREATE TRIGGER trig_row_before
+ BEFORE UPDATE OR DELETE ON bar2
+ FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
+
+ CREATE TRIGGER trig_row_after
+ AFTER UPDATE OR DELETE ON bar2
+ FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
+
+ explain (verbose, costs off)
+ update bar set f2 = f2 + 100;
+ update bar set f2 = f2 + 100;
+
+ explain (verbose, costs off)
+ delete from bar where f2 < 400;
+ delete from bar where f2 < 400;
+
+ -- cleanup
+ DROP TRIGGER trig_row_before ON bar2;
+ DROP TRIGGER trig_row_after ON bar2;
  drop table foo cascade;
  drop table bar cascade;
  drop table loct1;
diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
index a2f8137..e610fa0 100644
*** a/doc/src/sgml/fdwhandler.sgml
--- b/doc/src/sgml/fdwhandler.sgml
*************** AddForeignUpdateTargets(Query *parsetree
*** 428,438 ****
       Avoid using names matching <literal>ctid<replaceable>N</replaceable></literal>,
       <literal>wholerow</literal>, or
       <literal>wholerow<replaceable>N</replaceable></literal>, as the core system can
!      generate junk columns of these names.
      </para>

      <para>
!      This function is called in the rewriter, not the planner, so the
       information available is a bit different from that available to the
       planning routines.
       <literal>parsetree</literal> is the parse tree for the <command>UPDATE</command> or
--- 428,440 ----
       Avoid using names matching <literal>ctid<replaceable>N</replaceable></literal>,
       <literal>wholerow</literal>, or
       <literal>wholerow<replaceable>N</replaceable></literal>, as the core system can
!      generate junk columns of these names.  Also, as it assumes that those
!      expressions have already been simplified enough to execute,
!      apply <function>eval_const_expressions</function> if necessary.
      </para>

      <para>
!      Although this function is called in the planner, the
       information available is a bit different from that available to the
       planning routines.
       <literal>parsetree</literal> is the parse tree for the <command>UPDATE</command> or
diff --git a/doc/src/sgml/rules.sgml b/doc/src/sgml/rules.sgml
index 2074fcc..3372b1a 100644
*** a/doc/src/sgml/rules.sgml
--- b/doc/src/sgml/rules.sgml
***************
*** 167,178 ****

      <para>
          <command>DELETE</command> commands don't need a normal target list
!         because they don't produce any result.  Instead, the rule system
          adds a special <acronym>CTID</acronym> entry to the empty target list,
          to allow the executor to find the row to be deleted.
          (<acronym>CTID</acronym> is added when the result relation is an ordinary
!         table.  If it is a view, a whole-row variable is added instead,
!         as described in <xref linkend="rules-views-update"/>.)
      </para>

      <para>
--- 167,178 ----

      <para>
          <command>DELETE</command> commands don't need a normal target list
!         because they don't produce any result.  Instead, the planner
          adds a special <acronym>CTID</acronym> entry to the empty target list,
          to allow the executor to find the row to be deleted.
          (<acronym>CTID</acronym> is added when the result relation is an ordinary
!         table.  If it is a view, a whole-row variable is added instead, by
!         the rule system, as described in <xref linkend="rules-views-update"/>.)
      </para>

      <para>
***************
*** 194,200 ****
          column = expression</literal> part of the command.  The planner will
          handle missing columns by inserting expressions that copy the values
          from the old row into the new one.  Just as for <command>DELETE</command>,
!         the rule system adds a <acronym>CTID</acronym> or whole-row variable so that
          the executor can identify the old row to be updated.
      </para>

--- 194,200 ----
          column = expression</literal> part of the command.  The planner will
          handle missing columns by inserting expressions that copy the values
          from the old row into the new one.  Just as for <command>DELETE</command>,
!         a <acronym>CTID</acronym> or whole-row variable is added so that
          the executor can identify the old row to be updated.
      </para>

diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 503b89f..1f5ef02 100644
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
*************** ExecModifyTable(PlanState *pstate)
*** 1699,1704 ****
--- 1699,1705 ----
          EvalPlanQualSetSlot(&node->mt_epqstate, planSlot);
          slot = planSlot;

+         tupleid = NULL;
          oldtuple = NULL;
          if (junkfilter != NULL)
          {
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index f6b8bbf..ef2eaea 100644
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
*************** grouping_planner(PlannerInfo *root, bool
*** 1555,1561 ****
                   double tuple_fraction)
  {
      Query       *parse = root->parse;
!     List       *tlist = parse->targetList;
      int64        offset_est = 0;
      int64        count_est = 0;
      double        limit_tuples = -1.0;
--- 1555,1561 ----
                   double tuple_fraction)
  {
      Query       *parse = root->parse;
!     List       *tlist;
      int64        offset_est = 0;
      int64        count_est = 0;
      double        limit_tuples = -1.0;
*************** grouping_planner(PlannerInfo *root, bool
*** 1685,1697 ****
          }

          /* Preprocess targetlist */
!         tlist = preprocess_targetlist(root, tlist);
!
!         if (parse->onConflict)
!             parse->onConflict->onConflictSet =
!                 preprocess_onconflict_targetlist(parse->onConflict->onConflictSet,
!                                                  parse->resultRelation,
!                                                  parse->rtable);

          /*
           * We are now done hacking up the query's targetlist.  Most of the
--- 1685,1691 ----
          }

          /* Preprocess targetlist */
!         tlist = preprocess_targetlist(root);

          /*
           * We are now done hacking up the query's targetlist.  Most of the
diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c
index d7db32e..44f3ae8 100644
*** a/src/backend/optimizer/prep/preptlist.c
--- b/src/backend/optimizer/prep/preptlist.c
***************
*** 4,23 ****
   *      Routines to preprocess the parse tree target list
   *
   * For INSERT and UPDATE queries, the targetlist must contain an entry for
!  * each attribute of the target relation in the correct order.  For all query
!  * types, we may need to add junk tlist entries for Vars used in the RETURNING
!  * list and row ID information needed for SELECT FOR UPDATE locking and/or
!  * EvalPlanQual checking.
   *
!  * The rewriter's rewriteTargetListIU and rewriteTargetListUD routines
!  * also do preprocessing of the targetlist.  The division of labor between
!  * here and there is partially historical, but it's not entirely arbitrary.
!  * In particular, consider an UPDATE across an inheritance tree.  What the
!  * rewriter does need be done only once (because it depends only on the
!  * properties of the parent relation).  What's done here has to be done over
!  * again for each child relation, because it depends on the column list of
!  * the child, which might have more columns and/or a different column order
!  * than the parent.
   *
   * The fact that rewriteTargetListIU sorts non-resjunk tlist entries by column
   * position, which expand_targetlist depends on, violates the above comment
--- 4,25 ----
   *      Routines to preprocess the parse tree target list
   *
   * For INSERT and UPDATE queries, the targetlist must contain an entry for
!  * each attribute of the target relation in the correct order.  For UPDATE and
!  * DELETE queries, it must also contain a junk tlist entry needed to allow the
!  * executor to identify the physical locations of the rows to be updated or
!  * deleted.  For all query types, we may need to add junk tlist entries for
!  * Vars used in the RETURNING list and row ID information needed for SELECT
!  * FOR UPDATE locking and/or EvalPlanQual checking.
   *
!  * The rewriter's rewriteTargetListIU routine also does preprocessing of the
!  * targetlist.  The division of labor between here and there is partially
!  * historical, but it's not entirely arbitrary.  In particular, consider an
!  * UPDATE across an inheritance tree.  What rewriteTargetListIU does need be
!  * done only once (because it depends only on the properties of the parent
!  * relation).  What's done here has to be done over again for each child
!  * relation, because it depends on the relation type and column list of the
!  * child, which might be of a different type, or have more columns and/or a
!  * different column order than the parent.
   *
   * The fact that rewriteTargetListIU sorts non-resjunk tlist entries by column
   * position, which expand_targetlist depends on, violates the above comment
***************
*** 47,57 ****
  #include "optimizer/var.h"
  #include "parser/parsetree.h"
  #include "parser/parse_coerce.h"
  #include "utils/rel.h"


  static List *expand_targetlist(List *tlist, int command_type,
!                   Index result_relation, List *range_table);


  /*
--- 49,60 ----
  #include "optimizer/var.h"
  #include "parser/parsetree.h"
  #include "parser/parse_coerce.h"
+ #include "rewrite/rewriteHandler.h"
  #include "utils/rel.h"


  static List *expand_targetlist(List *tlist, int command_type,
!                   Index result_relation, Relation rel);


  /*
*************** static List *expand_targetlist(List *tli
*** 59,94 ****
   *      Driver for preprocessing the parse tree targetlist.
   *
   *      Returns the new targetlist.
   */
  List *
! preprocess_targetlist(PlannerInfo *root, List *tlist)
  {
      Query       *parse = root->parse;
      int            result_relation = parse->resultRelation;
      List       *range_table = parse->rtable;
      CmdType        command_type = parse->commandType;
      ListCell   *lc;

      /*
!      * Sanity check: if there is a result relation, it'd better be a real
!      * relation not a subquery.  Else parser or rewriter messed up.
       */
      if (result_relation)
      {
!         RangeTblEntry *rte = rt_fetch(result_relation, range_table);

!         if (rte->subquery != NULL || rte->relid == InvalidOid)
!             elog(ERROR, "subquery cannot be result relation");
      }

      /*
       * for heap_form_tuple to work, the targetlist must match the exact order
       * of the attributes. We also need to fill in any missing attributes. -ay
       * 10/94
       */
      if (command_type == CMD_INSERT || command_type == CMD_UPDATE)
          tlist = expand_targetlist(tlist, command_type,
!                                   result_relation, range_table);

      /*
       * Add necessary junk columns for rowmarked rels.  These values are needed
--- 62,120 ----
   *      Driver for preprocessing the parse tree targetlist.
   *
   *      Returns the new targetlist.
+  *
+  * As a side effect, if there's an ON CONFLICT UPDATE clause, its targetlist
+  * is also preprocessed (and updated in-place).
   */
  List *
! preprocess_targetlist(PlannerInfo *root)
  {
      Query       *parse = root->parse;
      int            result_relation = parse->resultRelation;
      List       *range_table = parse->rtable;
      CmdType        command_type = parse->commandType;
+     RangeTblEntry *target_rte = NULL;
+     Relation    target_relation = NULL;
+     List       *tlist;
      ListCell   *lc;

      /*
!      * If there is a result relation, open it so we can look for missing
!      * columns and so on.  We assume that previous code already acquired at
!      * least AccessShareLock on the relation, so we need no lock here.
       */
      if (result_relation)
      {
!         target_rte = rt_fetch(result_relation, range_table);

!         /*
!          * Sanity check: it'd better be a real relation not, say, a subquery.
!          * Else parser or rewriter messed up.
!          */
!         if (target_rte->rtekind != RTE_RELATION)
!             elog(ERROR, "result relation must be a regular relation");
!
!         target_relation = heap_open(target_rte->relid, NoLock);
      }
+     else
+         Assert(command_type == CMD_SELECT);
+
+     /*
+      * For UPDATE/DELETE, add any junk column(s) needed to allow the executor
+      * to identify the rows to be updated or deleted.
+      */
+     if (command_type == CMD_UPDATE || command_type == CMD_DELETE)
+         rewriteTargetListUD(parse, target_rte, target_relation);

      /*
       * for heap_form_tuple to work, the targetlist must match the exact order
       * of the attributes. We also need to fill in any missing attributes. -ay
       * 10/94
       */
+     tlist = parse->targetList;
      if (command_type == CMD_INSERT || command_type == CMD_UPDATE)
          tlist = expand_targetlist(tlist, command_type,
!                                   result_relation, target_relation);

      /*
       * Add necessary junk columns for rowmarked rels.  These values are needed
*************** preprocess_targetlist(PlannerInfo *root,
*** 193,211 ****
          list_free(vars);
      }

!     return tlist;
! }

! /*
!  * preprocess_onconflict_targetlist
!  *      Process ON CONFLICT SET targetlist.
!  *
!  *      Returns the new targetlist.
!  */
! List *
! preprocess_onconflict_targetlist(List *tlist, int result_relation, List *range_table)
! {
!     return expand_targetlist(tlist, CMD_UPDATE, result_relation, range_table);
  }


--- 219,239 ----
          list_free(vars);
      }

!     /*
!      * If there's an ON CONFLICT UPDATE clause, preprocess its targetlist too
!      * while we have the relation open.
!      */
!     if (parse->onConflict)
!         parse->onConflict->onConflictSet =
!             expand_targetlist(parse->onConflict->onConflictSet,
!                               CMD_UPDATE,
!                               result_relation,
!                               target_relation);

!     if (target_relation)
!         heap_close(target_relation, NoLock);
!
!     return tlist;
  }


*************** preprocess_onconflict_targetlist(List *t
*** 223,233 ****
   */
  static List *
  expand_targetlist(List *tlist, int command_type,
!                   Index result_relation, List *range_table)
  {
      List       *new_tlist = NIL;
      ListCell   *tlist_item;
-     Relation    rel;
      int            attrno,
                  numattrs;

--- 251,260 ----
   */
  static List *
  expand_targetlist(List *tlist, int command_type,
!                   Index result_relation, Relation rel)
  {
      List       *new_tlist = NIL;
      ListCell   *tlist_item;
      int            attrno,
                  numattrs;

*************** expand_targetlist(List *tlist, int comma
*** 238,249 ****
       * order; but we have to insert TLEs for any missing attributes.
       *
       * Scan the tuple description in the relation's relcache entry to make
!      * sure we have all the user attributes in the right order.  We assume
!      * that the rewriter already acquired at least AccessShareLock on the
!      * relation, so we need no lock here.
       */
-     rel = heap_open(getrelid(result_relation, range_table), NoLock);
-
      numattrs = RelationGetNumberOfAttributes(rel);

      for (attrno = 1; attrno <= numattrs; attrno++)
--- 265,272 ----
       * order; but we have to insert TLEs for any missing attributes.
       *
       * Scan the tuple description in the relation's relcache entry to make
!      * sure we have all the user attributes in the right order.
       */
      numattrs = RelationGetNumberOfAttributes(rel);

      for (attrno = 1; attrno <= numattrs; attrno++)
*************** expand_targetlist(List *tlist, int comma
*** 386,393 ****
          tlist_item = lnext(tlist_item);
      }

-     heap_close(rel, NoLock);
-
      return new_tlist;
  }

--- 409,414 ----
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index c2bc3ad..65f1b8e 100644
*** a/src/backend/rewrite/rewriteHandler.c
--- b/src/backend/rewrite/rewriteHandler.c
*************** static TargetEntry *process_matched_tle(
*** 72,79 ****
  static Node *get_assignment_input(Node *node);
  static void rewriteValuesRTE(RangeTblEntry *rte, Relation target_relation,
                   List *attrnos);
- static void rewriteTargetListUD(Query *parsetree, RangeTblEntry *target_rte,
-                     Relation target_relation);
  static void markQueryForLocking(Query *qry, Node *jtnode,
                      LockClauseStrength strength, LockWaitPolicy waitPolicy,
                      bool pushedDown);
--- 72,77 ----
*************** rewriteValuesRTE(RangeTblEntry *rte, Rel
*** 1293,1306 ****
   * This function adds a "junk" TLE that is needed to allow the executor to
   * find the original row for the update or delete.  When the target relation
   * is a regular table, the junk TLE emits the ctid attribute of the original
!  * row.  When the target relation is a view, there is no ctid, so we instead
!  * emit a whole-row Var that will contain the "old" values of the view row.
!  * If it's a foreign table, we let the FDW decide what to add.
   *
!  * For UPDATE queries, this is applied after rewriteTargetListIU.  The
!  * ordering isn't actually critical at the moment.
   */
! static void
  rewriteTargetListUD(Query *parsetree, RangeTblEntry *target_rte,
                      Relation target_relation)
  {
--- 1291,1305 ----
   * This function adds a "junk" TLE that is needed to allow the executor to
   * find the original row for the update or delete.  When the target relation
   * is a regular table, the junk TLE emits the ctid attribute of the original
!  * row.  When the target relation is a foreign table, we let the FDW decide
!  * what to add.
   *
!  * We used to do this during RewriteQuery(), but now that inheritance trees
!  * can contain a mix of regular and foreign tables, we must postpone it till
!  * planning, after the inheritance tree has been expanded.  In that way we
!  * can do the right thing for each child table.
   */
! void
  rewriteTargetListUD(Query *parsetree, RangeTblEntry *target_rte,
                      Relation target_relation)
  {
*************** rewriteTargetListUD(Query *parsetree, Ra
*** 1358,1376 ****
              attrname = "wholerow";
          }
      }
-     else
-     {
-         /*
-          * Emit whole-row Var so that executor will have the "old" view row to
-          * pass to the INSTEAD OF trigger.
-          */
-         var = makeWholeRowVar(target_rte,
-                               parsetree->resultRelation,
-                               0,
-                               false);
-
-         attrname = "wholerow";
-     }

      if (var != NULL)
      {
--- 1357,1362 ----
*************** ApplyRetrieveRule(Query *parsetree,
*** 1497,1502 ****
--- 1483,1490 ----
                   parsetree->commandType == CMD_DELETE)
          {
              RangeTblEntry *newrte;
+             Var           *var;
+             TargetEntry *tle;

              rte = rt_fetch(rt_index, parsetree->rtable);
              newrte = copyObject(rte);
*************** ApplyRetrieveRule(Query *parsetree,
*** 1527,1532 ****
--- 1515,1533 ----
              ChangeVarNodes((Node *) parsetree->returningList, rt_index,
                             parsetree->resultRelation, 0);

+             /*
+              * To allow the executor to find the original view row to pass to
+              * the INSTEAD OF trigger, we add a whole-row reference to the
+              * original RTE to the query's targetlist.
+              */
+             var = makeWholeRowVar(rte, rt_index, 0, false);
+             tle = makeTargetEntry((Expr *) var,
+                                   list_length(parsetree->targetList) + 1,
+                                   pstrdup("wholerow"),
+                                   true);
+
+             parsetree->targetList = lappend(parsetree->targetList, tle);
+
              /* Now, continue with expanding the original view RTE */
          }
          else
*************** rewriteTargetView(Query *parsetree, Rela
*** 2967,2992 ****
      view_rte->securityQuals = NIL;

      /*
-      * For UPDATE/DELETE, rewriteTargetListUD will have added a wholerow junk
-      * TLE for the view to the end of the targetlist, which we no longer need.
-      * Remove it to avoid unnecessary work when we process the targetlist.
-      * Note that when we recurse through rewriteQuery a new junk TLE will be
-      * added to allow the executor to find the proper row in the new target
-      * relation.  (So, if we failed to do this, we might have multiple junk
-      * TLEs with the same name, which would be disastrous.)
-      */
-     if (parsetree->commandType != CMD_INSERT)
-     {
-         TargetEntry *tle = (TargetEntry *) llast(parsetree->targetList);
-
-         Assert(tle->resjunk);
-         Assert(IsA(tle->expr, Var) &&
-                ((Var *) tle->expr)->varno == parsetree->resultRelation &&
-                ((Var *) tle->expr)->varattno == 0);
-         parsetree->targetList = list_delete_ptr(parsetree->targetList, tle);
-     }
-
-     /*
       * Now update all Vars in the outer query that reference the view to
       * reference the appropriate column of the base relation instead.
       */
--- 2968,2973 ----
*************** RewriteQuery(Query *parsetree, List *rew
*** 3347,3357 ****
                                      parsetree->override,
                                      rt_entry_relation,
                                      parsetree->resultRelation, NULL);
-             rewriteTargetListUD(parsetree, rt_entry, rt_entry_relation);
          }
          else if (event == CMD_DELETE)
          {
!             rewriteTargetListUD(parsetree, rt_entry, rt_entry_relation);
          }
          else
              elog(ERROR, "unrecognized commandType: %d", (int) event);
--- 3328,3337 ----
                                      parsetree->override,
                                      rt_entry_relation,
                                      parsetree->resultRelation, NULL);
          }
          else if (event == CMD_DELETE)
          {
!             /* Nothing to do here */
          }
          else
              elog(ERROR, "unrecognized commandType: %d", (int) event);
diff --git a/src/include/optimizer/prep.h b/src/include/optimizer/prep.h
index 80fbfd6..804c9e3 100644
*** a/src/include/optimizer/prep.h
--- b/src/include/optimizer/prep.h
*************** extern Expr *canonicalize_qual(Expr *qua
*** 38,47 ****
  /*
   * prototypes for preptlist.c
   */
! extern List *preprocess_targetlist(PlannerInfo *root, List *tlist);
!
! extern List *preprocess_onconflict_targetlist(List *tlist,
!                                  int result_relation, List *range_table);

  extern PlanRowMark *get_plan_rowmark(List *rowmarks, Index rtindex);

--- 38,44 ----
  /*
   * prototypes for preptlist.c
   */
! extern List *preprocess_targetlist(PlannerInfo *root);

  extern PlanRowMark *get_plan_rowmark(List *rowmarks, Index rtindex);

diff --git a/src/include/rewrite/rewriteHandler.h b/src/include/rewrite/rewriteHandler.h
index 494fa29..86ae571 100644
*** a/src/include/rewrite/rewriteHandler.h
--- b/src/include/rewrite/rewriteHandler.h
*************** extern void AcquireRewriteLocks(Query *p
*** 23,28 ****
--- 23,31 ----
                      bool forUpdatePushedDown);

  extern Node *build_column_default(Relation rel, int attrno);
+ extern void rewriteTargetListUD(Query *parsetree, RangeTblEntry *target_rte,
+                     Relation target_relation);
+
  extern Query *get_view_query(Relation view);
  extern const char *view_query_is_auto_updatable(Query *viewquery,
                               bool check_cols);