Обсуждение: [PATCH] Implement INSERT SET syntax

Hello,

Attached is a patch that adds the option of using SET clause to specify
the columns and values in an INSERT statement in the same manner as that
of an UPDATE statement.

A simple example that uses SET instead of a VALUES() clause:

INSERT INTO t SET c1 = 'foo', c2 = 'bar', c3 = 'baz';

Values may also be sourced from a CTE using a FROM clause:

WITH x AS (
  SELECT 'foo' AS c1, 'bar' AS c2, 'baz' AS c3
)
INSERT INTO t SET c1 = x.c1, c2 = x.c2, c3 = x.c3 FROM x;

The advantage of using the SET clause style is that the column and value
are kept together, which can make changing or removing a column or value from
a large list easier.

Internally the grammar parser converts INSERT SET without a FROM clause into
the equivalent INSERT with a VALUES clause. When using a FROM clause it becomes
the equivalent of INSERT with a SELECT statement.

There was a brief discussion regarding INSERT SET on pgsql-hackers in late
August 2009 [1].

INSERT SET is not part of any SQL standard (that I am aware of), however this
syntax is also implemented by MySQL [2]. Their implementation does not support
specifying a FROM clause.

Patch also contains regression tests and documentation.


Regards,
Gareth


[1] https://www.postgresql.org/message-id/flat/2c5ef4e30908251010s46d9d566m1da21357891bab3d%40mail.gmail.com
[2] https://dev.mysql.com/doc/refman/8.0/en/insert.html

Hi Marko,

> On 17/07/2019, at 5:52 PM, Marko Tiikkaja <marko@joh.to> wrote:
>
> On Wed, Jul 17, 2019 at 7:30 AM Gareth Palmer <gareth@internetnz.net.nz> wrote:
> Attached is a patch that adds the option of using SET clause to specify
> the columns and values in an INSERT statement in the same manner as that
> of an UPDATE statement.
>
> Cool!  Thanks for working on this, I'd love to see the syntax in PG.
>
> There was a brief discussion regarding INSERT SET on pgsql-hackers in late
> August 2009 [1].
>
> There was also at least one slightly more recent adventure:
https://www.postgresql.org/message-id/709e06c0-59c9-ccec-d216-21e38cb5ed61%40joh.to
>
> You might want to check that thread too, in case any of the criticism there applies to this patch as well.

Thank-you for the pointer to that thread.

I think my version avoids issue raised there by doing the conversion of the SET clause as part of the INSERT grammar
rules.

Gareth

Hello.

At Thu, 18 Jul 2019 11:30:04 +1200, Gareth Palmer <gareth@internetnz.net.nz> wrote in
<D50A93EB-11F3-4ED2-8192-0328DF901BBA@internetnz.net.nz>
> Hi Marko,
> 
> > On 17/07/2019, at 5:52 PM, Marko Tiikkaja <marko@joh.to> wrote:
> > 
> > On Wed, Jul 17, 2019 at 7:30 AM Gareth Palmer <gareth@internetnz.net.nz> wrote:
> > Attached is a patch that adds the option of using SET clause to specify
> > the columns and values in an INSERT statement in the same manner as that
> > of an UPDATE statement.
> > 
> > Cool!  Thanks for working on this, I'd love to see the syntax in PG.
> > 
> > There was a brief discussion regarding INSERT SET on pgsql-hackers in late
> > August 2009 [1].
> > 
> > There was also at least one slightly more recent adventure:
https://www.postgresql.org/message-id/709e06c0-59c9-ccec-d216-21e38cb5ed61%40joh.to
> > 
> > You might want to check that thread too, in case any of the criticism there applies to this patch as well.
> 
> Thank-you for the pointer to that thread.
> 
> I think my version avoids issue raised there by doing the conversion of the SET clause as part of the INSERT grammar
rules.

If I'm not missing something, "SELECT <targetlist>" without
having FROM clause doesn't need to be tweaked. Thus
insert_set_clause is useless and all we need here would be
something like the following. (and the same for OVERRIDING.)

+       | SET set_clause_list from_clause
+         {
+           SelectStmt *n = makeNode(SelectStmt);
+           n->targetList = $2;
+           n->fromClause = $3;
+           $$ = makeNode(InsertStmt);
+           $$->selectStmt = (Node *)n;
+           $$->cols = $2;
+         }

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hi Kyotaro,

