Skip to content

Conversation

jseldess
Copy link
Contributor

@jseldess jseldess commented May 20, 2016

Fixes #302 and #279

@paperstreet, PTAL.


This change is Reviewable

@jseldess
Copy link
Contributor Author

@danhhz
Copy link
Contributor

danhhz commented May 20, 2016

Great stuff! I've never reviewed public facing docs before, so I erred on the side of overcommunication. Feel free to ignore any suggestions you feel aren't really appropriate or are too pedantic

Previously, jseldess wrote…

@paperstreet, here are html version, in case you want to view that way:


Review status: 0 of 5 files reviewed at latest revision, 11 unresolved discussions.


insert.md, line 48 [r1] (raw file):

### Default Values

[Defaults values](data-definition.html) are used when you leave specific columns out of your statement, or when you explicitly request default values. For example, both of the following statements would create a row with `balance` filled with its default value, in this case `NULL`:

nit: Default values


insert.md, line 51 [r1] (raw file):

~~~ sql
INSERT INTO accounts (id, balance) VALUES (5);

I'm surprised this works. Seems a little fishy even if it does. I think of the "leave specific columns out of your statement" part more in terms of columns in the table that you've left out of the insert statement: create table ab (a int, b int default 7); insert into ab (a) values (1); will use the default for b

tl;dr consider leaving ", balance" out of this line


insert.md, line 77 [r1] (raw file):

To return all columns of the inserted rows, use the * character:

Dunno how fine of a point you want to put on it, but this returns all columns in the table, not all columns being inserted.


insert.md, line 106 [r1] (raw file):

### Update Values with `ON CONFLICT`

Normally, when inserted values conflict with a `UNIQUE` constraint on one or more columns, CockroachDB returns an error. To update the affected rows instead, use an `ON CONFLICT` clause containing the column with the unique conflict and the `DO UPDATE SET` expression set to the columns to be updated:

if the unique index has more than one column, you have to list all of them (and in the same order as in the index)


insert.md, line 112 [r1] (raw file):

When a unique conflict is detected, CockroachDB temporarily stores the row(s) in a table called excluded. As you can see above, you set the columns to be update to the corresponding columns in the temporary excluded table.

nit: row not row(s)
nit: "to be updated to"


insert.md, line 116 [r1] (raw file):

Note the following limitations:

-   It's not possible to update the same row twice with a single `INSERT ... ON CONFLICT` statement. For example, the following would not be allowed:

missing example?


insert.md, line 123 [r2] (raw file):

### Ignore Insert `ON CONFLICT`

To avoid an error in case an insert conflicts with a `UNIQUE` constraint, set the `ON CONFLICT` clause to `DO NOTHING`:

"to avoid an error" gives me an incomplete picture. maybe explicitly say that any rows that conflict leave the ones in the database untouched and that the statement succeeds


insert.md, line 29 [r3] (raw file):

`select_stmt` | A [`SELECT`](select.html) statement to retrieve values from another table and insert them as new rows. The target columns in the `SELECT` statement must match the type of the columns being inserted into.
`opt_on_conflict` |
`opt_on_conflict` | 

opt_on_conflict twice?


insert.md, line 34 [r4] (raw file):

