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: disallow partial unique constraints in ALTER TABLE #68403

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Aug 3, 2021

Previously, a WHERE clause in ALTER TABLE .. ADD CONSTRAINT .. UNIQUE
statements was parsed successfully but ignored so that the resulting
unique constraint was not partial. This commit disallows the WHERE
clause. Postgres allows partial unique indexes, but not partial unique
constraints, so we will likely remove support for partial unique
constraints in CREATE TABLE statements in an upcoming major release.
See discussion in #65825.

Fixes #67234

Release note (bug fix/sql change): A bug was identified that created
non-partial unique constraints when a user attempted to create a partial
unique constraint in ALTER TABLE statements. Creating partial unique
constraints in ALTER TABLE is now disallowed. A partial unique index
can be created instead with CREATE UNIQUE INDEX .. WHERE.

@mgartner mgartner requested review from ajwerner, michae2 and a team August 3, 2021 22:46
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mgartner mgartner force-pushed the disallow-predicate-in-alter-table-unique branch from 3ee72a3 to f510f19 Compare August 3, 2021 22:46
Previously, a `WHERE` clause in `ALTER TABLE .. ADD CONSTRAINT .. UNIQUE`
statements was parsed successfully but ignored so that the resulting
unique constraint was not partial. This commit disallows the `WHERE`
clause. Postgres allows partial unique indexes, but not partial unique
constraints, so we will likely remove support for partial unique
constraints in `CREATE TABLE` statements in an upcoming major release.
See discussion in cockroachdb#65825.

Fixes cockroachdb#67234

Release note (bug fix/sql change): A bug was identified that created
non-partial unique constraints when a user attempted to create a partial
unique constraint in `ALTER TABLE` statements. Creating partial unique
constraints in `ALTER TABLE` is now disallowed. A partial unique index
can be created instead with `CREATE UNIQUE INDEX .. WHERE`.
@mgartner mgartner force-pushed the disallow-predicate-in-alter-table-unique branch from f510f19 to a50bbb6 Compare August 6, 2021 23:30
Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

I'm cool with this but we should get @vy-ton to sign off. The other option is to fix it, issue a NOTICE that we may remove it, and then treat it as though it was a CREATE INDEX. The fact that we support partial UNIQUE WITHOUT INDEX makes the whole thing messy.

Reviewed 2 of 3 files at r1, 1 of 2 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @michae2)


pkg/sql/logictest/testdata/logic_test/alter_table, line 1488 at r2 (raw file):

# Partial predicates are allowed for UNIQUE WITHOUT INDEX, but not UNIQUE.
# TODO(mgartner): This is confusing, and something we should think more about
# if we ever make UNIQUE WITHOUT INDEX available by default.

Do we like this?

Copy link
Contributor

@vy-ton vy-ton left a comment

Choose a reason for hiding this comment

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

I'm inclined to leave CREATE TABLE as is until we determine our long term plan for unique index vs constraint

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

Copy link
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

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

:lgtm: but I defer to @ajwerner and @vy-ton

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

@mgartner
Copy link
Collaborator Author

mgartner commented Aug 9, 2021

I'm inclined to leave CREATE TABLE as is until we determine our long term plan for unique index vs constraint

That's fair. So should we just allow predicates it in ALTER TABLE for now then and call it a day?

@ajwerner
Copy link
Contributor

ajwerner commented Aug 9, 2021

I'm inclined to leave CREATE TABLE as is until we determine our long term plan for unique index vs constraint

That's fair. So should we just allow predicates it in ALTER TABLE for now then and call it a day?

Yeah. I think there's a question of whether we should mark the resultant index as created_explicitly or not.

@mgartner
Copy link
Collaborator Author

Closing in favor of #68629.

@mgartner mgartner closed this Aug 10, 2021
@mgartner mgartner deleted the disallow-predicate-in-alter-table-unique branch August 10, 2021 00:54
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.

sql: predicate ignored when adding unique constraint
5 participants