Thank-you for looking at the patch.

> On 18/07/2019, at 6:54 PM, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
>
> Hello.
>
> If I'm not missing something, "SELECT <targetlist>" without
> having FROM clause doesn't need to be tweaked. Thus
> insert_set_clause is useless and all we need here would be
> something like the following. (and the same for OVERRIDING.)
>
> +       | SET set_clause_list from_clause
> +         {
> +           SelectStmt *n = makeNode(SelectStmt);
> +           n->targetList = $2;
> +           n->fromClause = $3;
> +           $$ = makeNode(InsertStmt);
> +           $$->selectStmt = (Node *)n;
> +           $$->cols = $2;
> +         }

While that would mostly work, it would prevent setting the column to its
default value using the DEFAULT keyword.

Only expressions specified in valuesLists allow DEFAULT to be used. Those
in targetList do not because transformInsertStmt() treats that as a general
SELECT statement and the grammar does not allow the use of DEFAULT there.

So this would generate a "DEFAULT is not allowed in this context" error
if only targetList was used:

INSERT INTO t set c1 = DEFAULT;


Regards,
Gareth

> regards.
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center

Patch conflict with this assertion 
Assert(pstate->p_expr_kind == EXPR_KIND_UPDATE_SOURCE); 

src/backend/parser/parse_expr.c line 1570

The new status of this patch is: Waiting on Author

Hi Ibrar,

> On 16/08/2019, at 7:14 AM, Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote:
> 
> Patch conflict with this assertion 
> Assert(pstate->p_expr_kind == EXPR_KIND_UPDATE_SOURCE); 
> 
> src/backend/parser/parse_expr.c line 1570
> 
> The new status of this patch is: Waiting on Author

Thank-you for reviewing the patch.

Attached is version 2 of the patch that fixes the above by allowing
p_expr_kind to be EXPR_KIND_VALUES_SINGLE as well.


Gareth

On Wed, Jul 17, 2019 at 10:00 AM Gareth Palmer <gareth@internetnz.net.nz> wrote:
>
> Hello,
>
> Attached is a patch that adds the option of using SET clause to specify
> the columns and values in an INSERT statement in the same manner as that
> of an UPDATE statement.
>
> A simple example that uses SET instead of a VALUES() clause:
>
> INSERT INTO t SET c1 = 'foo', c2 = 'bar', c3 = 'baz';
>
> Values may also be sourced from a CTE using a FROM clause:
>
> WITH x AS (
>   SELECT 'foo' AS c1, 'bar' AS c2, 'baz' AS c3
> )
> INSERT INTO t SET c1 = x.c1, c2 = x.c2, c3 = x.c3 FROM x;
>
> The advantage of using the SET clause style is that the column and value
> are kept together, which can make changing or removing a column or value from
> a large list easier.
>
> Internally the grammar parser converts INSERT SET without a FROM clause into
> the equivalent INSERT with a VALUES clause. When using a FROM clause it becomes
> the equivalent of INSERT with a SELECT statement.
>
> There was a brief discussion regarding INSERT SET on pgsql-hackers in late
> August 2009 [1].
>
> INSERT SET is not part of any SQL standard (that I am aware of), however this
> syntax is also implemented by MySQL [2]. Their implementation does not support
> specifying a FROM clause.
>

I think this can be a handy feature in some cases as pointed by you,
but do we really want it for PostgreSQL?  In the last round of
discussions as pointed by you, there doesn't seem to be a consensus
that we want this feature.  I guess before spending too much time into
reviewing this feature, we should first build a consensus on whether
we need this.

Along with users, I request some senior hackers/committers to also
weigh in about the desirability of this feature.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 2019-08-16 05:19, Amit Kapila wrote:
> I think this can be a handy feature in some cases as pointed by you,
> but do we really want it for PostgreSQL?  In the last round of
> discussions as pointed by you, there doesn't seem to be a consensus
> that we want this feature.  I guess before spending too much time into
> reviewing this feature, we should first build a consensus on whether
> we need this.

I think the problem this is attempting to solve is valid.

What I don't like about the syntax is that it kind of breaks the
notional processing model of INSERT in a fundamental way.  The model is

INSERT INTO $target $table_source

where $table_source could be VALUES, SELECT, possibly others in theory.

The proposed syntax changes this to only allow a single row to be
specified via the SET syntax, and the SET syntax does not function as a
row or table source in other contexts.

