Re: Support UPDATE table SET(*)=...

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