Re: [PATCH] Implement INSERT SET syntax

Gareth Palmer <gareth(at)internetnz(dot)net(dot)nz> writes:
>> On 19/08/2019, at 3:00 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Perhaps the way to resolve Peter's objection is to make the syntax
>> more fully like UPDATE:
>> INSERT INTO target SET c1 = x, c2 = y+z, ... FROM tables-providing-x-y-z
>> (with the patch as-submitted corresponding to the case with an empty
>> FROM clause, hence no variables in the expressions-to-be-assigned).

> Thanks for the feedback. Attached is version 3 of the patch that makes
> the syntax work more like an UPDATE statement when a FROM clause is used.

Since nobody has objected to this, I'm supposing that there's general
consensus that that design sketch is OK, and we can move on to critiquing
implementation details. I took a look, and didn't like much of what I saw.

* In the grammar, there's no real need to have separate productions
for the cases with FROM and without. The way you have it is awkward,
and it arbitrarily rejects combinations that work fine in plain
SELECT, such as WHERE without FROM. You should just do

insert_set_clause:
SET set_clause_list from_clause where_clause
group_clause having_clause window_clause opt_sort_clause
opt_select_limit

relying on the ability of all those symbols (except set_clause_list) to
reduce to empty.

* This is randomly inconsistent with select_no_parens, and not in a
good way, because you've omitted the option that's actually most likely
to be useful, namely for_locking_clause. I wonder whether it's practical
to refactor select_no_parens so that the stuff involving optional trailing
clauses can be separated out into a production that insert_set_clause
could also use. Might not be worth the trouble, but I'm concerned
about select_no_parens growing additional clauses that we then forget
to also add to insert_set_clause.

* I'm not sure if it's worth also refactoring simple_select so that
the "into_clause ... window_clause" business could be shared. But
it'd likely be a good idea to at least have a comment there noting
that any changes in that production might need to be applied to
insert_set_clause as well.

* In kind of the same vein, it feels like the syntax documentation
is awkwardly failing to share commonality that it ought to be
able to share with the SELECT man page.

* I dislike the random hacking you did in transformMultiAssignRef.
That weakens a useful check for error cases, and it's far from clear
why the new assertion is OK. It also raises the question of whether
this is really the only place you need to touch in parse analysis.
Perhaps it'd be better to consider inventing new EXPR_KIND_ values
for this situation; you'd then have to run around and look at all the
existing EXPR_KIND uses, but that seems like a useful cross-check
activity anyway. Or maybe we need to take two steps back and
understand why that change is needed at all. I'd imagined that this
patch would be only syntactic sugar for something you can do already,
so it's not quite clear to me why we need additional changes.

(If it's *not* just syntactic sugar, then the scope of potential
problems becomes far greater, eg does ruleutils.c need to know
how to reconstruct a valid SQL command from a querytree like this.
If we're not touching ruleutils.c, we need to be sure that every
command that can be written this way can be written old-style.)

* Other documentation gripes: the lone example seems insufficient,
and there needs to be an entry under COMPATIBILITY pointing out
that this is not per SQL spec.

* Some of the test cases seem to be expensively repeating
construction/destruction of tables that they could have shared with
existing test cases. I do not consider it a virtue for new tests
added to an existing test script to be resolutely independent of
what's already in that script.

I'm setting this back to Waiting on Author.

regards, tom lane