schemachanger: Implement `ADD CONSTRAINT NOT VALID` #96243

Xiang-Gu · 2023-01-30T22:24:23Z

This PR implements the following three statements in the declarative schema changer:

ALTER TABLE ... ADD CHECK ... NOT VALID
ALTER TABLE ... ADD UNIQUE WITHOUT INDEX ... NOT VALID
ALTER TABLE ... ADD FOREIGN KEY ... NOT VALID

The idea is to introduce a new element for each statement. That element is like simple
dependent and will transition between ABSENT and PUBLIC directly.

Epic: None

cockroach-teamcity · 2023-01-30T22:24:35Z

This change is

Xiang-Gu · 2023-02-02T17:41:10Z

This is ready for a look!

fqazi

Reviewed 35 of 35 files at r1, 24 of 24 files at r2, 24 of 24 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Xiang-Gu)

-- commits line 15 at r1:
*filters

pkg/sql/schemachanger/scexec/scmutationexec/constraint.go line 89 at r1 (raw file):

}

func (i *immediateVisitor) MakeAbsentCheckConstraintNotValidPublic(

Nity: Maybe call these unvalidated

Code quote:

MakeAbsentCheckConstraintNotValidPublic

pkg/sql/schemachanger/scpb/elements.proto line 79 at r2 (raw file):

  RowLevelTTL row_level_ttl = 29 [(gogoproto.customname) = "RowLevelTTL", (gogoproto.moretags) = "parent:\"Table\""];
  CheckConstraintNotValid check_constraint_not_valid = 170 [(gogoproto.moretags) = "parent:\"Table\""];
  UniqueWithoutIndexConstraintNotValid unique_without_index_constraint_not_valid = 171 [(gogoproto.moretags) = "parent:\"Table\""];

I guess for VALIDATE CONSTRAINT command...we are going to drop and add elements? Trying to reason about how that transformation will work in our current model, since we don't want a limited number of operations

pkg/sql/schemachanger/scplan/internal/opgen/opgen_foreign_key_constraint_not_valid.go line 41 at r3 (raw file):

			to(scpb.Status_ABSENT,
				// TODO(postamar): remove revertibility constraint when possible
				revertible(false),

Should this be considered revertible?

pkg/sql/schemachanger/scplan/internal/rules/current/dep_drop_constraint.go line 46 at r1 (raw file):

			return rel.Clauses{
				from.TypeFilter(rulesVersionKey, isConstraintDependent),
				to.TypeFilter(rulesVersionKey, isNonIndexBackedComplexConstraint),

Can we share these with the Or operation?

postamar · 2023-02-03T12:31:20Z

I guess for VALIDATE CONSTRAINT command...we are going to drop and add elements? Trying to reason about how that transformation will work in our current model, since we don't want a limited number of operations

Yes. The elements' terminal state outside of a schema change has to be either ABSENT or PUBLIC. We can't re-use the CheckConstraint element and leave it in WRITE_ONLY. So, VALIDATE CONSTRAINT takes down the NotValid constraint element in favour of its vanilla equivalent.

postamar

This is close. I'm very pleased to see that the impact on the rules is very benign. The main theme of my comments is around tightening things:

tighten the ops by re-using the existing ones
tighten the type predicates used in the rules

The test failures seem unconcerning except for that backup test failure. I suspect that you need to add some more descriptor ID rewrite support in pkg/sql/catalog/rewrite.

I won't be around to witness it happening but I hope VALIDATE CONSTRAINT to be equally painless.

postamar · 2023-02-03T12:37:10Z

pkg/sql/schemachanger/scexec/scmutationexec/constraint.go

+		ConstraintID: op.ConstraintID,
+	}
+	tbl.Checks = append(tbl.Checks, ck)
+	return nil


Can you instead repurpose the MakeAbsentCheckConstraintPublic op to accept an extra Validity field? These ops are the same otherwise. Same comment for other NotValid ops.

postamar · 2023-02-03T12:39:29Z

pkg/sql/schemachanger/scpb/elements.proto

@@ -75,6 +75,9 @@ message ElementProto {
  ForeignKeyConstraint foreign_key_constraint = 27 [(gogoproto.moretags) = "parent:\"Table\""];
  TableComment table_comment = 28 [(gogoproto.moretags) = "parent:\"Table, View, Sequence\""];
  RowLevelTTL row_level_ttl = 29 [(gogoproto.customname) = "RowLevelTTL", (gogoproto.moretags) = "parent:\"Table\""];
+  CheckConstraintNotValid check_constraint_not_valid = 170 [(gogoproto.moretags) = "parent:\"Table\""];
+  UniqueWithoutIndexConstraintNotValid unique_without_index_constraint_not_valid = 171 [(gogoproto.moretags) = "parent:\"Table\""];
+  ForeignKeyConstraintNotValid foreign_key_constraint_not_valid = 172 [(gogoproto.moretags) = "parent:\"Table\""];


nit: Can you list these next to their vanilla equivalents? This is slightly harder to read.

postamar · 2023-02-03T12:45:30Z

pkg/sql/schemachanger/scpb/elements.proto

+  repeated uint32 column_ids = 3 [(gogoproto.customname) = "ColumnIDs", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.ColumnID"];
+  // Predicate, if non-nil, means a partial uniqueness constraint.
+  Expression predicate = 4 [(gogoproto.customname) = "Predicate"];
+}


meta-comment: I mentioned something about re-using the same ops in another, so you might expect me to do the same here and say we need to embed some common message, but no, this doesn't bother me. The reason is that I only dislike copy-pasting of code when it introduces opportunities for bugs. It's quite possible that in the future we find a bug in one of those ops, fix that op, and forget about the other very similar op which also has the same bug, the change sails through review because review tooling only shows the diff, etc... On the other hand, these element definitions are more future-proof, they're unlikely to change much over time, they're right next to each other in the file so unlikely to be overlooked, etc. The same reasoning applies for the op-gen definitions, because they don't tend to change much. If they do, we can, later on, factor the code there by introducing helper functions.

postamar · 2023-02-03T12:54:28Z

pkg/sql/schemachanger/scplan/internal/rules/current/helpers.go

+//
+// We call them "complex" constraint in contrast to simple constraint that
+// does not have an intermediate state.
+func isNonIndexBackedComplexConstraint(e scpb.Element) bool {


This name "complex" is not very meaningful. These constraints are subject to the 2-version invariant and have intermediate states. The NOT VALID constraints don't. One can distinguish the latter easily with the conjunction And(isConstraint, Not(isSubjectTo2VersionInvariant)).

When building a grammar like this try to make the existing constructs as powerful as possible and only add new ones if you can't avoid it. The more you add, the bigger the maintenance burden, and the lesser the expressivity and easiness to reason about.

postamar · 2023-02-03T13:06:09Z

pkg/sql/schemachanger/scplan/internal/rules/current/helpers.go

+// TODO (xiang): UniqueWithoutIndex and UniqueWithoutIndexNotValid should
+// also be treated as cross-descriptor constraint because its partial predicate
+// can references other descriptors.
+func isNonIndexBackedCrossDescriptorConstraint(e scpb.Element) bool {


Like I said in my previous comment, I don't think you need this either. case *scpb.CheckConstraint, *scpb.UniqueWithoutIndexConstraint, *scpb.ForeignKeyConstraint should verify isSubjectTo2VersionInvariant and the others not.

postamar · 2023-02-03T13:09:22Z

pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_not_valid

+
+
+test
+ALTER TABLE t ADD CHECK (i > 0) NOT VALID;


These end-to-end tests are quite expensive because they balloon into TestRollback and TestBackup tests. I don't think there's much value in having one for each three non-index constraint types, just pick one.

Unrelated but this test case is nice in that it sets the stage for a VALIDATE CONSTRAINT case to be appended to the file later on.

Xiang-Gu

@postamar Thanks for your prompt review.

yes, I've changed it to re-use existing operations so now each XxxUnvalidate element share the same set of operation as their vanilla counterpart;
I agree with you that I don't like "complexConstraint" filter either, so I got rid of it and instead implement it in the subjectToTwoVersionInvariant filter.
The backup failure is due to the fact that FK constraintID in the referencing table does not match that in the referencing table so we do need to grab the FK name first from the referencing table and use it to identify FKs in the referenced table.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi and @postamar)

pkg/sql/schemachanger/scexec/scmutationexec/constraint.go line 89 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Nity: Maybe call these unvalidated

Indeed that's a better name. Done!

pkg/sql/schemachanger/scexec/scmutationexec/constraint.go line 111 at r6 (raw file):

Previously, postamar (Marius Posta) wrote…

Can you instead repurpose the MakeAbsentCheckConstraintPublic op to accept an extra Validity field? These ops are the same otherwise. Same comment for other NotValid ops.

Done

pkg/sql/schemachanger/scpb/elements.proto line 79 at r2 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

I guess for VALIDATE CONSTRAINT command...we are going to drop and add elements? Trying to reason about how that transformation will work in our current model, since we don't want a limited number of operations

yes, VALIDATE CONSTRAINT is just as simple as dropping the xxxUnvalidated element and add a xxx element. We will need to add rules to ensure the xxxUnvalidated element does not get dropped until xxx element is added.

pkg/sql/schemachanger/scpb/elements.proto line 80 at r6 (raw file):

Previously, postamar (Marius Posta) wrote…

nit: Can you list these next to their vanilla equivalents? This is slightly harder to read.

done

pkg/sql/schemachanger/scpb/elements.proto line 367 at r6 (raw file):

Previously, postamar (Marius Posta) wrote…

meta-comment: I mentioned something about re-using the same ops in another, so you might expect me to do the same here and say we need to embed some common message, but no, this doesn't bother me. The reason is that I only dislike copy-pasting of code when it introduces opportunities for bugs. It's quite possible that in the future we find a bug in one of those ops, fix that op, and forget about the other very similar op which also has the same bug, the change sails through review because review tooling only shows the diff, etc... On the other hand, these element definitions are more future-proof, they're unlikely to change much over time, they're right next to each other in the file so unlikely to be overlooked, etc. The same reasoning applies for the op-gen definitions, because they don't tend to change much. If they do, we can, later on, factor the code there by introducing helper functions.

ack, thanks a lot for the advice!

pkg/sql/schemachanger/scplan/internal/rules/current/helpers.go line 218 at r6 (raw file):

Previously, postamar (Marius Posta) wrote…

This name "complex" is not very meaningful. These constraints are subject to the 2-version invariant and have intermediate states. The NOT VALID constraints don't. One can distinguish the latter easily with the conjunction And(isConstraint, Not(isSubjectTo2VersionInvariant)).

When building a grammar like this try to make the existing constructs as powerful as possible and only add new ones if you can't avoid it. The more you add, the bigger the maintenance burden, and the lesser the expressivity and easiness to reason about.

The thing is we defined isSubjectTo2VersionInvariant based on the following three predicates:

return isIndex(e) || isColumn(e) || isNonIndexBackedComplexConstraint(e)

I don't particularly like the name "complex" here either. So let's get rid of this filter and instead implement isSubjectTo2VersionInvariant as a building block:

if isIndex(e) || isColumn(e) {
    return true
}
switch e.(type) {
    case Check, FK, UWI, NotNull:
        return true
}
return false

pkg/sql/schemachanger/scplan/internal/rules/current/helpers.go line 238 at r6 (raw file):

Previously, postamar (Marius Posta) wrote…

Like I said in my previous comment, I don't think you need this either. case *scpb.CheckConstraint, *scpb.UniqueWithoutIndexConstraint, *scpb.ForeignKeyConstraint should verify isSubjectTo2VersionInvariant and the others not.

Done.

pkg/sql/schemachanger/scplan/internal/opgen/opgen_foreign_key_constraint_not_valid.go line 41 at r3 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Should this be considered revertible?

you're right: it's okay to add back an invalidated constraint shortly after it's removed. All the rows added during that time won't violate anything when this invalidated constraint is added back.

pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_not_valid line 10 at r6 (raw file):

Previously, postamar (Marius Posta) wrote…

These end-to-end tests are quite expensive because they balloon into TestRollback and TestBackup tests. I don't think there's much value in having one for each three non-index constraint types, just pick one.

Unrelated but this test case is nice in that it sets the stage for a VALIDATE CONSTRAINT case to be appended to the file later on.

alright, I'll just keep the end-to-end test for check.

Not-valid constraints are like simple dependents: they don't have any intermediate state so it can transition directly between public and absent. This nature requires us not to enforce certains rules that are enforced on the normal constraints. To that effect, we introduce the concept/partition of "simple" vs. "complex" constraints: - "complex constraint": a constraint that needs to transition through an intermediate status; - "simple constraint": a constraint that does not need to transition through an intermediate status. They are the NOT VALID version of the complex constraints. This commit also slightly modified the fitlers on constraints to be more principled.

… tests

fqazi

great work!

Reviewed 1 of 38 files at r4, 20 of 50 files at r7, 32 of 32 files at r10, 23 of 23 files at r11, 25 of 25 files at r12, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @postamar and @Xiang-Gu)

Xiang-Gu · 2023-02-06T15:53:14Z

bors r+

craig · 2023-02-06T17:47:56Z

Build succeeded:

Bazel Essential CI (Cockroach)

Xiang-Gu force-pushed the implement-add-check-not-valid branch 6 times, most recently from a6d5d35 to a1b95a1 Compare February 1, 2023 20:51

Xiang-Gu changed the title ~~WIP: schemachanger: Implement ADD CHECK NOT VALID~~ WIP: schemachanger: Implement ADD CONSTRAINT NOT VALID Feb 1, 2023

Xiang-Gu force-pushed the implement-add-check-not-valid branch 4 times, most recently from f0c2ff6 to d654981 Compare February 2, 2023 17:03

Xiang-Gu changed the title ~~WIP: schemachanger: Implement ADD CONSTRAINT NOT VALID~~ schemachanger: Implement ADD CONSTRAINT NOT VALID Feb 2, 2023

Xiang-Gu marked this pull request as ready for review February 2, 2023 17:41

Xiang-Gu requested a review from a team February 2, 2023 17:41

fqazi requested changes Feb 2, 2023

View reviewed changes

Xiang-Gu force-pushed the implement-add-check-not-valid branch from d654981 to 2bcab3c Compare February 2, 2023 23:30

postamar reviewed Feb 3, 2023

View reviewed changes

Xiang-Gu force-pushed the implement-add-check-not-valid branch from 2bcab3c to 60acc70 Compare February 3, 2023 21:51

Xiang-Gu commented Feb 3, 2023

View reviewed changes

Xiang-Gu added 3 commits February 3, 2023 16:59

schemachanger: Implement ADD UNIQUE WITHOUT INDEX NOT VALID and add…

e7d394b

… tests

schemachanger: Implement ADD FOREIGN KEY NOT VALID and add tests

b9a0471

Xiang-Gu force-pushed the implement-add-check-not-valid branch from 60acc70 to b9a0471 Compare February 3, 2023 21:59

fqazi approved these changes Feb 6, 2023

View reviewed changes

Xiang-Gu mentioned this pull request Feb 6, 2023

schemachanger: implement ALTER TABLE .. VALIDATE CONSTRAINT .. #96648

Merged

craig bot merged commit b031933 into cockroachdb:master Feb 6, 2023

Xiang-Gu deleted the implement-add-check-not-valid branch February 6, 2023 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schemachanger: Implement `ADD CONSTRAINT NOT VALID` #96243

schemachanger: Implement `ADD CONSTRAINT NOT VALID` #96243

Xiang-Gu commented Jan 30, 2023 •

edited

Loading

cockroach-teamcity commented Jan 30, 2023

Xiang-Gu commented Feb 2, 2023

fqazi left a comment

postamar commented Feb 3, 2023

postamar left a comment

postamar Feb 3, 2023

postamar Feb 3, 2023

postamar Feb 3, 2023

postamar Feb 3, 2023

postamar Feb 3, 2023

postamar Feb 3, 2023

Xiang-Gu left a comment

fqazi left a comment

Xiang-Gu commented Feb 6, 2023

craig bot commented Feb 6, 2023

schemachanger: Implement ADD CONSTRAINT NOT VALID #96243

schemachanger: Implement ADD CONSTRAINT NOT VALID #96243

Conversation

Xiang-Gu commented Jan 30, 2023 • edited Loading

cockroach-teamcity commented Jan 30, 2023

Xiang-Gu commented Feb 2, 2023

fqazi left a comment

Choose a reason for hiding this comment

postamar commented Feb 3, 2023

postamar left a comment

Choose a reason for hiding this comment

postamar Feb 3, 2023

Choose a reason for hiding this comment

postamar Feb 3, 2023

Choose a reason for hiding this comment

postamar Feb 3, 2023

Choose a reason for hiding this comment

postamar Feb 3, 2023

Choose a reason for hiding this comment

postamar Feb 3, 2023

Choose a reason for hiding this comment

postamar Feb 3, 2023

Choose a reason for hiding this comment

Xiang-Gu left a comment

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

Xiang-Gu commented Feb 6, 2023

craig bot commented Feb 6, 2023

schemachanger: Implement `ADD CONSTRAINT NOT VALID` #96243

schemachanger: Implement `ADD CONSTRAINT NOT VALID` #96243

Xiang-Gu commented Jan 30, 2023 •

edited

Loading