Let's think about how we can achieve this using existing concepts in
SQL.  What we really need here at a fundamental level is an option to
match $target to $table_source by column *name* rather than column
*position*.  There is existing syntax in SQL for that, namely

    a UNION b

vs

    a UNION CORRESPONDING b

I think this could be used for INSERT as well.

And then you need a syntax to assign column names inside the VALUES
rows.  I think you could do either of the following:

    VALUES (a => 1, b => 2)

or

    VALUES (1 AS a, 2 AS b)

Another nice effect of this would be that you could so something like

    INSERT INTO tbl2 CORRESPONDING SELECT * FROM tbl1;

which copies the contents of tbl1 to tbl2 if they have the same column
names but allowing for a different column order.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 18/08/2019 11:03, Peter Eisentraut wrote:
>
>     a UNION b
>
> vs
>
>     a UNION CORRESPONDING b


I have a WIP patch for CORRESPONDING [BY].  Is there any interest in me
continuing it?  If so, I'll start another thread for it.

-- 

Vik Fearing

Vik Fearing <vik.fearing@2ndquadrant.com> writes:
> On 18/08/2019 11:03, Peter Eisentraut wrote:
>> a UNION b
>> vs
>> a UNION CORRESPONDING b

> I have a WIP patch for CORRESPONDING [BY].  Is there any interest in me
> continuing it?  If so, I'll start another thread for it.

CORRESPONDING is in the SQL standard, so in theory we ought to provide
it.  I think the hard question is how big/complicated the patch would be
--- if the answer is "complicated", maybe it's not worth it.  People
have submitted patches for it before that didn't go anywhere, suggesting
that the tradeoffs are not very good ... but maybe you'll think of a
better way.

            regards, tom lane

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> What I don't like about the syntax is that it kind of breaks the
> notional processing model of INSERT in a fundamental way.

Agreed.  I really don't like that this only works for a VALUES-like case
(and only the one-row form at that).  It's hard to see it as anything
but a wart pasted onto the syntax.

> Let's think about how we can achieve this using existing concepts in
> SQL.  What we really need here at a fundamental level is an option to
> match $target to $table_source by column *name* rather than column
> *position*.  There is existing syntax in SQL for that, namely
>     a UNION b
> vs
>     a UNION CORRESPONDING b

A potential issue here --- and something that applies to Vik's question
as well, now that I think about it --- is that CORRESPONDING breaks down
in the face of ALTER TABLE RENAME COLUMN.  Something that had been a
legal query before the rename might be invalid, or mean something quite
different, afterwards.  This is really nasty for stored views/rules,
because we have neither a mechanism for forbidding input-table renames
nor a mechanism for revalidating views/rules afterwards.  Maybe we could
make it go by resolving CORRESPONDING in the rewriter or planner, rather
than in parse analysis; but that seems quite unpleasant as well.
Changing our conclusions about the data types coming out of a UNION
really shouldn't happen later than parse analysis.

The SET-style syntax doesn't have that problem, since it's explicit
about which values go into which columns.

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).

Of course, this is not functionally distinct from

INSERT INTO target(c1,c2,...) SELECT x, y+z, ... FROM tables-providing-x-y-z

and it's fair to question whether it's worth supporting a nonstandard
syntax just to allow the target column names to be written closer to
the expressions-to-be-assigned.

            regards, tom lane

Hi Tom,

> On 19/08/2019, at 3:00 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> What I don't like about the syntax is that it kind of breaks the
>> notional processing model of INSERT in a fundamental way.
>
> Agreed.  I really don't like that this only works for a VALUES-like case
> (and only the one-row form at that).  It's hard to see it as anything
> but a wart pasted onto the syntax.
>
>> Let's think about how we can achieve this using existing concepts in
>> SQL.  What we really need here at a fundamental level is an option to
>> match $target to $table_source by column *name* rather than column
>> *position*.  There is existing syntax in SQL for that, namely
>>    a UNION b
>> vs
>>    a UNION CORRESPONDING b
>
> A potential issue here --- and something that applies to Vik's question
> as well, now that I think about it --- is that CORRESPONDING breaks down
> in the face of ALTER TABLE RENAME COLUMN.  Something that had been a
> legal query before the rename might be invalid, or mean something quite
> different, afterwards.  This is really nasty for stored views/rules,
> because we have neither a mechanism for forbidding input-table renames
> nor a mechanism for revalidating views/rules afterwards.  Maybe we could
> make it go by resolving CORRESPONDING in the rewriter or planner, rather
> than in parse analysis; but that seems quite unpleasant as well.
> Changing our conclusions about the data types coming out of a UNION
> really shouldn't happen later than parse analysis.
>
> The SET-style syntax doesn't have that problem, since it's explicit
> about which values go into which columns.
>
> 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).
>
> Of course, this is not functionally distinct from
>
> INSERT INTO target(c1,c2,...) SELECT x, y+z, ... FROM tables-providing-x-y-z
>
> and it's fair to question whether it's worth supporting a nonstandard
> syntax just to allow the target column names to be written closer to
> 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.

