Re: BUG #15237: I got "ERROR: source for a multiple-column UPDATE item must be a sub-SELECT or ROW() expression"

[Pruning the CC list and moving this to -hackers]

>>>>> "Tom" == Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

Tom> It's telling you what to do: use a ROW() expression, ie
Tom> update fvt_obj_operate_update_table_033 set (c_int) = row(20)
Tom> where c_int = 20;

>> Yeah, but (a) this used to work, and has worked since at least as
>> far back as 9.0, and (b) the spec requires it to work.

Tom> As far as (a) goes, it was an intentional compatibility breakage,

So here I have a problem: the fact that it was a compatibility breakage
seems not to have been discussed *or even so much as mentioned* on
-hackers at any time.

The original patch, a self-described "lazy-sunday project", made
absolutely no mention of any compatibility issue, simply discussed the
new options it was allowing, and got no feedback or review. Had it
mentioned or even hinted that existing queries might be broken, I would
hope that there would have been more discussion at the time.

It then apparently went unnoticed until after the release of pg 10, at
which point it got retroactively documented (in the release notes and
nowhere else), in response to a brief discussion of a user complaint
that happened on -general and not -hackers (or even -bugs).

Tom> cf commits 86182b189 and 906bfcad7,

I encourage everyone to refer to these and decide whether this is
sufficient grounds or sufficient discussion to introduce a breaking
change, even a minor one.

Tom> and as for (b), the SQL committee is just nuts here.

We all know that the spec is a dog's breakfast, yes.

But breaking backwards compatibility AND the spec, even if it's in order
to support other parts of the spec, needs to be something that's
actually discussed and the tradeoff assessed rather than being snuck
through not only without discussion but also without even asking "is
there any sane way we can avoid breaking this".

Tom> The expectation in 906bfcad7 was that we'd move towards the
Tom> *other* thing the spec demands, namely that the RHS can be any
Tom> a_expr yielding a suitable row value.

The spec doesn't have anything that corresponds to our a_expr,
obviously.

Tom> We can't do that if it's unclear what is or isn't a ROW()
Tom> expression, because then the intended semantics would be
Tom> undecidable.

As far as I can tell, the spec distinguishes a lot of these cases only
by means of the type of the value.

For example:

<contextually typed row value expression> can be a <row value
special case> which is a <nonparenthesized value expression primary>
which must have a row type. (Note that for example (select row(1,2))
is a nonparenthesized value expression primary even though it has parens
around it.)

Or <contextually typed row value constructor> can be a <common
value expression> which can be one of several <$type value expression>
productions all of which contain <nonparenthesized value
expression primary> as one of the possible expansions. So for example
(select 1) ends up being treated as row((select 1)).

Tom> And I don't think we want a situation in which adding "extra"
Tom> parens changes a legal command into an illegal one.

To some extent I agree, but the spec does rely on this in places:

Tom> For example, suppose f(...) returns a single-column tuple result.
Tom> This should be legal, if x matches the type of the single column:

Tom> update ... set (x) = f(...)

Tom> but if we try to do what you seem to have in mind, this would not
Tom> be:

Tom> update ... set (x) = (f(...))

The spec indeed says that this is not valid, since it would wrap an
additional ROW() around the result, and would try and assign the row
value to x.

Tom> That's sufficiently brain-dead that I don't think we want to go
Tom> there.

Maybe so, but not without discussion.

--
Andrew (irc:RhodiumToad)