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.

Sign up for GitHub

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

Jump to bottom

sql: add support for NOT VALID check constraints #38382

Merged

craig merged 1 commit into cockroachdb:master from tyler314:add_constraint_not_valid

Jun 25, 2019

tyler314 commented Jun 24, 2019 •

edited

Loading

Mark constraint as unvalidated if user specifies NOT VALID in their
check constraint. Within backfill, do not add the unvalidated constraints
to the queues for validating.

Release note (sql change): Support NOT VALID for check constraints,
which supports not checking constraints for existing rows.

tyler314 requested review from thoszhang and a team

June 24, 2019 20:58

tyler314 assigned thoszhang

CLAassistant commented Jun 24, 2019 •

edited

Loading

All committers have signed the CLA.

Member

cockroach-teamcity commented Jun 24, 2019

This change is

tyler314 force-pushed the add_constraint_not_valid branch from feda258 to df20d1d Compare

June 24, 2019 21:39

dt changed the base branch from add_constraint_not_valid to master

June 24, 2019 21:44

tyler314 force-pushed the add_constraint_not_valid branch from 393a6c7 to df20d1d Compare

June 25, 2019 16:41

tyler314 requested review from a team

June 25, 2019 16:41

tyler314 changed the title ~~Add constraint not valid~~ sql: add support for NOT VALID check constraints

thoszhang reviewed

View reviewed changes

Contributor

thoszhang left a comment

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

.gitignore, line 19 at r1 (raw file):

