/cockroach

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: assorted mutation fixes #31222

Merged
merged 10 commits into from Oct 11, 2018

Conversation

Reviewers

@BramGruneir BramGruneir

@jordanlewis jordanlewis

@cockroachdb/sql-wiring-prs sql-wiring-prs

@cockroachdb/sql-ccl-prs sql-ccl-prs

@cockroachdb/sql-rest-prs sql-rest-prs

@cockroachdb/sql-execution-prs sql-execution-prs

@cockroachdb/sql-planning-prs sql-planning-prs

@cockroachdb/sql-async-prs sql-async-prs

Assignees
No one assigned
Labels
None yet
Projects
  
Finished (milestone r2.1) in SQL Front-end, Lang & Semantics
Milestone
No milestone
4 participants
@knz @cockroach-teamcity @jordanlewis @BramGruneir
@knz
Member

knz commented Oct 10, 2018 •
edited

Fixes #25742.
Fixes #26106.
Fixes #29494.
Fixes #31255.
Fixes #29497.

This may even full address #26106 but I have a couple more checks to add first to confirm this. edit: it does

@knz knz requested review from jordanlewis and BramGruneir Oct 10, 2018

@knz knz added this to Triage in SQL Front-end, Lang & Semantics via automation Oct 10, 2018

@knz knz requested review from cockroachdb/sql-async-prs as code owners Oct 10, 2018

@cockroach-teamcity

This comment has been minimized.

Show comment
Hide comment
@cockroach-teamcity

cockroach-teamcity Oct 10, 2018

Member

This change is Reviewable
Member

cockroach-teamcity commented Oct 10, 2018

This change is Reviewable
@knz

This comment has been minimized.

Show comment
Hide comment
@knz

knz Oct 10, 2018

Member

I did manually check that the insert/upsert fast paths do verify check and column constraints. Also in the code it's pretty clear because both the slow and fast path the same GenerateInsertRow() method which performs the check.

This was not true in previous versions, but I had forgotten that I fixed this at some point :)
Member

knz commented Oct 10, 2018 •
edited

I did manually check that the insert/upsert fast paths do verify check and column constraints. Also in the code it's pretty clear because both the slow and fast path the same GenerateInsertRow() method which performs the check.

This was not true in previous versions, but I had forgotten that I fixed this at some point :)
@knz

This comment has been minimized.

Show comment
Hide comment
@knz

knz Oct 11, 2018

Member

I think I squashed them all. It would be great if this got reviewed and backported this week.
Member

knz commented Oct 11, 2018

I think I squashed them all. It would be great if this got reviewed and backported this week.
@knz

This comment has been minimized.

Show comment
Hide comment
@knz

knz Oct 11, 2018

Member

yay it's green
Member

knz commented Oct 11, 2018

yay it's green
@BramGruneir

BramGruneir approved these changes Oct 11, 2018
View changes

:lgtm:

Just a few of small comments.

Well done!

Reviewed 10 of 10 files at r1, 3 of 3 files at r2, 4 of 4 files at r3, 2 of 2 files at r4, 6 of 6 files at r5, 3 of 3 files at r6, 1 of 1 files at r7, 2 of 2 files at r8, 2 of 2 files at r9, 2 of 2 files at r10.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/sql/descriptor_mutation_test.go, line 307 at r9 (raw file):

						_, err = sqlDB.Exec(`INSERT INTO t.test VALUES ('b', 'y', 'i')`)
					}
					if !testutils.IsError(err, "(IN|UP)SERT has more expressions than target columns, 3 expressions for 2 targets") {

Why would this be upsert?

pkg/sql/logictest/testdata/logic_test/collatedstring, line 219 at r1 (raw file):

  ('ü' COLLATE en)

statement error value type collatedstring{de} doesn't match type STRING COLLATE en of column "a"

I prefer the earlier error. It was clearer on what was wrong.

pkg/sql/logictest/testdata/logic_test/delete, line 187 at r5 (raw file):

1

# Regression tests for #29494

subtest please

pkg/sql/logictest/testdata/logic_test/insert, line 545 at r3 (raw file):

# This test requires that the error message for a CHECK constraint
# validation error be different than a column validation error. So we
# test the former first, as a sanity check.

Subtest please, and please pass these through sqlfmt.

pkg/sql/logictest/testdata/logic_test/insert, line 566 at r4 (raw file):


# Verify that column constraints and CHECK handling occur before
# foreign key validation.

subtest please

pkg/sql/logictest/testdata/logic_test/insert, line 577 at r4 (raw file):


statement error failed to satisfy CHECK constraint
INSERT INTO derived(y) VALUES('abcd')

please add an FK violation here too then one that passes all checks, just to complete the test

pkg/sql/logictest/testdata/logic_test/insert, line 579 at r5 (raw file):

INSERT INTO derived(y) VALUES('abcd')

# Regression tests for #29494

subtest please

pkg/sql/logictest/testdata/logic_test/update, line 395 at r3 (raw file):

# validation error be different than a column validation error. So we
# test the former first, as a sanity check.
statement ok

subtest please, and sqlfmt.

pkg/sql/logictest/testdata/logic_test/update, line 416 at r4 (raw file):

UPDATE tn2 SET y = 'abcd'

# Verify that column constraints and CHECK handling occur before

subtest, and add an fk violation and then a case that passes all.

pkg/sql/logictest/testdata/logic_test/update, line 431 at r5 (raw file):

UPDATE derived SET y = 'abcd'

# Regression tests for #29494

subtest please

Nice test.

pkg/sql/logictest/testdata/logic_test/upsert, line 601 at r3 (raw file):

# validation error be different than a column validation error. So we
# test the former first, as a sanity check.
statement ok

subtest please and sqlfmt

pkg/sql/logictest/testdata/logic_test/upsert, line 620 at r5 (raw file):

UPSERT INTO tn2(x, y) VALUES (123, 'abcd')

# Regression tests for #29494

subtest please

pkg/sql/logictest/testdata/logic_test/upsert, line 648 at r8 (raw file):

ROLLBACK

# Regression test for #31255

subtest please

pkg/sql/logictest/testdata/logic_test/upsert, line 662 at r10 (raw file):

UPSERT INTO tc VALUES (1,2)

# Regression test for #29497

subtest please

@knz

knz reviewed Oct 11, 2018
View changes

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/sql/descriptor_mutation_test.go, line 307 at r9 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Why would this be upsert?

See the 5 lines before: the test uses both forms one after the other.

pkg/sql/logictest/testdata/logic_test/collatedstring, line 219 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

I prefer the earlier error. It was clearer on what was wrong.

I'll file a followup PR to clarify. Having a detailed error message requires more and more complex code.

pkg/sql/logictest/testdata/logic_test/delete, line 187 at r5 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

subtest please

Done.

pkg/sql/logictest/testdata/logic_test/insert, line 545 at r3 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Subtest please, and please pass these through sqlfmt.

Done.

sqlfmt doesn't make any change - it all fits on 1 line.

pkg/sql/logictest/testdata/logic_test/insert, line 566 at r4 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

subtest please

Done.

pkg/sql/logictest/testdata/logic_test/insert, line 577 at r4 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

please add an FK violation here too then one that passes all checks, just to complete the test

Done.

pkg/sql/logictest/testdata/logic_test/insert, line 579 at r5 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

subtest please

Done.

pkg/sql/logictest/testdata/logic_test/update, line 395 at r3 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

subtest please, and sqlfmt.

Done.

pkg/sql/logictest/testdata/logic_test/update, line 416 at r4 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

subtest, and add an fk violation and then a case that passes all.

Done.

pkg/sql/logictest/testdata/logic_test/update, line 431 at r5 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

subtest please

Nice test.

Done.

pkg/sql/logictest/testdata/logic_test/upsert, line 601 at r3 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

subtest please and sqlfmt

Done.

pkg/sql/logictest/testdata/logic_test/upsert, line 620 at r5 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

subtest please

Done.

pkg/sql/logictest/testdata/logic_test/upsert, line 648 at r8 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

subtest please

Done.

pkg/sql/logictest/testdata/logic_test/upsert, line 662 at r10 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

subtest please

Done.

knz added some commits Oct 10, 2018

@knz
sql: fix the schema type check for INSERT/UPSERT 
The types of the input data for a mutation (the data source clause for
INSERT/UPSERT, the result of SET RHS for UPDATE) must match the types
of the columns being mutated.

Prior to this patch, this check was properly done early by UPDATE,
however it was not done at the right time for INSERT/UPSERT.

The code for INSERT/UPSERT was relying on an assertion check very deep
down in the encoding of datums to KV values. This assertion check was
never meant to be an input validation check, but because Go does not
really give us semantic assertions nobody noticed the check was
abused.

This patch clarifies the situation by introducing
`pgerror.NewAssertionErrorf` and using it there as appropriate. Later
patches can introduce more uses of pgerror.NewAssertionErrorf where
appropriate (e.g. everywhere we already use CodeInternalError).

In addition, the proper check for INSERT/UPSERT is introduced at the
right place, early during semantic analysis.

Release note (bug fix): CockroachDB will no longer fail in unexpected
ways or write invalid data when the type of input values provided to
INSERT/UPSERT does not match the type of the target columns.
d3598eb
@knz
sql: produce the right pg error codes for column constraint failures 
Release note: None
0c7d584
@knz
sql: ensure UPDATE checks column constraints before CHECK constraints 
This is required for postgres compatibility: the CHECK expressions
must be able to assume that the column data is already valid according
to the column type and nullability constraint.

The code for INSERT/UPSERT was already valid, only UPDATE needed to be
fixed. However this patch provides the corresponding test for all 3
mutations.

Release note (bug fix): UPDATE now verifies the column constraints
before CHECK constraints, for compatibility with PostgreSQL.
30a2af7
@knz
sql: add missing mutation check tests 
The mutations must validate column constraints before they perform FK
checks. There were no clear tests for this. This patch adds the
missing tests.

Release note: None
5e2f02c
@knz
sql: prevent UPDATE RETURNING from seeing non-public columns 
Prior to this patch, it was possible to use a newly added but not yet
visible column in UPDATE RETURNING. This patch fixes it.

INSERT/UPSERT/DELETE were not affected, however this patch adds the
corresponding tests for all 4 mutation statements -- this aspect
was not tested before.

Release note (bug fix): It is not possible any more to use
not-fully-added-yet columns in the RETURNING clause of UPDATE
statements.
55706f8
@knz
sql: fix the generation of an error message 
Release note: None
f54b09d
@knz
sql: add a missing comment 
Release note: None
c222698
@knz
sql: prevent INSERT DO UPDATE from silently setting computed columns 
Release note (bug fix): CockroachDB does no longer incorrectly and
silently accept a computed column on the left side of the assignment
in a ON CONFLICT DO UPDATE SET clause.
19794f3
@knz
sql: improve an error message 
Release note: None
82a5199
@knz
sql: prevent INSERT DO UPDATE SET from assigning mutation columns 
This is a band aid, a refactoring of the upsert pipeline is required
for a more robust solution.

Release note (bug fix): CockroachDB does no longer incorrectly and
silently accept a not-fully-added-yet column on the left side of the
assignment in a ON CONFLICT DO UPDATE SET clause.
Loading status checks…
feb9c84
@knz

This comment has been minimized.

Show comment
Hide comment
@knz

knz Oct 11, 2018

Member

should I prepare the backport already?
Member

knz commented Oct 11, 2018

should I prepare the backport already?
@BramGruneir

BramGruneir reviewed Oct 11, 2018
View changes

:lgtm:

And yes, go for it.

Reviewed 1 of 3 files at r12, 4 of 6 files at r15, 2 of 3 files at r16, 1 of 2 files at r19, 2 of 2 files at r20.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)

@jordanlewis

jordanlewis approved these changes Oct 11, 2018
View changes

:lgtm: wow, great work

Reviewed 5 of 10 files at r1, 12 of 12 files at r11, 3 of 3 files at r12, 4 of 4 files at r13, 2 of 2 files at r14, 5 of 6 files at r15, 2 of 3 files at r16, 1 of 1 files at r17, 2 of 2 files at r18, 2 of 2 files at r19.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale)

pkg/sql/sqlbase/column_type_properties.go, line 654 at r12 (raw file):

		if typ.Width > 0 && utf8.RuneCountInString(sv) > int(typ.Width) {
			return pgerror.NewErrorf(pgerror.CodeStringDataRightTruncationError,

yay! nice precise pg errors.

@knz

This comment has been minimized.

Show comment
Hide comment
@knz

knz Oct 11, 2018

Member

cool thanks

bors r+
Member

knz commented Oct 11, 2018

cool thanks

bors r+

craig bot pushed a commit that referenced this pull request Oct 11, 2018

craig[bot] @knz
Merge #31007 #31222 
31007: sql: properly support CTEs inside views r=knz a=knz

Fixes #23833.


31222: sql: assorted mutation fixes r=knz a=knz

Fixes #25742.
Fixes #26106.
Fixes #29494.
Fixes #31255.
Fixes #29497.

~This may even full address #26106 but I have a couple more checks to add first to confirm this.~ edit: it does



Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
Loading status checks…
e900cd8

@knz knz referenced this pull request Oct 11, 2018

Merged

release-2.1: sql: assorted mutation fixes #31280

@craig

This comment has been minimized.

Show comment
Hide comment
@craig

craig bot Oct 11, 2018

Build succeeded

craig bot commented Oct 11, 2018

Build succeeded

@craig craig bot merged commit feb9c84 into cockroachdb:master Oct 11, 2018
3 checks passed

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@knz knz deleted the knz:20181010-coltype-check branch Oct 11, 2018

@knz knz moved this from Triage to Finished (milestone r2.1) in SQL Front-end, Lang & Semantics Oct 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment