From d717ee357aaff5c3335249667cebb16167d8dbaa Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 29 Oct 2023 22:13:44 +0200 Subject: [PATCH] schemadiff: fix missing `DROP CONSTRAINT` in duplicate/redundant constraints scenario. (#14387) Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/schema_diff_test.go | 14 ++++++++++ go/vt/schemadiff/table.go | 17 ++++++++++-- go/vt/schemadiff/table_test.go | 39 ++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 2 deletions(-) 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 { 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) diff --git a/go/vt/schemadiff/table_test.go b/go/vt/schemadiff/table_test.go index bb4ad0571da..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)))", @@ -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", + 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)",