Hi Atri,
Sorry for the delay. With pgconf.eu and all, it's been very hard to
find the time to look at this.
On 10/15/14, 10:02 AM, Atri Sharma wrote:
Please find attached a patch which implements support for UPDATE table1
SET(*)=...
The patch supports both UPDATE table SET(*)=(a,b,c) and UPDATE table1
SET(*)=(SELECT a,b,c FROM...).
It solves the problem of doing UPDATE from a record variable of the same
type as the table e.g. update foo set (*) = (select foorec.*) where ...;
Excellent! This is a very welcome change.
I've had a few looks at this patch and I have a few comments:
1) This doesn't work for the zero-column table case at all:
CREATE TABLE foo();
UPDATE foo SET (*) = (SELECT);
ERROR: number of columns does not match number of values
2) What's the purpose of the second condition here?
if (!(origTarget) || !(origTarget->name))
3) The extra parentheses around everything make this code for some
reason very hard to read.
4) transformTargetList() is a mess right now. If this is the
approach we want to take, the common code should probably be refactored
into a function. But the usage of List as a somehow magical way to
represent the SET (*) case makes me feel weird inside.
5) The complete lack of regression tests make it hard to poke around
the code to try and figure out what each line/condition is trying to do.
I feel like I understand what this code is doing and some details feel a
bit icky, but I'm not the right person to comment on whether the broad
strokes are on the right canvas or not. Maybe someone else wants to
take a closer look before Atri spends too much time on this approach?
After all, this patch has a very distinctive WIP feel to it, so I guess
feedback on the general approach is what's being sought after here, and
in that area I consider my skills and knowledge lacking.
The design is simple. It basically expands the * in transformation stage,
does the necessary type checking and adds it to the parse tree. This allows
for normal execution for the rest of the stages.
I can't poke any big holes into this approach (disregarding the details
of this implementation), but perhaps someone else can?
.marko
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