So, an updated summary of the new syntax is:

1. Equivalent to VALUES(...):

  INSERT INTO t SET c1 = x, c2 = y, c3 = z;

2. Equivalent to INSERT INTO ... SELECT ...:

  INSERT INTO t SET c1 = sum(x.c1) FROM x WHERE x.c1 < y AND x.c2 != z
    GROUP BY x.c3 ORDER BY x.c4 ASC LIMIT a OFFSET b;


Gareth

>             regards, tom lane

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            not tested

Patch looks to me and works on my machine 73025140885c889410b9bfc4a30a3866396fc5db  - HEAD I have not reviewed the
documentaionchanges. 

The new status of this patch is: Ready for Committer

On Sun, Aug 18, 2019 at 11:00 AM Tom Lane <tgl@sss.pgh.pa.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).
>
> Of course, this is not functionally distinct from
>
> INSERT INTO target(c1,c2,...) SELECT x, y+z, ... FROM tables-providing-x-y-z
>
> and it's fair to question whether it's worth supporting a nonstandard
> syntax just to allow the target column names to be written closer to
> the expressions-to-be-assigned.

For what it's worth, I think this would be useful enough to justify
its existence. Back in days of yore when dragons roamed the earth and
I wrote database-driven applications instead of hacking on the
database itself, I often wondered why I had to write two
completely-different looking SQL statements, one to insert the data
which a user had entered into a webform into the database, and another
to update previously-entered data. This feature would allow those
queries to be written in the same way, which would have pleased me,
back in the day.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Gareth Palmer <gareth@internetnz.net.nz> writes:
>> On 19/08/2019, at 3:00 AM, Tom Lane <tgl@sss.pgh.pa.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

Pantelis Theodosiou <ypercube@gmail.com> writes:
> On 19/08/2019, at 3:00 AM, Tom Lane <tgl@sss.pgh.pa.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

> Regarding syntax and considering that it makes INSERT look like UPDATE:
> there is another difference between INSERT and UPDATE. INSERT allows SELECT
> with ORDER BY and OFFSET/LIMIT (or FETCH FIRST), e.g.: ...
> But UPDATE doesn't. I suppose the proposed behaviour of INSERT .. SET will
> be the same as standard INSERT. So we'll need a note for the differences
> between INSERT/SET and UPDATE/SET syntax.

I was supposing that this syntax should be just another way to spell

INSERT INTO target (columnlist) SELECT ...

So everything past FROM would work exactly like it does in SELECT.

> On a related not, column aliases can be used in ORDER BY, e.g:

As proposed, there's no option equivalent to writing output-column aliases
in the INSERT ... SELECT form, so the question doesn't come up.

            regards, tom lane

> On 15/11/2019, at 10:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Gareth Palmer <gareth@internetnz.net.nz> writes:
>>> On 19/08/2019, at 3:00 AM, Tom Lane <tgl@sss.pgh.pa.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.

There are two separate productions to match the two different types
of inserts: INSERT with VALUES and INSERT with SELECT.

The former has to store the the values in valuesLists so that DEFAULT
can still be used.

Allowing a WHERE without a FROM also mean that while this would
work:

INSERT INTO t SET c = DEFAULT;

But this would fail with 'DEFAULT is not allowed in this context':

INSERT INTO t SET c = DEFAULT WHERE true;

I should have put a comment explaining why there are two rules.

It could be combined into one production but there would have to be
a check that $4 .. $9 are NULL to determine what type of INSERT to
use.

transformInsertStmt() also has an optimisation for the case of a
single valueLists entry.

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

