From aa1cf497d2aae9c59f4e2006932b97a45463207c Mon Sep 17 00:00:00 2001 From: "vitess-bot[bot]" <108069721+vitess-bot[bot]@users.noreply.github.com> Date: Fri, 10 Nov 2023 23:08:44 +0530 Subject: [PATCH] [release-16.0] schemadiff: fix missing `DROP CONSTRAINT` in duplicate/redundant constraints scenario. (#14387) (#14389) Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com> Co-authored-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/table.go | 17 +++++++++++++-- go/vt/schemadiff/table_test.go | 39 ++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/go/vt/schemadiff/table.go b/go/vt/schemadiff/table.go index f58faa06310..965b632bd2e 100644 --- a/go/vt/schemadiff/table.go +++ b/go/vt/schemadiff/table.go @@ -1196,11 +1196,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(constraint)] = constraint } for _, constraint := range t2Constraints { - t2ConstraintsMap[normalizeConstraintName(constraint)] = constraint + constraintName := normalizeConstraintName(constraint) + t2ConstraintsMap[constraintName] = constraint + t2ConstraintsCountMap[constraintName]++ } dropConstraintStatement := func(constraint *sqlparser.ConstraintDefinition) *sqlparser.DropKey { @@ -1213,12 +1216,22 @@ func (c *CreateTableEntity) diffConstraints(alterTable *sqlparser.AlterTable, // evaluate dropped constraints // for _, t1Constraint := range t1Constraints { - if _, ok := t2ConstraintsMap[normalizeConstraintName(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(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(t2Constraint) diff --git a/go/vt/schemadiff/table_test.go b/go/vt/schemadiff/table_test.go index 18fce4ec759..9380f678b33 100644 --- a/go/vt/schemadiff/table_test.go +++ b/go/vt/schemadiff/table_test.go @@ -569,6 +569,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)))", @@ -620,6 +628,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)",