/cockroach*
/certs
.idea/*

This shouldn't be here

pkg/sql/backfill.go, line 165 at r1 (raw file):

						constraintsToValidate = append(constraintsToValidate, *t.Constraint)
					}
				case sqlbase.ConstraintToUpdate_FOREIGN_KEY:

Can you add a TODO comment either here or in alter_table.go indicating that we don't yet support adding NOT VALID foreign keys, because we don't add those FK mutations?

pkg/sql/backfill.go, line 169 at r1 (raw file):

						constraintsToAddBeforeValidation = append(constraintsToAddBeforeValidation, *t.Constraint)
						constraintsToValidate = append(constraintsToValidate, *t.Constraint)
					}

The SET NOT NULL bug is due to the fact that we're not handling NOT NULL constraints here. This needs an extra case:

				case sqlbase.ConstraintToUpdate_NOT_NULL:
					// NOT NULL constraints are always validated before they can be added
					constraintsToAddBeforeValidation = append(constraintsToAddBeforeValidation, *t.Constraint)
					constraintsToValidate = append(constraintsToValidate, *t.Constraint)

pkg/sql/logictest/testdata/logic_test/alter_table, line 925 at r1 (raw file):

INSERT INTO t VALUES (1), (NULL)

# Error no longer occurs, EXPECTED?

This is a real failure, related to the fact that SET NOT NULL goes through much of the same path as check constraints. (See the comment in backfill.go)

pkg/sql/logictest/testdata/logic_test/alter_table, line 985 at r1 (raw file):


query I
SELECT * FROM t

The select statement isn't really necessary here.

pkg/sql/logictest/testdata/logic_test/alter_table, line 1003 at r1 (raw file):

SHOW CONSTRAINTS FROM t
----
t  check_a   CHECK  CHECK  (a < 100)  true

These tests are failing because we recently changed the formatting for check constraints in the output of SHOW CONSTRAINTS. They should have two sets of parentheses (see #38318 or earlier in this file for examples).

pkg/sql/sqlbase/structured.go, line 2388 at r1 (raw file):

					}
				default:
					return errors.AssertionFailedf("impossible constraint validity state: %d", t.Constraint.Check.Validity)

nit: this should probably say "invalid" instead of "impossible"

pkg/sql/sqlbase/structured.go, line 2424 at r1 (raw file):

}

// AddCheckMutation adds a check constraint validation mutation to desc.Mutations.

The word validation shouldn't be here anymore

tyler314 force-pushed the add_constraint_not_valid branch from df20d1d to 96dcc88 Compare

June 25, 2019 19:11

tyler314 commented

View reviewed changes

Author

tyler314 left a comment

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

.gitignore, line 19 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

This shouldn't be here

Done.

pkg/sql/backfill.go, line 165 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Can you add a TODO comment either here or in alter_table.go indicating that we don't yet support adding NOT VALID foreign keys, because we don't add those FK mutations?

Done.

pkg/sql/backfill.go, line 169 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

The SET NOT NULL bug is due to the fact that we're not handling NOT NULL constraints here. This needs an extra case:

				case sqlbase.ConstraintToUpdate_NOT_NULL:
					// NOT NULL constraints are always validated before they can be added
					constraintsToAddBeforeValidation = append(constraintsToAddBeforeValidation, *t.Constraint)
					constraintsToValidate = append(constraintsToValidate, *t.Constraint)

Done.

pkg/sql/logictest/testdata/logic_test/alter_table, line 925 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

This is a real failure, related to the fact that SET NOT NULL goes through much of the same path as check constraints. (See the comment in backfill.go)

Done.

pkg/sql/logictest/testdata/logic_test/alter_table, line 985 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

The select statement isn't really necessary here.

Done.

pkg/sql/logictest/testdata/logic_test/alter_table, line 1003 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

These tests are failing because we recently changed the formatting for check constraints in the output of SHOW CONSTRAINTS. They should have two sets of parentheses (see #38318 or earlier in this file for examples).

Done.

pkg/sql/sqlbase/structured.go, line 2424 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

The word validation shouldn't be here anymore

Done.

thoszhang approved these changes

View reviewed changes

Contributor

thoszhang left a comment

LGTM, just one typo

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

pkg/sql/backfill.go, line 165 at r2 (raw file):

						constraintsToValidate = append(constraintsToValidate, *t.Constraint)
					}
					// TODO (tyler): we do not yet support teh NOT VALID foreign keys,

s/teh/the

tyler314 force-pushed the add_constraint_not_valid branch from 96dcc88 to a5317d3 Compare

June 25, 2019 19:20

tyler314 commented

View reviewed changes

Author

tyler314 left a comment

Fixed.

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

Author

tyler314 commented Jun 25, 2019

bors r+

craig bot pushed a commit that referenced this pull request


          Merge #38377 #38382

fa68533

38377: sql: fix double-adding FK backreferences when retrying r=lucy-zhang a=lucy-zhang

Currently, `PublishMultiple()` on the lease manager, which updates multiple
table descriptors in a single transaction as part of a schema change, updates
each table descriptor independently of the others. There was a bug where if the
call to `PublishMultiple()` to add FKs and backreferences was retried (e.g., if
there was a crash after this step but before the finalization of the schema
change), we would correctly avoid re-adding the reference to the table, but the
backreferences would be incorrectly added a second time.

This change updates the interface of `PublishMultiple()`: There's now a single
update closure which has access to a map of all table descriptors being
modified. Backreferences are now only installed if the forward reference was
also installed.

Release note: None

38382: sql: add support for NOT VALID check constraints r=Tyler314 a=Tyler314

Mark constraint as unvalidated if user specifies NOT VALID in their
check constraint. Within backfill, do not add the unvalidated constraints
to the queues for validating.

Release note (sql change): Support NOT VALID for check constraints,
which supports not checking constraints for existing rows.

Co-authored-by: Lucy Zhang <[email protected]>
Co-authored-by: Tyler314 <[email protected]>

thoszhang mentioned this pull request

sql: support ADD CONSTRAINT ... NOT VALID #33551

Closed

Contributor

craig bot commented Jun 25, 2019

Build failed (retrying...)

GitHub CI (Cockroach)

Contributor

craig bot commented Jun 25, 2019

Build failed

GitHub CI (Cockroach)


          sql: add support for NOT VALID check constraints

716f0fd

Mark constraint as unvalidated if user specifies NOT VALID in their
check constraint. Within backfill, do not add the unvalidated constraints
to the queues for validating.

Release note (sql change): Support NOT VALID for check constraints,
which supports not checking constraints for existing rows.

tyler314 force-pushed the add_constraint_not_valid branch from a5317d3 to 716f0fd Compare

June 25, 2019 20:25

Author

tyler314 commented Jun 25, 2019

bors r+
ORM test seems flaky

craig bot pushed a commit that referenced this pull request


          Merge #38382

b381e11

38382: sql: add support for NOT VALID check constraints r=Tyler314 a=Tyler314

Mark constraint as unvalidated if user specifies NOT VALID in their
check constraint. Within backfill, do not add the unvalidated constraints
to the queues for validating.

Release note (sql change): Support NOT VALID for check constraints,
which supports not checking constraints for existing rows.

Co-authored-by: Tyler314 <[email protected]>

Contributor

craig bot commented Jun 25, 2019

Build succeeded

GitHub CI (Cockroach)

craig bot merged commit 716f0fd into cockroachdb:master

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

Labels

None yet