`DEFAULT VALUES` | To fill all columns with their [default values](data-definition.html#default-value), use `DEFAULT VALUES` in place of `select_stmt`. To fill a specific column with its default value, leave the value out of the `select_stmt` or use `DEFAULT` at the appropriate position. See the [Insert Default Values](#insert-default-values) examples below.
`RETURNING target_list` | Return values based on rows inserted, where `target_list` can be specific column names from the table, `*` for all columns, or a computation on specific columns. See the [Insert and Return Values](#insert-and-return-values) example below. 
`on_conflict` | Normally, when inserted values conflict with a `UNIQUE` constraint on one or more columns, CockroachDB returns an error. To update the affected rows instead, use an `ON CONFLICT` clause containing the column with the unique constraint and the `DO UPDATE SET` expression set to the column(s) to be updated. To avoid an error, set the `ON CONFLICT` clause to `DO NOTHING`. See the [Update Values ON CONFLICT](#update-values-on-conflict) and [Ignore Insert ON CONFLICT](#ignore-insert-on-conflict) examples below.<br><br>Note that it's not possible to update the same row twice with a single `INSERT ... ON CONFLICT` statement. Also, if the values in the `SET` expression cause uniqueness conflicts, CockroachDB will return an error.<br><br>As a short-hand alternative to the `ON CONFLICT` clause, you can use the [`UPSERT`](upsert.html) statement. However, `UPSERT` does not let you specify the column with the unique constraint; it assumes that the column is the primary key. Using `ON CONFLICT` is therefore more flexible.

FWIW, the set expressions are also more flexible


insert.md, line 12 [r7] (raw file):

## Required Privileges

The user must have the `INSERT` [privilege](privileges.html) on the table. To use `ON CONFLICT`, the user must also have the `UPDATE` privilege on the table.

UPDATE should only be necessary for ON CONFLICT DO UPDATE. DO NOTHING should still only require INSERT (though I realize this is not currently true, I'll go file an issue: cockroachdb/cockroach#6826)


upsert.md, line 8 [r4] (raw file):

The `UPSERT` [statement](sql-statements.html) is short-hand for [`INSERT ... ON CONFLICT`](insert.html). It inserts rows in cases where specified values do not violate uniqueness constraints, and it updates rows in cases where values do violate uniqueness constraints. 

Note that `UPSERT` does not let you specify the column with the unique constraint; it assumes that the column is the primary key. Using `INSERT ... ON CONFLICT` is therefore more flexible.

Any UPSERT statement can be exactly transformed into an INSERT ... ON CONFLICT. I'd love to see this transformation called out.

UPSERT INTO foo (a, b) VALUES (1, 2) exactly transforms into INSERT INTO foo (a, b) VALUES (1, 2) ON CONFLICT (<primary key columns>) DO UPDATE SET b = excluded b where there is a SET expr for every column that's in qualified_name list but not in on_conflict


Comments from Reviewable

@jseldess
Copy link
Contributor Author

Review status: 0 of 5 files reviewed at latest revision, 11 unresolved discussions.


insert.md, line 48 [r1] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

nit: Default values

Sorry, you're looking at an earlier commit again. I need to learn how to clean them up.

insert.md, line 51 [r1] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

I'm surprised this works. Seems a little fishy even if it does. I think of the "leave specific columns out of your statement" part more in terms of columns in the table that you've left out of the insert statement: create table ab (a int, b int default 7); insert into ab (a) values (1); will use the default for b

tl;dr consider leaving ", balance" out of this line

Taking `balance` out of that example.

insert.md, line 77 [r1] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

Dunno how fine of a point you want to put on it, but this returns all columns in the table, not all columns being inserted.

I think the latest version is ok, but let me know if not.

insert.md, line 106 [r1] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

if the unique index has more than one column, you have to list all of them (and in the same order as in the index)

Done.

insert.md, line 112 [r1] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

nit: row not row(s)
nit: "to be updated to"

I think this is correct in the latest version. Please let me know if you still see trouble.

insert.md, line 116 [r1] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

missing example?

From an earlier commit. Shouldn't be a problem in the latest version.

insert.md, line 123 [r2] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

"to avoid an error" gives me an incomplete picture. maybe explicitly say that any rows that conflict leave the ones in the database untouched and that the statement succeeds

Fixed this language a bit and enhanced the related example toward the bottom of the page.

insert.md, line 29 [r3] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

opt_on_conflict twice?

This is fixed. I think you must've been looking at an earlier commit (sorry they're not so easy to follow).

insert.md, line 34 [r4] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

FWIW, the set expressions are also more flexible

I didn't realize this. Can you give me some examples?

insert.md, line 12 [r7] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

UPDATE should only be necessary for ON CONFLICT DO UPDATE. DO NOTHING should still only require INSERT (though I realize this is not currently true, I'll go file an issue: cockroachdb/cockroach#6826)

Changing `ON CONFLICT` to `ON CONFLICT DO UPDATE` to be more specific.

upsert.md, line 8 [r4] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

Any UPSERT statement can be exactly transformed into an INSERT ... ON CONFLICT. I'd love to see this transformation called out.

UPSERT INTO foo (a, b) VALUES (1, 2) exactly transforms into INSERT INTO foo (a, b) VALUES (1, 2) ON CONFLICT (<primary key columns>) DO UPDATE SET b = excluded b where there is a SET expr for every column that's in qualified_name list but not in on_conflict

Adding a "How `UPSERT` Tranforms into `INSERT ON CONFLICT`" section.

Comments from Reviewable

@jseldess jseldess force-pushed the jseldess/insert-upsert branch from 8c3745a to fc8dbb4 Compare May 20, 2016 15:55
@danhhz
Copy link
Contributor

danhhz commented May 20, 2016

:lgtm:

Previously, paperstreet (Daniel Harrison) wrote…

Great stuff! I've never reviewed public facing docs before, so I erred on the side of overcommunication. Feel free to ignore any suggestions you feel aren't really appropriate or are too pedantic


Review status: 0 of 5 files reviewed at latest revision, 12 unresolved discussions.


insert.md, line 34 [r4] (raw file):

Previously, jseldess wrote…

I didn't realize this. Can you give me some examples?

It accepts any expression that UPDATE does. A useful concrete example is counting things:

CREATE TABLE counters (name STRING PRIMARY KEY, count INT); INSERT INTO counters VALUES ('foo', 1) ON CONFLICT (name) DO UPDATE SET count = counters.count + excluded.count;


upsert.md, line 14 [r8] (raw file):

## Required Privileges

When there is no uniqueness conflict on the primary key, the user must have the `INSERT` [privilege](privileges.html) on the table. When there is a uniqueness conflict on the primary key, the user must have the `UPDATE` privilege on the table. 

UPSERT always requires both privileges


Comments from Reviewable

@jseldess
Copy link
Contributor Author

Review status: 0 of 5 files reviewed at latest revision, 12 unresolved discussions.


insert.md, line 34 [r4] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

It accepts any expression that UPDATE does. A useful concrete example is counting things:

CREATE TABLE counters (name STRING PRIMARY KEY, count INT); INSERT INTO counters VALUES ('foo', 1) ON CONFLICT (name) DO UPDATE SET count = counters.count + excluded.count;

Added reference to `UPDATE`.

upsert.md, line 14 [r8] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

UPSERT always requires both privileges

Done.

Comments from Reviewable

@jseldess jseldess merged commit 0084bae into gh-pages May 20, 2016
@jseldess jseldess deleted the jseldess/insert-upsert branch May 20, 2016 18:27
Simran-B pushed a commit to Simran-B/docs that referenced this pull request Aug 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants