|From:||Marko Tiikkaja <marko(at)joh(dot)to>|
|To:||Atri Sharma <atri(dot)jiit(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: Support UPDATE table SET(*)=...|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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
> 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?
|Next Message||Demai Ni||2014-10-29 21:16:23||Re: foreign data wrapper option manipulation during Create foreign table time?|
|Previous Message||Stephen Frost||2014-10-29 21:08:32||Re: Directory/File Access Permissions for COPY and Generic File Access Functions|