Does rewriteTargetListIU still need to add UPDATE tlist entries? - Mailing list pgsql-hackers

The comments for rewriteTargetListIU say (or said until earlier today)

 * 2. For an UPDATE on a trigger-updatable view, add tlist entries for any
 * unassigned-to attributes, assigning them their old values.  These will
 * later get expanded to the output values of the view.  (This is equivalent
 * to what the planner's expand_targetlist() will do for UPDATE on a regular
 * table, but it's more convenient to do it here while we still have easy
 * access to the view's original RT index.)  This is only necessary for
 * trigger-updatable views, for which the view remains the result relation of
 * the query.  For auto-updatable views we must not do this, since it might
 * add assignments to non-updatable view columns.  For rule-updatable views it
 * is unnecessary extra work, since the query will be rewritten with a
 * different result relation which will be processed when we recurse via
 * RewriteQuery.

I noticed that this is referencing something that, in fact,
expand_targetlist() doesn't do anymore, so that this is a poor
justification for the behavior.  My first thought was that we still
need to do it to produce the correct row contents for the INSTEAD OF
trigger, so I updated the comment (in 08a986966) to claim that.

However, on closer inspection, that's nonsense.  nodeModifyTable.c
populates the trigger "OLD" row from the whole-row variable that is
generated for the view, and then it computes the "NEW" row using
that old row and the UPDATE tlist; there is no need there for the
UPDATE tlist to compute all the columns.  The regression tests still
pass just fine if we take out the questionable logic (cf. attached
patch).  Moreover, if you poke into it a little closer than the tests
do, you notice that the existing code is uselessly computing non-updated
columns twice, in both the extra tlist item and the whole-row variable.
As an example, consider

create table base (a int, b int);
create view v1 as select a+1 as a1, b+2 as b2 from base;
-- you also need an INSTEAD OF UPDATE trigger, not shown here
explain verbose update v1 set a1 = a1 - 44;

With HEAD you get

 Update on public.v1  (cost=0.00..60.85 rows=0 width=0)
   ->  Seq Scan on public.base  (cost=0.00..60.85 rows=2260 width=46)
         Output: ((base.a + 1) - 44), (base.b + 2), ROW((base.a + 1), (base.b + 2)), base.ctid

There's really no need to compute base.b + 2 twice, and with this
patch we don't:

 Update on public.v1  (cost=0.00..55.20 rows=0 width=0)
   ->  Seq Scan on public.base  (cost=0.00..55.20 rows=2260 width=42)
         Output: ((base.a + 1) - 44), ROW((base.a + 1), (base.b + 2)), base.ctid


I would think that this is a totally straightforward improvement,
but there's one thing in the comments for rewriteTargetListIU that
gives me a little pause: it says

 * We must do items 1,2,3 before firing rewrite rules, else rewritten
 * references to NEW.foo will produce wrong or incomplete results.

As far as I can tell, though, references to NEW values still do the
right thing.  I'm not certain whether any of the existing regression
tests really cover this point, but experimenting with the scenario shown
in the attached SQL file says that the DO ALSO rule gets the right
results.  It's possible that the expansion sequence is a bit different
than before, but we still end up with the right answer.

So, as far as I can tell, this is an oversight in 86dc90056 and we
ought to clean it up as attached.

            regards, tom lane

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 4fa4ac2aef..4bba6eb805 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -679,18 +679,7 @@ adjustJoinTreeList(Query *parsetree, bool removert, int rt_index)
  * and UPDATE, replace explicit DEFAULT specifications with column default
  * expressions.
  *
- * 2. For an UPDATE on a trigger-updatable view, add tlist entries for any
- * unassigned-to attributes, assigning them their old values.  These will
- * later get expanded to the output values of the view.  This is only needed
- * for trigger-updatable views, for which the view remains the result relation
- * of the query; without it, we would not have a complete "new tuple" to pass
- * to triggers.  For auto-updatable views we must not do this, since it might
- * add assignments to non-updatable view columns.  For rule-updatable views it
- * is unnecessary extra work, since the query will be rewritten with a
- * different result relation which will be processed when we recurse via
- * RewriteQuery.
- *
- * 3. Merge multiple entries for the same target attribute, or declare error
+ * 2. Merge multiple entries for the same target attribute, or declare error
  * if we can't.  Multiple entries are only allowed for INSERT/UPDATE of
  * portions of an array or record field, for example
  *            UPDATE table SET foo[2] = 42, foo[4] = 43;
@@ -698,11 +687,11 @@ adjustJoinTreeList(Query *parsetree, bool removert, int rt_index)
  * the expression we want to produce in this case is like
  *        foo = array_set_element(array_set_element(foo, 2, 42), 4, 43)
  *
- * 4. Sort the tlist into standard order: non-junk fields in order by resno,
+ * 3. Sort the tlist into standard order: non-junk fields in order by resno,
  * then junk fields (these in no particular order).
  *
- * We must do items 1,2,3 before firing rewrite rules, else rewritten
- * references to NEW.foo will produce wrong or incomplete results.  Item 4
+ * We must do items 1 & 2 before firing rewrite rules, else rewritten
+ * references to NEW.foo will produce wrong or incomplete results.  Item 3
  * is not needed for rewriting, but it is helpful for the planner, and we
  * can do it essentially for free while handling the other items.
  *
@@ -984,29 +973,6 @@ rewriteTargetListIU(List *targetList,
                                           false);
         }

-        /*
-         * For an UPDATE on a trigger-updatable view, provide a dummy entry
-         * whenever there is no explicit assignment.
-         */
-        if (new_tle == NULL && commandType == CMD_UPDATE &&
-            target_relation->rd_rel->relkind == RELKIND_VIEW &&
-            view_has_instead_trigger(target_relation, CMD_UPDATE))
-        {
-            Node       *new_expr;
-
-            new_expr = (Node *) makeVar(result_rti,
-                                        attrno,
-                                        att_tup->atttypid,
-                                        att_tup->atttypmod,
-                                        att_tup->attcollation,
-                                        0);
-
-            new_tle = makeTargetEntry((Expr *) new_expr,
-                                      attrno,
-                                      pstrdup(NameStr(att_tup->attname)),
-                                      false);
-        }
-
         if (new_tle)
             new_tlist = lappend(new_tlist, new_tle);
     }
drop table base cascade;
drop table log cascade;
create table base (a int, b int);
create view v1 as select a+1 as a1, b+2 as b2 from base;
insert into base values (1,10),(11,13);

create table log(ao int, an int, bo int, bn int);

CREATE OR REPLACE FUNCTION vtrigger()
 RETURNS trigger
 LANGUAGE plpgsql
AS $function$
begin
    raise notice '% % % %', TG_TABLE_NAME, TG_WHEN, TG_OP, TG_LEVEL;
    raise NOTICE 'OLD: %, NEW: %', OLD, NEW;
    UPDATE base SET a = NEW.a1-1, b = NEW.b2-2
      WHERE a = OLD.a1-1 AND b = OLD.b2-2;
    if NOT FOUND then RETURN NULL; end if;
    RETURN NEW;
end;
$function$;

CREATE TRIGGER instead_of_update_trig INSTEAD OF UPDATE ON v1
FOR EACH ROW EXECUTE PROCEDURE vtrigger();

create rule logit as on update to v1 do also
insert into log values(old.a1, new.a1, old.b2, new.b2);

update v1 set a1 = a1-44;