From 254acfa31fe521d68cf70f07dea0bf1912a02b13 Mon Sep 17 00:00:00 2001 From: "vitess-bot[bot]" <108069721+vitess-bot[bot]@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 670e84c6f1a..f6477c1885f 100644 --- a/go/vt/schemadiff/schema_diff_test.go +++ b/go/vt/schemadiff/schema_diff_test.go @@ -649,6 +649,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 dbc01ec315c..b24184fe487 100644 --- a/go/vt/schemadiff/table.go +++ b/go/vt/schemadiff/table.go @@ -1285,11 +1285,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 { @@ -1302,12 +1305,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 633fdc9a5d6..4d41d9584c0 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)",