I can add opt_for_locking_clause and a comment to simple_select to
start with while the format of insert_set_clause is still being
worked out.

> * 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 could collapse the from clause to just '[ FROM from_clause ]'
and have it refer to the from clause and everything after it in
SELECT.

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

It was intended to just be syntatic sugar, however because
set_clause_list is being re-used the ability to do multi-assignment
in an INSERT's targetList 'came along for the ride' which has
no equivalent in the current INSERT syntax.

That would be why those EXPR_KIND's are now appearing in
transformMultiAssignRef().

There are 3 things that could be done here:

1. Update ruletutils.c to emit INSERT SET in get_insert_query_def()
   if query->hasSubLinks is true.

2. Add a new production similar to set_clause_list which doesn't
   allow multi-assignment.

3. Re-use set_clause_list but reject targetLists that contain
   multi-assignment.

Keeping that feature is probably desirable at least for consistency
with other SET clauses.

I will work on getting get_insert_query_def() to correctly
reconstruct the new syntax.

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

I will add something to in the compatibility section.

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

Those test cases will be changed to share the those tables.

> I'm setting this back to Waiting on Author.
>
>             regards, tom lane

> On 15/11/2019, at 10:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Gareth Palmer <gareth@internetnz.net.nz> writes:
>>> On 19/08/2019, at 3:00 AM, Tom Lane <tgl@sss.pgh.pa.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

> On 19/11/2019, at 5:05 PM, Gareth Palmer <gareth@internetnz.net.nz> wrote:
>>
>> 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.

Attached is an updated patch with for_locking_clause added, test-cases
re-use existing tables and the comments and documentation have been
expanded.

>> I'm setting this back to Waiting on Author.

On Fri, Nov 22, 2019 at 12:24:15PM +1300, Gareth Palmer wrote:
> Attached is an updated patch with for_locking_clause added, test-cases
> re-use existing tables and the comments and documentation have been
> expanded.

Per the automatic patch tester, documentation included in the patch
does not build.  Could you please fix that?  I have moved the patch to
next CF, waiting on author.
--
Michael

On Sun, Dec 1, 2019 at 4:32 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Nov 22, 2019 at 12:24:15PM +1300, Gareth Palmer wrote:
> > Attached is an updated patch with for_locking_clause added, test-cases
> > re-use existing tables and the comments and documentation have been
> > expanded.
>
> Per the automatic patch tester, documentation included in the patch
> does not build.  Could you please fix that?  I have moved the patch to
> next CF, waiting on author.

Attached is a fixed version.

> --
> Michael

Hi Tom,

On 12/3/19 4:44 AM, Gareth Palmer wrote:
> On Sun, Dec 1, 2019 at 4:32 PM Michael Paquier <michael@paquier.xyz> wrote:
>>
>> On Fri, Nov 22, 2019 at 12:24:15PM +1300, Gareth Palmer wrote:
>>> Attached is an updated patch with for_locking_clause added, test-cases
>>> re-use existing tables and the comments and documentation have been
>>> expanded.
>>
>> Per the automatic patch tester, documentation included in the patch
>> does not build.  Could you please fix that?  I have moved the patch to
>> next CF, waiting on author.
> 
> Attached is a fixed version.

Does this version of the patch address your concerns?

Regards,
-- 
-David
david@pgmasters.net

David Steele <david@pgmasters.net> writes:
> On 12/3/19 4:44 AM, Gareth Palmer wrote:
>> Attached is a fixed version.

> Does this version of the patch address your concerns?

No.  I still find the reliance on a FROM clause being present
to be pretty arbitrary.  Also, I don't believe that ruleutils.c
requires no changes, because it's not going to be possible to
transform every usage of this syntax to old-style.  I tried to
prove the point with this trivial example:

regression=# create table foo (f1 int ,f2 int, f3 int);
CREATE TABLE
regression=# create table bar (f1 int ,f2 int, f3 int);
CREATE TABLE
regression=# create rule r1 as on insert to foo do instead
regression-# insert into bar set (f1,f2,f3) = (select f1,f2,f3 from foo);

intending to show that the rule decompilation was bogus, but
I didn't get that far because the parser crashed:

