Marko Tiikkaja <marko@joh.to> writes:
> On 10/15/14, 10:02 AM, Atri Sharma wrote:
>> Please find attached a patch which implements support for UPDATE table1
>> SET(*)=...
> 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?
FWIW, I opined upthread that transformTargetList was not the place to
be handling this; it should be done in the same place(s) that support
the existing MultiAssignRef feature, and transformTargetList is not
that.
I think what's likely missing here is a clear design for the raw parse
tree representation (what's returned by the bison grammar). The patch
seems to be trying to skate by without creating any new parse node types
or fields, but that may well be a bad idea. At the very least there
needs to be some commentary added to parsenodes.h explaining what the
representation actually is (cf commentary there for MultiAssignRef).
Also, I think it's a mistake not to be following the MultiAssignRef
model for the case of "(*) = ctext_row". We optimize the ROW-source
case at the grammar stage when there's a fixed number of target columns,
because that's a very easy transformation --- but you can't do it like
that when there's not. It's possible that that optimization should be
delayed till later even in the existing case; in general, optimizing
in gram.y is a bad habit that's better avoided ...
regards, tom lane