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

Document asynchronous CHECK constraint validation #4612

Merged
merged 1 commit into from
Apr 15, 2019

Conversation

rmloveland
Copy link
Contributor

Fixes #4362.

Summary of changes:

  • Update CHECK constraint page to note that validation is now
    asynchronous.

  • Update 'Add Constraint' page to note that validation is now
    asynchronous, and to give an example of adding a CHECK constraint to a
    column added earlier in the same transaction.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rmloveland rmloveland requested a review from thoszhang April 1, 2019 17:43
@rmloveland
Copy link
Contributor Author

Direct links for easier reference:

Copy link

@thoszhang thoszhang left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @lucy-zhang and @rmloveland)


v19.1/add-constraint.md, line 60 at r1 (raw file):

~~~

<span class="version-tag">New in v19.1</span>: Check constraints begin validating table data asynchronously as soon as the transaction where the constraint was added commits.  Also, check constraints can be added to columns that were created earlier in the transaction.  For example:

It's currently not entirely clear what "validating" means in this context. We should mention that if a row is found that violates the constraint during the validation step, the ADD CONSTRAINT will fail, which is very different from the previous behavior of VALIDATE CONSTRAINT. If the column was added in the same transaction, it will also be rolled back. (Currently, there's no longer a way to add a check constraint that's enforced for writes but is potentially violated for pre-existing rows.)

This can happen if someone adds a column with a default value or a computed column, and tries to add a constraint on the column in the same transaction, but the default/computed values violate the constraint. In that case, the column won't be added.

@rmloveland rmloveland force-pushed the 20190328-async-check-constraint-validation branch from fe4dd1c to 46151f9 Compare April 3, 2019 19:37
Copy link
Contributor Author

@rmloveland rmloveland left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @lucy-zhang)


v19.1/add-constraint.md, line 60 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

It's currently not entirely clear what "validating" means in this context. We should mention that if a row is found that violates the constraint during the validation step, the ADD CONSTRAINT will fail, which is very different from the previous behavior of VALIDATE CONSTRAINT. If the column was added in the same transaction, it will also be rolled back. (Currently, there's no longer a way to add a check constraint that's enforced for writes but is potentially violated for pre-existing rows.)

This can happen if someone adds a column with a default value or a computed column, and tries to add a constraint on the column in the same transaction, but the default/computed values violate the constraint. In that case, the column won't be added.

Thanks for this additional info.

I have updated this example and the CHECK constraint page, split up across them roughly as follows:

  • CHECK was updated to include everything you mentioned above, since it's supposed to be a reference
  • This example on ADD CONSTRAINT focuses more on the "if you add a column, then a check that fails, your add column is rolled back" since that is what's happening in the example (and the user can refer to CHECK for more info)

@rmloveland
Copy link
Contributor Author

@lucy-zhang are you ok with this as it stands? I'd like to move it into the docs team review stage if you're satisfied with the updates.

@rmloveland
Copy link
Contributor Author

@lucy-zhang ping

Copy link

@thoszhang thoszhang left a comment

Choose a reason for hiding this comment

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

Sorry for the delay.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @lucy-zhang and @rmloveland)


v19.1/add-constraint.md, line 79 at r2 (raw file):

{{site.data.alerts.callout_info}}
If an existing row is found that violates the check constraint, the entire transaction will be rolled back, including any new columns that were added.

I think this could still be clearer about what "existing row" means; part of the behavior is that if any value that would have been in the new column (if the column has a default/computed value) would violate the constraint, the column will not get added.

Maybe add a sentence like "If any new column has a default value or is a computed column, and would have contained values that violate the new check constraint, then the entire transaction will also be rolled back," though that's a little clunky.


v19.1/check.md, line 11 at r2 (raw file):
For the first sentence, I actually think

If you add a CHECK constraint to an existing table, CockroachDB will run a background job to validate existing table data in the process of adding the constraint.

or something like that would be preferable here. It's a comparable level of detail to our existing docs for online schema changes, and I'm worried people might interpret the existing sentence as "the async job runs sometime after a result for ALTER TABLE is returned to the client," which isn't the case.

