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.

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

schemadiff: fix missing DROP CONSTRAINT in duplicate/redundant constraints scenario. #14387

Merged
merged 5 commits into from
Oct 29, 2023
Merged

schemadiff: fix missing DROP CONSTRAINT in duplicate/redundant constraints scenario. #14387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8736b50
Testing that duplicate FK constraints are still treated as distinct
shlomi-noach Oct 29, 2023
1370607
adding schema-scope test
shlomi-noach Oct 29, 2023
cb89a48
adding more tests, with different constraint hints
shlomi-noach Oct 29, 2023
024e542
more tests
shlomi-noach Oct 29, 2023
af629cd
fix logic: count number of constraint appearances for sake of 'DROP C…
shlomi-noach Oct 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions go/vt/schemadiff/schema_diff_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -741,6 +741,20 @@ func TestSchemaDiff(t *testing.T) {
sequential: true,
conflictingDiffs: 2,
},
{
name: "two identical foreign keys in table, drop one",
fromQueries: []string{
"create table parent (id int primary key)",
"create table t1 (id int primary key, i int, key i_idex (i), constraint f1 foreign key (i) references parent(id), constraint f2 foreign key (i) references parent(id))",
},
toQueries: []string{
"create table parent (id int primary key)",
"create table t1 (id int primary key, i int, key i_idex (i), constraint f1 foreign key (i) references parent(id))",
},
expectDiffs: 1,
expectDeps: 0,
entityOrder: []string{"t1"},
},
}
hints := &DiffHints{RangeRotationStrategy: RangeRotationDistinctStatements}
for _, tc := range tt {
Expand Down
17 changes: 15 additions & 2 deletions go/vt/schemadiff/table.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1302,11 +1302,14 @@ func (c *CreateTableEntity) diffConstraints(alterTable *sqlparser.AlterTable,
}
t1ConstraintsMap := map[string]*sqlparser.ConstraintDefinition{}
t2ConstraintsMap := map[string]*sqlparser.ConstraintDefinition{}
t2ConstraintsCountMap := map[string]int{}
for _, constraint := range t1Constraints {
t1ConstraintsMap[normalizeConstraintName(t1Name, constraint)] = constraint
}
for _, constraint := range t2Constraints {
t2ConstraintsMap[normalizeConstraintName(t2Name, constraint)] = constraint
constraintName := normalizeConstraintName(t2Name, constraint)
t2ConstraintsMap[constraintName] = constraint
t2ConstraintsCountMap[constraintName]++
}

dropConstraintStatement := func(constraint *sqlparser.ConstraintDefinition) *sqlparser.DropKey {
Expand All @@ -1319,12 +1322,22 @@ func (c *CreateTableEntity) diffConstraints(alterTable *sqlparser.AlterTable,
// evaluate dropped constraints
//
for _, t1Constraint := range t1Constraints {
if _, ok := t2ConstraintsMap[normalizeConstraintName(t1Name, t1Constraint)]; !ok {
// Due to how we normalize the constraint string (e.g. in ConstraintNamesIgnoreAll we
// completely discard the constraint name), it's possible to have multiple constraints under
// the same string. Effectively, this means the schema design has duplicate/redundant constraints,
// which of course is poor design -- but still valid.
// To deal with dropping constraints, we need to not only account for the _existence_ of a constraint,
// but also to _how many times_ it appears.
constraintName := normalizeConstraintName(t1Name, t1Constraint)
if t2ConstraintsCountMap[constraintName] == 0 {
// constraint exists in t1 but not in t2, hence it is dropped
dropConstraint := dropConstraintStatement(t1Constraint)
alterTable.AlterOptions = append(alterTable.AlterOptions, dropConstraint)
} else {
t2ConstraintsCountMap[constraintName]--
}
}
// t2ConstraintsCountMap should not be used henceforth.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make it nil here then as well so that it cannot be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. So sorry to have merged this before noticing this comment. I'll address this elsewhere. I just happen to have #14373 as an open PR which I'll piggyride.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha. The linter doesn't like it. If I add t2ConstraintsCountMap = nil I get:

go/vt/schemadiff/table.go:1341:2: ineffectual assignment to t2ConstraintsCountMap (ineffassign)
	t2ConstraintsCountMap = nil

Because nothing does use the map. The linter is sometimes overzealous.

Copy link
Contributor Author

@shlomi-noach shlomi-noach Oct 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@mattlord how do you feel about adding a new scope block and a copy of the map, like so:

	{
		t2ConstraintsCountMapCopy := maps.Clone(t2ConstraintsCountMap)
		for _, t1Constraint := range t1Constraints {
			// Due to how we normalize the constraint string (e.g. in ConstraintNamesIgnoreAll we
			// completely discard the constraint name), it's possible to have multiple constraints under
			// the same string. Effectively, this means the schema design has duplicate/redundant constraints,
			// which of course is poor design -- but still valid.
			// To deal with dropping constraints, we need to not only account for the _existence_ of a constraint,
			// but also to _how many times_ it appears.
			constraintName := normalizeConstraintName(t1Name, t1Constraint)
			if t2ConstraintsCountMapCopy[constraintName] == 0 {
				// constraint exists in t1 but not in t2, hence it is dropped
				dropConstraint := dropConstraintStatement(t1Constraint)
				alterTable.AlterOptions = append(alterTable.AlterOptions, dropConstraint)
			} else {
				t2ConstraintsCountMapCopy[constraintName]--
			}
		}
	}

?


for _, t2Constraint := range t2Constraints {
normalizedT2ConstraintName := normalizeConstraintName(t2Name, t2Constraint)
Expand Down
39 changes: 39 additions & 0 deletions go/vt/schemadiff/table_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,6 +607,14 @@ func TestCreateTableDiff(t *testing.T) {
cdiff: "ALTER TABLE `t1` DROP CHECK `check3`",
constraint: ConstraintNamesIgnoreAll,
},
{
name: "check constraints, remove duplicate",
from: "create table t1 (id int primary key, i int, constraint `chk_123abc` CHECK ((`i` > 2)), constraint `check3` CHECK ((`i` > 2)), constraint `chk_789def` CHECK ((`i` < 5)))",
to: "create table t2 (id int primary key, i int, constraint `chk_123abc` CHECK ((`i` > 2)), constraint `chk_789def` CHECK ((`i` < 5)))",
diff: "alter table t1 drop check check3",
cdiff: "ALTER TABLE `t1` DROP CHECK `check3`",
constraint: ConstraintNamesIgnoreAll,
},
{
name: "check constraints, remove, ignore vitess, no match",
from: "create table t1 (id int primary key, i int, constraint `chk_123abc` CHECK ((`i` > 2)), constraint `check3` CHECK ((`i` != 3)), constraint `chk_789def` CHECK ((`i` < 5)))",
Expand Down Expand Up @@ -658,6 +666,37 @@ func TestCreateTableDiff(t *testing.T) {
from: "create table t1 (id int primary key, i int, constraint f foreign key (i) references parent(id) on delete cascade)",
to: "create table t2 (id int primary key, i int, constraint f foreign key (i) references parent(id) on delete cascade)",
},
{
name: "two identical foreign keys, dropping one",
from: "create table t1 (id int primary key, i int, key i_idex (i), constraint f1 foreign key (i) references parent(id), constraint f2 foreign key (i) references parent(id))",
to: "create table t2 (id int primary key, i int, key i_idex (i), constraint f1 foreign key (i) references parent(id))",
diff: "alter table t1 drop foreign key f2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming we'll always drop the second one that we parse from left to right and this won't be flaky.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point, and yes this is consistent, but let me check what makes it consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK the order is left-to-right (order of definition), and not map iteration order. So it is indeed consistent.

cdiff: "ALTER TABLE `t1` DROP FOREIGN KEY `f2`",
},
{
name: "two identical foreign keys, dropping one, ignore vitess names",
from: "create table t1 (id int primary key, i int, key i_idex (i), constraint f1 foreign key (i) references parent(id), constraint f2 foreign key (i) references parent(id))",
to: "create table t2 (id int primary key, i int, key i_idex (i), constraint f1 foreign key (i) references parent(id))",
diff: "alter table t1 drop foreign key f2",
cdiff: "ALTER TABLE `t1` DROP FOREIGN KEY `f2`",
constraint: ConstraintNamesIgnoreVitess,
},
{
name: "two identical foreign keys, dropping one, ignore all names",
from: "create table t1 (id int primary key, i int, key i_idex (i), constraint f1 foreign key (i) references parent(id), constraint f2 foreign key (i) references parent(id))",
to: "create table t2 (id int primary key, i int, key i_idex (i), constraint f1 foreign key (i) references parent(id))",
diff: "alter table t1 drop foreign key f2",
cdiff: "ALTER TABLE `t1` DROP FOREIGN KEY `f2`",
constraint: ConstraintNamesIgnoreAll,
},
{
name: "add two identical foreign key constraints, ignore all names",
from: "create table t1 (id int primary key, i int, key i_idex (i))",
to: "create table t2 (id int primary key, i int, key i_idex (i), constraint f1 foreign key (i) references parent(id), constraint f2 foreign key (i) references parent(id))",
diff: "alter table t1 add constraint f1 foreign key (i) references parent (id), add constraint f2 foreign key (i) references parent (id)",
cdiff: "ALTER TABLE `t1` ADD CONSTRAINT `f1` FOREIGN KEY (`i`) REFERENCES `parent` (`id`), ADD CONSTRAINT `f2` FOREIGN KEY (`i`) REFERENCES `parent` (`id`)",
constraint: ConstraintNamesIgnoreAll,
},
{
name: "implicit foreign key indexes",
from: "create table t1 (id int primary key, i int, key f(i), constraint f foreign key (i) references parent(id) on delete cascade)",
Expand Down
Loading