From 8736b507dfb4a35aaf407786ee72081d1fba718a Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 29 Oct 2023 08:16:36 +0200 Subject: [PATCH 1/5] Testing that duplicate FK constraints are still treated as distinct Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/table_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/go/vt/schemadiff/table_test.go b/go/vt/schemadiff/table_test.go index bb4ad0571da..7f5bf296b42 100644 --- a/go/vt/schemadiff/table_test.go +++ b/go/vt/schemadiff/table_test.go @@ -658,6 +658,13 @@ 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", + cdiff: "ALTER TABLE `t1` DROP FOREIGN KEY `f2`", + }, { 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)", From 1370607583a7ad9c2c43e1f9ac4cca4d43822ce7 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 29 Oct 2023 09:30:09 +0200 Subject: [PATCH 2/5] adding schema-scope test Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/schema_diff_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/go/vt/schemadiff/schema_diff_test.go b/go/vt/schemadiff/schema_diff_test.go index 03d7c3e2766..d835177c795 100644 --- a/go/vt/schemadiff/schema_diff_test.go +++ b/go/vt/schemadiff/schema_diff_test.go @@ -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 { From cb89a48110ea8b7d8897a3823ad8596adae376c4 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 29 Oct 2023 09:39:02 +0200 Subject: [PATCH 3/5] adding more tests, with different constraint hints Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/table_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/go/vt/schemadiff/table_test.go b/go/vt/schemadiff/table_test.go index 7f5bf296b42..a0982225e84 100644 --- a/go/vt/schemadiff/table_test.go +++ b/go/vt/schemadiff/table_test.go @@ -665,6 +665,22 @@ func TestCreateTableDiff(t *testing.T) { diff: "alter table t1 drop foreign key f2", 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: "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)", From 024e5423ba56bdaf21e5602263c2dd3a8f5ccf49 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 29 Oct 2023 10:27:42 +0200 Subject: [PATCH 4/5] more tests Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/table_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/go/vt/schemadiff/table_test.go b/go/vt/schemadiff/table_test.go index a0982225e84..d3df8274fee 100644 --- a/go/vt/schemadiff/table_test.go +++ b/go/vt/schemadiff/table_test.go @@ -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)))", @@ -681,6 +689,14 @@ func TestCreateTableDiff(t *testing.T) { 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)", From af629cd254fd150d250d717eeb3574794f780be2 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 29 Oct 2023 10:28:07 +0200 Subject: [PATCH 5/5] fix logic: count number of constraint appearances for sake of 'DROP CONSTRAINT' Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/table.go | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/go/vt/schemadiff/table.go b/go/vt/schemadiff/table.go index 73178ee31fe..3f256889721 100644 --- a/go/vt/schemadiff/table.go +++ b/go/vt/schemadiff/table.go @@ -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 { @@ -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. for _, t2Constraint := range t2Constraints { normalizedT2ConstraintName := normalizeConstraintName(t2Name, t2Constraint)