Re: [PATCH] Implement INSERT SET syntax

> On 15/11/2019, at 10:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> 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.)

So it appears as though it may not require any changes to ruleutils.c
as the parser is converting the multi-assignments into separate
columns, eg:

CREATE RULE r1 AS ON INSERT TO tab1
DO INSTEAD
INSERT INTO tab2 SET (col2, col1) = (new.col2, 0), col3 = tab3.col3
FROM tab3

The rule generated is:

r1 AS ON INSERT TO tab1 DO INSTEAD
INSERT INTO tab2 (col2, col1, col3)
SELECT new.col2, 0 AS col1, tab3.col3 FROM tab3

It will trigger that Assert() though, as EXPR_KIND_SELECT_TARGET is
now also being passed to transformMultiassignRef().

> * 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