TRAP: FailedAssertion("pstate->p_multiassign_exprs == NIL", File: "parse_target.c", Line: 287)
postgres: postgres regression [local] CREATE RULE(ExceptionalCondition+0x55)[0x8fb6e5]
postgres: postgres regression [local] CREATE RULE[0x5bd0c3]
postgres: postgres regression [local] CREATE RULE[0x583def]
postgres: postgres regression [local] CREATE RULE(transformStmt+0x2d5)[0x582665]
postgres: postgres regression [local] CREATE RULE(transformRuleStmt+0x2ad)[0x5bf2ad]
postgres: postgres regression [local] CREATE RULE(DefineRule+0x17)[0x793847]

If I do it like this, I get a different assertion:

regression=# insert into bar set (f1,f2,f3) = (select f1,f2,f3) from foo;
server closed the connection unexpectedly

TRAP: FailedAssertion("exprKind == EXPR_KIND_UPDATE_SOURCE", File: "parse_target.c", Line: 209)
postgres: postgres regression [local] INSERT(ExceptionalCondition+0x55)[0x8fb6e5]
postgres: postgres regression [local] INSERT(transformTargetList+0x1a7)[0x5bd277]
postgres: postgres regression [local] INSERT(transformStmt+0xbe0)[0x582f70]
postgres: postgres regression [local] INSERT[0x5839f3]
postgres: postgres regression [local] INSERT(transformStmt+0x2d5)[0x582665]
postgres: postgres regression [local] INSERT(transformTopLevelStmt+0xd)[0x58411d]
postgres: postgres regression [local] INSERT(parse_analyze+0x69)[0x584269]


No doubt that's all fixable, but the realization that some cases of
this syntax are *not* just syntactic sugar for standards-compliant
syntax is giving me pause.  Do we really want to get out front of
the SQL committee on extending INSERT in an incompatible way?

            regards, tom lane

I wrote:
> No doubt that's all fixable, but the realization that some cases of
> this syntax are *not* just syntactic sugar for standards-compliant
> syntax is giving me pause.  Do we really want to get out front of
> the SQL committee on extending INSERT in an incompatible way?

One compromise that might be worth thinking about is to disallow
multiassignments in this syntax, so as to (1) avoid the possibility
of generating something that can't be represented by standard INSERT
and (2) get something done in time for v13.  The end of March is not
that far off.  Perhaps somebody would come back and extend it later,
or perhaps not.

A slightly more ambitious compromise would be to allow multiassignment
only when the source can be pulled apart into independent subexpressions,
comparable to the restriction we used to have in UPDATE itself (before
8f889b108 or thereabouts).

In either case the transformation could be done right in gram.y and
a helpful error thrown for unsupported cases.

            regards, tom lane

On 2020-03-24 18:57, Tom Lane wrote:
> No doubt that's all fixable, but the realization that some cases of
> this syntax are*not*  just syntactic sugar for standards-compliant
> syntax is giving me pause.  Do we really want to get out front of
> the SQL committee on extending INSERT in an incompatible way?

What is the additional functionality that we are considering adding here?

The thread started out proposing a more convenient syntax, but it seems 
to go deeper now and perhaps not everyone is following.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2020-03-24 18:57, Tom Lane wrote:
>> No doubt that's all fixable, but the realization that some cases of
>> this syntax are*not*  just syntactic sugar for standards-compliant
>> syntax is giving me pause.  Do we really want to get out front of
>> the SQL committee on extending INSERT in an incompatible way?

> What is the additional functionality that we are considering adding here?
> The thread started out proposing a more convenient syntax, but it seems 
> to go deeper now and perhaps not everyone is following.

AIUI, the proposal is to allow INSERT commands to be written
using an UPDATE-like syntax, for example

INSERT INTO table SET col1 = value1, col2 = value2, ... [ FROM ... ]

where everything after FROM is the same as it is in SELECT.  My initial
belief was that this was strictly equivalent to what you could do with
a target-column-names list in standard INSERT, viz

INSERT INTO table (col1, col2, ...) VALUES (value1, value2, ...);
or
INSERT INTO table (col1, col2, ...) SELECT value1, value2, ... FROM ...

but it's arguably more legible/convenient because the column names
are written next to their values.

However, that rewriting falls down for certain multiassignment cases
where you have a row source that can't be decomposed, such as my
example

INSERT INTO table SET (col1, col2) = (SELECT value1, value2 FROM ...),
... [ FROM ... ]

So, just as we found for UPDATE, multiassignment syntax is strictly
stronger than plain column-by-column assignment.

