[PATCH] Implement INSERT SET syntax

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
31 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCH] Implement INSERT SET syntax

Gareth Palmer
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


insert-set-v1.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Implement INSERT SET syntax

Marko Tiikkaja-4
On Wed, Jul 17, 2019 at 7:30 AM Gareth Palmer <[hidden email]> 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.


.m
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Implement INSERT SET syntax

Gareth Palmer
Hi Marko,

> On 17/07/2019, at 5:52 PM, Marko Tiikkaja <[hidden email]> wrote:
>
> On Wed, Jul 17, 2019 at 7:30 AM Gareth Palmer <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Implement INSERT SET syntax

Kyotaro Horiguchi-4
Hello.

At Thu, 18 Jul 2019 11:30:04 +1200, Gareth Palmer <[hidden email]> wrote in <[hidden email]>

> Hi Marko,
>
> > On 17/07/2019, at 5:52 PM, Marko Tiikkaja <[hidden email]> wrote:
> >
> > On Wed, Jul 17, 2019 at 7:30 AM Gareth Palmer <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Implement INSERT SET syntax

Gareth Palmer
Hi Kyotaro,

Thank-you for looking at the patch.

> On 18/07/2019, at 6:54 PM, Kyotaro Horiguchi <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Implement INSERT SET syntax

Ibrar Ahmed-4
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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Implement INSERT SET syntax

Gareth Palmer
Hi Ibrar,

> On 16/08/2019, at 7:14 AM, Ibrar Ahmed <[hidden email]> 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


insert-set-v2.patch (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Implement INSERT SET syntax

akapila
In reply to this post by Gareth Palmer
On Wed, Jul 17, 2019 at 10:00 AM Gareth Palmer <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Implement INSERT SET syntax

Ibrar Ahmed-4


On Fri, Aug 16, 2019 at 8:19 AM Amit Kapila <[hidden email]> wrote:
On Wed, Jul 17, 2019 at 10:00 AM Gareth Palmer <[hidden email]> 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.

 
I agree with you Amit, that we need a consensus on that. Do we really need that
feature or not. In the previous discussion, there was no resistance to have that
in PostgreSQL, but some problem with the patch. Current patch is very simple
and not invasive, but still, we need a consensus on that.

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




--
Ibrar Ahmed
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Implement INSERT SET syntax

Peter Eisentraut-6
In reply to this post by akapila
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


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Implement INSERT SET syntax

Vik Fearing-4
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



Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Implement INSERT SET syntax

Tom Lane-2
Vik Fearing <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Implement INSERT SET syntax

Tom Lane-2
In reply to this post by Peter Eisentraut-6
Peter Eisentraut <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Implement INSERT SET syntax

Gareth Palmer
Hi Tom,

> On 19/08/2019, at 3:00 AM, Tom Lane <[hidden email]> wrote:
>
> Peter Eisentraut <[hidden email]> 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





insert-set-v3.patch (19K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Implement INSERT SET syntax

Ibrar Ahmed-4
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 documentaion changes.

The new status of this patch is: Ready for Committer
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Implement INSERT SET syntax

Robert Haas
In reply to this post by Tom Lane-2
On Sun, Aug 18, 2019 at 11:00 AM Tom Lane <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Implement INSERT SET syntax

Marko Tiikkaja-4
On Fri, Nov 1, 2019 at 6:31 PM Robert Haas <[hidden email]> wrote:
On Sun, Aug 18, 2019 at 11:00 AM Tom Lane <[hidden email]> 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.

I still do, and this would be a big help.  I don't care if it's non-standard.


.m
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Implement INSERT SET syntax

Tom Lane-2
In reply to this post by Gareth Palmer
Gareth Palmer <[hidden email]> writes:
>> On 19/08/2019, at 3:00 AM, Tom Lane <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Implement INSERT SET syntax

Pantelis Theodosiou


On Thu, Nov 14, 2019 at 9:20 PM Tom Lane <[hidden email]> wrote:
Gareth Palmer <[hidden email]> writes:
>> On 19/08/2019, at 3:00 AM, Tom Lane <[hidden email]> 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.

...

I'm setting this back to Waiting on Author.

                        regards, tom lane



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

INSERT INTO t (a,b)
SELECT a+10. b+10
FROM t
ORDER BY a 
LIMIT 3;

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.

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

insert into t (a, b)
select
    a + 20,
    b - 2 * a as f
from t
order by f desc
limit 3 ;

Would that be expressed as follows?:

insert into t
set
    a = a + 20, 
    b = b - 2 * a as f
from t
order by f desc
limit 3 ;

Best regards,
Pantelis Theodosiou
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Implement INSERT SET syntax

Tom Lane-2
Pantelis Theodosiou <[hidden email]> writes:
> On 19/08/2019, at 3:00 AM, Tom Lane <[hidden email]> 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


12