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

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(*)=...
Date: 2014-10-29 21:12:05
Message-ID: 54515825.2030607@joh.to
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
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