There are some secondary issues about which variants of this syntax
will allow a column value to be written as DEFAULT, and perhaps
about whether set-returning functions work.  But the major point
right now is about whether its's possible to rewrite to standard
syntax.

            regards, tom lane

> On 26/03/2020, at 3:17 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> On 2020-03-24 18:57, Tom Lane wrote:
>>> No doubt that's all fixable, but the realization that some cases of
>>> this syntax are*not*  just syntactic sugar for standards-compliant
>>> syntax is giving me pause.  Do we really want to get out front of
>>> the SQL committee on extending INSERT in an incompatible way?
> 
>> What is the additional functionality that we are considering adding here?
>> The thread started out proposing a more convenient syntax, but it seems 
>> to go deeper now and perhaps not everyone is following.
> 
> AIUI, the proposal is to allow INSERT commands to be written
> using an UPDATE-like syntax, for example
> 
> INSERT INTO table SET col1 = value1, col2 = value2, ... [ FROM ... ]
> 
> where everything after FROM is the same as it is in SELECT.  My initial
> belief was that this was strictly equivalent to what you could do with
> a target-column-names list in standard INSERT, viz
> 
> INSERT INTO table (col1, col2, ...) VALUES (value1, value2, ...);
> or
> INSERT INTO table (col1, col2, ...) SELECT value1, value2, ... FROM ...
> 
> but it's arguably more legible/convenient because the column names
> are written next to their values.
> 
> However, that rewriting falls down for certain multiassignment cases
> where you have a row source that can't be decomposed, such as my
> example
> 
> INSERT INTO table SET (col1, col2) = (SELECT value1, value2 FROM ...),
> ... [ FROM ... ]
> 
> So, just as we found for UPDATE, multiassignment syntax is strictly
> stronger than plain column-by-column assignment.
> 
> There are some secondary issues about which variants of this syntax
> will allow a column value to be written as DEFAULT, and perhaps
> about whether set-returning functions work.  But the major point
> right now is about whether its's possible to rewrite to standard
> syntax.
> 
>             regards, tom lane

Attached is v6 of the patch.

As per the suggestion the SET clause list is checked for any
MultiAssigmentRef nodes and to report an error if any are found.

For example, the rule definition that previously caused a parser crash
would now produce the following error:

vagrant=> create rule r1 as on insert to foo do instead
vagrant-> insert into bar set (f1,f2,f3) = (select f1,f2,f3 from foo);
ERROR:  INSERT SET syntax does not support multi-assignment of columns.
LINE 2: insert into bar set (f1,f2,f3) = (select f1,f2,f3 from foo);
                            ^
HINT:  Specify the column assignments separately.


Requiring a FROM clause was a way to differentiate between an INSERT
with VALUES() which does allow DEFAULT and an INSERT with SELECT which
does not.

The idea was that it would help the user understand that they were writing
a different type of query and that DEFAULT would not be allowed in that
context.

To show what it would look like without that requirement I have removed
it from the v6 patch. In the first example works but the second one will
generate an error.

INSERT INTO t SET c1 = 1 WHERE true;
INSERT INTO t SET c1 = DEFAULT WHERE true;

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

It builds failed by applying to the latest code version, and I try head
'73025140885c889410b9bfc4a30a3866396fc5db' which work well.

The new status of this patch is: Waiting on Author

Hi Movead,

> On 22/04/2020, at 2:40 PM, movead li <movead.li@highgo.ca> wrote:
> 
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:       tested, passed
> Spec compliant:           tested, passed
> Documentation:            tested, passed
> 
> It builds failed by applying to the latest code version, and I try head
> '73025140885c889410b9bfc4a30a3866396fc5db' which work well.
> 
> The new status of this patch is: Waiting on Author

Thank you for the review, attached is v7 of the patch which should
apply correcly to HEAD.

This version now uses it's own production rule for the SET clause to
avoid the issue with MultiAssigmentRef nodes in the targetList.

> On 4/23/20 8:04 PM, Gareth Palmer wrote:
> >
> > Thank you for the review, attached is v7 of the patch which should
> > apply correcly to HEAD.
> >

Hello Gareth,

This patch no longer applies to HEAD, can you please submit a rebased version?

Thanks,
Rachel

Hello Rachel,

