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: ALTER TABLE ... ADD CONSTRAINT should validate existing rows #33550

Closed
thoszhang opened this issue Jan 7, 2019 · 1 comment
Closed

sql: ALTER TABLE ... ADD CONSTRAINT should validate existing rows #33550

thoszhang opened this issue Jan 7, 2019 · 1 comment
Assignees

Comments

@thoszhang
Copy link
Contributor

Currently, check constraints and FK constraints are added in the Unvalidated state and not assumed to hold for existing rows in the table. Constraints can be validated later using VALIDATE CONSTRAINT. The preferable behavior would be to validate the new constraint on existing rows (asynchronously, after the transaction has committed). This would match the default behavior in Postgres, which also validates the existing rows up front (unless otherwise specified, using NOT VALID).

#32504 (not yet merged) implements this change for adding a check constraint. The plan is to handle FK constraints similarly.

@thoszhang thoszhang self-assigned this Jan 7, 2019
thoszhang pushed a commit to eriktrinh/cockroach that referenced this issue Jan 9, 2019
This change moves check constraint adds and drops through the schema
changer when the transaction commits, moving the constraint through the
same intermediate states as when index/columns are added or dropped. The
only small differences are:
 - Constraint adds can start in write-only and immediately start being
   enforced if all used columns are in write-only or public.
 - Constraint drops can move immediately from public to absent if they
   have not yet been validated.

Therefore, the check constraint is no longer immediately public when it
is added, but allows data validation of the constraint to be performed
when the constraint is added (and is now the default behaviour).
Constraints can now also be added on columns in the process of being
added.

This change also ensures that there are no data anomalies in either
versions of the schema when dropping a validated check constraint, as
previously the transition moved the constraint from public -> absent.
Writes on the new version were not being checked even though nodes on an
older version expect all rows to conform to the constraint.

Fixes cockroachdb#33593.
See also cockroachdb#33550.

Release note (sql change): Check constraint adds by default will
validate table data with the added constraint asynchronously after the
transaction commits.
thoszhang pushed a commit to eriktrinh/cockroach that referenced this issue Jan 10, 2019
This change moves check constraint adds and drops through the schema
changer when the transaction commits, moving the constraint through the
same intermediate states as when index/columns are added or dropped. The
only small differences are:
 - Constraint adds can start in write-only and immediately start being
   enforced if all used columns are in write-only or public.
 - Constraint drops can move immediately from public to absent if they
   have not yet been validated.

Therefore, the check constraint is no longer immediately public when it
is added, but allows data validation of the constraint to be performed
when the constraint is added (and is now the default behaviour).
Constraints can now also be added on columns in the process of being
added.

This change also ensures that there are no data anomalies in either
versions of the schema when dropping a validated check constraint, as
previously the transition moved the constraint from public -> absent.
Writes on the new version were not being checked even though nodes on an
older version expect all rows to conform to the constraint.

Fixes cockroachdb#33593.
See also cockroachdb#33550.

Release note (sql change): Check constraint adds by default will
validate table data with the added constraint asynchronously after the
transaction commits.
@thoszhang
Copy link
Contributor Author

Closing in favor of #34238 which is tracking all related work.

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

No branches or pull requests

1 participant