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