On Wed, 22 Sept 2021 at 17:13, Rachel Heaton <rachelmheaton@gmail.com> wrote:
>
> > On 4/23/20 8:04 PM, Gareth Palmer wrote:
> > >
> > > Thank you for the review, attached is v7 of the patch which should
> > > apply correcly to HEAD.
> > >
>
> Hello Gareth,
>
> This patch no longer applies to HEAD, can you please submit a rebased version?

Attached is a rebased version that should apply to HEAD.

Gareth

> Thanks,
> Rachel
>
>
>
>

Since this feature adds INSERT OVERRIDING SET syntax, it is recommended to add some related testcases.


Regards
Wenjing


> 2021年9月22日 07:38，Rachel Heaton <rachelmheaton@gmail.com> 写道：
>
>> On 4/23/20 8:04 PM, Gareth Palmer wrote:
>>>
>>> Thank you for the review, attached is v7 of the patch which should
>>> apply correcly to HEAD.
>>>
>
> Hello Gareth,
>
> This patch no longer applies to HEAD, can you please submit a rebased version?
>
> Thanks,
> Rachel
>
>

Justin Pryzby <pryzby@telsasoft.com> writes:
> You have to either include the pre-requisite patches as 0001, and your patch as
> 0002 (as I'm doing now), or name your patch something other than *.diff or
> *.patch, so cfbot doesn't think it's a new version of the patch to be tested.

This patch has been basically ignored for a full two years now.
(Remarkably, it's still passing in the cfbot.)

I have to think that that means there's just not enough interest
to justify committing it.  Should we mark it rejected and move on?
If not, what needs to happen to get it unstuck?

            regards, tom lane

On Wed, Mar 23, 2022 at 5:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Justin Pryzby <pryzby@telsasoft.com> writes:
> > You have to either include the pre-requisite patches as 0001, and your patch as
> > 0002 (as I'm doing now), or name your patch something other than *.diff or
> > *.patch, so cfbot doesn't think it's a new version of the patch to be tested.
>
> This patch has been basically ignored for a full two years now.
> (Remarkably, it's still passing in the cfbot.)
>
> I have to think that that means there's just not enough interest
> to justify committing it.  Should we mark it rejected and move on?
> If not, what needs to happen to get it unstuck?

I can help with review and/or other work here.  Please give me a
couple of weeks.


.m

On Thu, Apr 7, 2022 at 11:29 AM Marko Tiikkaja <marko@joh.to> wrote:
> I can help with review and/or other work here.  Please give me a
> couple of weeks.

Hi Marko, did you get a chance to pick up this patchset? If not, no
worries; I can mark this RwF and we can try again in a future
commitfest.

Thanks,
--Jacob

Hello,

Here is a new version of the patch that applies to HEAD.

It also adds some regression tests for overriding {system,user} values
based on Wenjing Zeng's work.

Gareth

On Thu, 14 Jul 2022 at 22:40, Jacob Champion <jchampion@timescale.com> wrote:
>
> On Thu, Apr 7, 2022 at 11:29 AM Marko Tiikkaja <marko@joh.to> wrote:
> > I can help with review and/or other work here.  Please give me a
> > couple of weeks.
>
> Hi Marko, did you get a chance to pick up this patchset? If not, no
> worries; I can mark this RwF and we can try again in a future
> commitfest.
>
> Thanks,
> --Jacob
>
>
>
>

As discussed in [1], we're taking this opportunity to return some
patchsets that don't appear to be getting enough reviewer interest.

This is not a rejection, since we don't necessarily think there's
anything unacceptable about the entry, but it differs from a standard
"Returned with Feedback" in that there's probably not much actionable
feedback at all. Rather than code changes, what this patch needs is more
community interest. You might

- ask people for help with your approach,
- see if there are similar patches that your code could supplement,
- get interested parties to agree to review your patch in a CF, or
- possibly present the functionality in a way that's easier to review
  overall.

(Doing these things is no guarantee that there will be interest, but
it's hopefully better than endlessly rebasing a patchset that is not
receiving any feedback from the community.)

Once you think you've built up some community support and the patchset
is ready for review, you (or any interested party) can resurrect the
patch entry by visiting

    https://commitfest.postgresql.org/38/2218/

and changing the status to "Needs Review", and then changing the
status again to "Move to next CF". (Don't forget the second step;
hopefully we will have streamlined this in the near future!)

Thanks,
--Jacob

[1]
https://postgr.es/m/flat/0ab66589-2f71-69b3-2002-49e821740b0d@timescale.com