Re: GSOC13 proposal - extend RETURNING syntax

I took a look at this patch today, and I'm pretty skeptical about
whether it's on the right track. It adds a new kind of RTE called
RTE_ALIAS, which doesn't seem particularly descriptive and alias is
used elsewhere to mean something fairly different. More generally,
I'm not convinced that adding a new type of RTE is the right way to
handle this. The changes in pull_up_subqueries_recurse,
pullup_replace_vars_callback, preprocess_targetlist, and
build_joinrel_tlist seem like weird hacks. Those functions aren't
directly related to this feature; why do they need to know about it?

I wonder if we shouldn't be trying to handle resolution of these names
at an earlier processing stage, closer to the processor. I notice
that set_returning_clause_references() seems to have already solved
the hard part of this problem, which is frobbing target list entries
to return values from the new row rather than, as they naturally
would, the old row. In fact, we can already get approximately the
desired effect already:

rhaas=# update foo as after set a = before.a + 1 from foo as before
where before.a = after.a returning before.a, after.a;
a | a
---+---
1 | 2
(1 row)

Now this is a hack, because we don't really want to add an extra
scan/join just to get the behavior we want. But it seems to me
significant that this processing makes Vars that refer to the target
table refer to the new values, and if we got rid of it, they'd refer
to the old values. Can't we contrive to make AFTER.x parse into the
same Var node that x currently does? Then we don't need an RTE for
it. And maybe BEFORE.x ought to parse to the same node that just
plain x does but with some marking, or some other node wrapped around
it (like a TargetEntry with some flag set?) that suppresses this
processing. I'm just shooting from the hip here; that might be wrong
in detail, or even in broad strokes, but it just looks to me like the
additional RTE kind is going to bleed into a lot of places.

This patch also has significant style issues. Conforming to
PostgreSQL coding style is essential; if neither the author nor the
reviewer fixes problems in this area, then that is essentially making
it the committer's job, and the committer may not feel like taking
time to do that. Here's a selection of issues that I noticed while
reading this through: we use spaces around operators; the patch adds
two blank lines that shouldn't be there to the middle of the variable
declarations section; variables should be declared in the innermost
possible scope; single-statement blocks shouldn't have curly braces;
there shouldn't be whitespace before a closing parenthesis; there
should be a space after if and before the subsequent parenthesis;
braces should be uncuddled; code that does non-obvious things isn't
commented.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company