Does rewriteTargetListIU still need to add UPDATE tlist entries?

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