@rmloveland rmloveland force-pushed the 20190328-async-check-constraint-validation branch from 46151f9 to 6cd3922 Compare April 15, 2019 15:30
Copy link
Contributor Author

@rmloveland rmloveland left a comment

Choose a reason for hiding this comment

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

No problem, thanks for the good review feedback

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @lucy-zhang)


v19.1/add-constraint.md, line 79 at r2 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

I think this could still be clearer about what "existing row" means; part of the behavior is that if any value that would have been in the new column (if the column has a default/computed value) would violate the constraint, the column will not get added.

Maybe add a sentence like "If any new column has a default value or is a computed column, and would have contained values that violate the new check constraint, then the entire transaction will also be rolled back," though that's a little clunky.

Thanks. I tried rewriting it as a bulleted list of cases (existing, new) based on your comment. PTAL.


v19.1/check.md, line 11 at r2 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

For the first sentence, I actually think

If you add a CHECK constraint to an existing table, CockroachDB will run a background job to validate existing table data in the process of adding the constraint.

or something like that would be preferable here. It's a comparable level of detail to our existing docs for online schema changes, and I'm worried people might interpret the existing sentence as "the async job runs sometime after a result for ALTER TABLE is returned to the client," which isn't the case.

Thanks for explaining that. I think I got a little turned around by trying to rephrase one of your commit messages and ended up at the wrong level of detail.

I swapped out the old sentence for yours. It sounds/looks much better to my ear/eye as well.

@rmloveland rmloveland requested a review from jseldess April 15, 2019 17:26
Copy link
Contributor

@jseldess jseldess left a comment

Choose a reason for hiding this comment

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

:lgtm: with a few small nits

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jseldess, @lucy-zhang, and @rmloveland)


v19.1/add-constraint.md, line 60 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Thanks for this additional info.

I have updated this example and the CHECK constraint page, split up across them roughly as follows:

  • CHECK was updated to include everything you mentioned above, since it's supposed to be a reference
  • This example on ADD CONSTRAINT focuses more on the "if you add a column, then a check that fails, your add column is rolled back" since that is what's happening in the example (and the user can refer to CHECK for more info)

nit: Remove extra space after period.


v19.1/check.md, line 12 at r3 (raw file):

- <span class="version-tag">New in v19.1</span>: If you add a `CHECK` constraint to an existing table, CockroachDB will run a background job to validate existing table data in the process of adding the constraint. If a row is found that violates the constraint during the validation step, the [`ADD CONSTRAINT`](add-constraint.html) statement will fail. This differs from previous versions of CockroachDB, which allowed you to add a check constraint that was enforced for writes but could be violated by rows that existed prior to adding the constraint.
- <span class="version-tag">New in v19.1</span>: Check constraints can be added to columns that were created earlier in the same transaction.  For an example, see [Add the `CHECK` constraint](add-constraint.html#add-the-check-constraint).

nit: Remove extra space after period.

Fixes #4362.

Summary of changes:

- Update `CHECK` constraint page to note that validation is now
  asynchronous.

- Update 'Add Constraint' page to note that validation is now
  asynchronous, and to give an example of adding a CHECK constraint to a
  column added earlier in the same transaction.
@rmloveland rmloveland force-pushed the 20190328-async-check-constraint-validation branch from 6cd3922 to 6da2683 Compare April 15, 2019 18:51
Copy link
Contributor Author

@rmloveland rmloveland left a comment

Choose a reason for hiding this comment

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

Thanks for the review Jesse!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jseldess and @lucy-zhang)


v19.1/add-constraint.md, line 60 at r1 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

nit: Remove extra space after period.

Fixed.


v19.1/check.md, line 12 at r3 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

nit: Remove extra space after period.

Fixed.

@rmloveland rmloveland merged commit 693a85e into master Apr 15, 2019
@rmloveland rmloveland deleted the 20190328-async-check-constraint-validation branch April 15, 2019 18:55
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: run check constraint mutations and validation by schema changer
4 participants