Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: the updated values on ON CONFLICT do not get constraint-checked properly #23699

Closed
knz opened this issue Mar 10, 2018 · 7 comments · Fixed by #26642
Closed

sql: the updated values on ON CONFLICT do not get constraint-checked properly #23699

knz opened this issue Mar 10, 2018 · 7 comments · Fixed by #26642
Assignees
Labels
A-sql-mutations Mutation statements: UPDATE/INSERT/UPSERT/DELETE. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. docs-done docs-known-limitation S-0-visible-logical-error Database stores inconsistent data in some cases, or queries return invalid results silently.
Milestone

Comments

@knz
Copy link
Contributor

knz commented Mar 10, 2018

Consider the following table:

CREATE TABLE ab(a INT PRIMARY KEY, b INT, CHECK (b < 1))

A simple INSERT statement fails, as it should:

> INSERT INTO ab(a,b) VALUES(1, 12312);
pq: failed to satisfy CHECK constraint (b < 1)

Now, the same with INSERT ... ON CONFLICT:

> INSERT INTO ab(a, b) VALUES (1,0); -- create some initial valid value
> INSERT INTO ab(a, b) VALUES (1,0) ON CONFLICT(a) DO UPDATE SET b = 123132;

This succeeds!
And results in a row that fails constraint validation:

> SELECT * FROM ab;
+---+--------+
| a |   b    |
+---+--------+
| 1 | 123132 |
+---+--------+
(1 row)

This may or may not need to be fixed for 2.0, but certainly needs a note in docs as known limitation.

@knz knz added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Mar 10, 2018
@knz knz added this to the 2.0 milestone Mar 10, 2018
@knz
Copy link
Contributor Author

knz commented Mar 10, 2018

Found while working on #23698 / #23373.

@knz
Copy link
Contributor Author

knz commented Mar 10, 2018

cc @justinj @dt.

@knz knz self-assigned this Mar 10, 2018
@dt
Copy link
Member

dt commented Mar 10, 2018

cc @BramGruneir should see if this is an issue with FKs too.

@knz
Copy link
Contributor Author

knz commented Mar 11, 2018

I checked - FKs do get checked properly, they use a different code path (in the RowInserter/RowUpdater from sqlbase) which is properly run in this case.

@dt
Copy link
Member

dt commented Mar 12, 2018

Thanks!. Let's make sure we get test coverage though all the same, as this is a really easy mutation path to overlook.

@jseldess
Copy link
Contributor

jseldess commented Apr 1, 2018

Documented as a known limitation in cockroachdb/docs#2823.

@petermattis petermattis modified the milestones: 2.0, 2.0.x Apr 5, 2018
@knz knz added S-0-corruption-or-data-loss Unrecoverable corruption, data loss, or other catastrophic issues that can’t be fixed by upgrading. backport-2.0.x labels Apr 24, 2018
@knz knz added S-0-visible-logical-error Database stores inconsistent data in some cases, or queries return invalid results silently. and removed S-0-corruption-or-data-loss Unrecoverable corruption, data loss, or other catastrophic issues that can’t be fixed by upgrading. labels Apr 29, 2018
@knz knz added the A-sql-mutations Mutation statements: UPDATE/INSERT/UPSERT/DELETE. label May 15, 2018
@knz knz assigned emsal0 and unassigned knz Jun 6, 2018
@knz
Copy link
Contributor Author

knz commented Jun 6, 2018

@emsal1863 when you're done with #6639 / #26465 you can have a look at this. It's a natural followup now that you understand how this works.

craig bot pushed a commit that referenced this issue Jun 13, 2018
26642: sql: Check CHECK constraints on upsert when updating a conflicting row r=emsal1863 a=emsal1863

* Before: On a table with a CHECK constraint, such as:

    ```
    CREATE TABLE ex(foo INT PRIMARY KEY, bar INT, CHECK (bar < 2)
    ```

    an upsert on the table would not properly check the CHECK constraints,
    e.g.

    ```
    INSERT INTO ex(foo, bar) VALUES(1, 2);
    INSERT INTO ex(foo, bar) VALUES(1, 2) ON CONFLICT (foo) DO UPDATE SET
        bar = 3
    ```

    would update the row, violating the check constraint.

* After: On attempting such an operation, CockroachDB now throws the
proper database error "failed to satisfy CHECK constraint ..."

Release note (sql change): CHECK constraints are now checked when
updating a conflicting row in INSERT ... ON CONFLICT DO UPDATE
statements.

closes #23699 

Co-authored-by: Emmanuel <[email protected]>
@craig craig bot closed this as completed in #26642 Jun 13, 2018
craig bot pushed a commit that referenced this issue Jun 13, 2018
26699: release-2.0: sql: Check CHECK constraints on upsert when updating a conflicting row r=emsal1863 a=emsal1863

Backport 1/1 commits from #26642.

/cc @cockroachdb/release

---

* Before: On a table with a CHECK constraint, such as:

    ```
    CREATE TABLE ex(foo INT PRIMARY KEY, bar INT, CHECK (bar < 2)
    ```

    an upsert on the table would not properly check the CHECK constraints,
    e.g.

    ```
    INSERT INTO ex(foo, bar) VALUES(1, 2);
    INSERT INTO ex(foo, bar) VALUES(1, 2) ON CONFLICT (foo) DO UPDATE SET
        bar = 3
    ```

    would update the row, violating the check constraint.

* After: On attempting such an operation, CockroachDB now throws the
proper database error "failed to satisfy CHECK constraint ..."

Release note (sql change): CHECK constraints are now checked when
updating a conflicting row in INSERT ... ON CONFLICT DO UPDATE
statements.

closes #23699 


Co-authored-by: Emmanuel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-mutations Mutation statements: UPDATE/INSERT/UPSERT/DELETE. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. docs-done docs-known-limitation S-0-visible-logical-error Database stores inconsistent data in some cases, or queries return invalid results silently.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants