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

opt, sql: add end-to-end support for uniqueness checks #58053

Merged
merged 3 commits into from
Dec 21, 2020

Conversation

rytaft
Copy link
Collaborator

@rytaft rytaft commented Dec 17, 2020

opt: generalize explain output for constraint checks

This commit changes the explain output for constraint checks to
say "constraint-check" rather than "fk-check" so that it can be used
for uniqueness checks in addition to foreign key checks.

Release note (sql change): The explain output for foreign key checks
is now labeled constraint-check rather than fk-check. This change
is in preparation for adding support for unique constraint checks,
which will use the same label.

sql: update unique violation error to be Postgres compatible

This commit fixes the violation error for unique constraints to match
the message used by Postres. Previously, the error would have been
printed as:

duplicate key value (k)=(1) violates unique constraint "primary"

Now this message is printed as:

ERROR:  duplicate key value violates unique constraint "primary"
DETAIL: Key (k)=(1) already exists.

Release note (sql change): The error message for unique constraint
violations now matches the error used by Postgres. For example,
the new error message looks like this:
ERROR: duplicate key value violates unique constraint "primary"
DETAIL: Key (k)=(1) already exists.

opt: add end-to-end support for uniqueness checks

This commit updates the execbuilder to build uniqueness checks for
insert and update queries when needed. It also fixes a few other bugs
in the existing code that were preventing the checks from being built.
As of this commit, uniqueness checks for insert and update now work
end-to-end. This commit also includes a number of execbuilder and logic
tests for uniqueness checks.

There is no release note since these checks are still gated behind the
experimental_enable_unique_without_index_constraints session variable.

Informs #41535

Release note: None

@rytaft rytaft requested review from mgartner, RaduBerinde and a team December 17, 2020 23:04
@rytaft rytaft requested a review from a team as a code owner December 17, 2020 23:04
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Very cool!

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner, @RaduBerinde, and @rytaft)


pkg/sql/logictest/testdata/logic_test/unique, line 133 at r3 (raw file):

INSERT INTO uniq_overlaps_pk VALUES (3, 3, 1, 3)

query IIII colnames

rowsort (+ more instances below)


pkg/sql/opt/exec/execbuilder/mutation.go, line 784 at r3 (raw file):

}

func (b *Builder) buildUniqueChecks(checks memo.UniqueChecksExpr) error {

[nit] could use a function comment (again, reminding the reader that this only concerns "unique without index")


pkg/sql/opt/exec/execbuilder/testdata/unique, line 83 at r3 (raw file):

│             size: 5 columns, 2 rows
│
├── • constraint-check

[nit] please file an issue to plumb some info about the constraint (kind, name) here


pkg/sql/opt/optbuilder/mutation_builder_unique.go, line 215 at r3 (raw file):

		Table:        h.mb.tabID,
		CheckOrdinal: h.uniqueOrdinal,
		KeyCols:      withScanCols[:len(h.uniqueOrdinals)],

This needs a comment, why is it ok? (I assume uniqueOrdinals is always a prefix of uniqueAndPrimaryKeyOrdinals)

This commit changes the explain output for constraint checks to
say "constraint-check" rather than "fk-check" so that it can be used
for uniqueness checks in addition to foreign key checks.

Release note (sql change): The explain output for foreign key checks
is now labeled constraint-check rather than fk-check. This change
is in preparation for adding support for unique constraint checks,
which will use the same label.
This commit fixes the violation error for unique constraints to match
the message used by Postres. Previously, the error would have been
printed as:

duplicate key value (k)=(1) violates unique constraint "primary"

Now this message is printed as:

duplicate key value violates unique constraint "primary"
DETAIL: Key (k)=(1) already exists\.

Release note (sql change): The error message for unique constraint
violations now matches the error used by Postgres. For example,
the new error message looks like this:
ERROR:  duplicate key value violates unique constraint "primary"
DETAIL: Key (k)=(1) already exists.
Copy link
Collaborator Author

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner)


pkg/sql/logictest/testdata/logic_test/unique, line 133 at r3 (raw file):

Previously, RaduBerinde wrote…

rowsort (+ more instances below)

Done.


pkg/sql/opt/exec/execbuilder/mutation.go, line 784 at r3 (raw file):

Previously, RaduBerinde wrote…

[nit] could use a function comment (again, reminding the reader that this only concerns "unique without index")

Done.


pkg/sql/opt/exec/execbuilder/testdata/unique, line 83 at r3 (raw file):

Previously, RaduBerinde wrote…

[nit] please file an issue to plumb some info about the constraint (kind, name) here

Done. #58132


pkg/sql/opt/optbuilder/mutation_builder_unique.go, line 215 at r3 (raw file):

Previously, RaduBerinde wrote…

This needs a comment, why is it ok? (I assume uniqueOrdinals is always a prefix of uniqueAndPrimaryKeyOrdinals)

Done.

Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 4 files at r1, 23 of 23 files at r2, 5 of 5 files at r3, 22 of 28 files at r4, 22 of 23 files at r5, 6 of 6 files at r6.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale)

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner)

This commit updates the execbuilder to build uniqueness checks for
insert and update queries when needed. It also fixes a few other bugs
in the existing code that were preventing the checks from being built.
As of this commit, uniqueness checks for insert and update now work
end-to-end. This commit also includes a number of execbuilder and logic
tests for uniqueness checks.

There is no release note since these checks are still gated behind the
experimental_enable_unique_without_index_constraints session variable.

Informs cockroachdb#41535

Release note: None
Copy link
Collaborator Author

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTRs!

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @mgartner)

@craig
Copy link
Contributor

craig bot commented Dec 21, 2020

Build succeeded:

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.

4 participants