Skip to content

Commit

Permalink
have generated index and foreign key names match mysql (#2438)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jycor
jycor committed Apr 9, 2024
1 parent 6b139de commit bfde35b
Show file tree
Hide file tree
Showing 5 changed files with 277 additions and 9 deletions.
202 changes: 201 additions & 1 deletion enginetest/queries/foreign_key_queries.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2282,7 +2282,7 @@ var ForeignKeyTests = []ScriptTest{
},
},
{
Name: "rename foreign key constraints",
Name: "rename check constraints",
SetUpScript: []string{
"create table t (i int, constraint `mychk` check (i > 0))",
},
Expand All @@ -2297,6 +2297,206 @@ var ForeignKeyTests = []ScriptTest{
},
},
},
{
Name: "foreign key naming",
SetUpScript: []string{
"create table theparent (i int primary key);",
"create table child1 (j int primary key);",
"create table child2 (j int primary key);",
"create table child3 (j int primary key);",
"create table child4 (j int primary key);",
"create table child5 (j int primary key);",
"create table child6 (j int primary key);",
"create table child7 (j int primary key);",
},
Assertions: []ScriptTestAssertion{
{
Query: "alter table child1 add foreign key (j) references theparent (i);",
Expected: []sql.Row{
{types.NewOkResult(0)},
},
},
{
Query: "alter table child1 add foreign key (j) references theparent (i);",
Expected: []sql.Row{
{types.NewOkResult(0)},
},
},
{
Query: "alter table child1 add foreign key (j) references theparent (i);",
Expected: []sql.Row{
{types.NewOkResult(0)},
},
},
{
Query: "show create table child1;",
Expected: []sql.Row{
{"child1", "CREATE TABLE `child1` (\n" +
" `j` int NOT NULL,\n" +
" PRIMARY KEY (`j`),\n" +
" CONSTRAINT `child1_ibfk_1` FOREIGN KEY (`j`) REFERENCES `theparent` (`i`),\n" +
" CONSTRAINT `child1_ibfk_2` FOREIGN KEY (`j`) REFERENCES `theparent` (`i`),\n" +
" CONSTRAINT `child1_ibfk_3` FOREIGN KEY (`j`) REFERENCES `theparent` (`i`)\n" +
") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"},
},
},

{
Query: "alter table child2 add constraint `child2_ibfk_1` foreign key (j) references theparent (i);",
Expected: []sql.Row{
{types.NewOkResult(0)},
},
},
{
// If generated name collides with existing, then a new name will be generated
Query: "alter table child2 add foreign key (j) references theparent (i);",
Expected: []sql.Row{
{types.NewOkResult(0)},
},
},
{
Query: "show create table child2;",
Expected: []sql.Row{
{"child2", "CREATE TABLE `child2` (\n" +
" `j` int NOT NULL,\n" +
" PRIMARY KEY (`j`),\n" +
" CONSTRAINT `child2_ibfk_1` FOREIGN KEY (`j`) REFERENCES `theparent` (`i`),\n" +
" CONSTRAINT `child2_ibfk_2` FOREIGN KEY (`j`) REFERENCES `theparent` (`i`)\n" +
") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"},
},
},
{
// explicit name collisions still error
Query: "alter table child2 add constraint `child2_ibfk_2` foreign key (j) references theparent (i);",
ExpectedErr: sql.ErrForeignKeyDuplicateName,
},

{
// unlike secondary index naming, constraints will find highest existing index and increment from there
Query: "alter table child3 add constraint `child3_ibfk_100` foreign key (j) references theparent (i);",
Expected: []sql.Row{
{types.NewOkResult(0)},
},
},
{
Query: "alter table child3 add foreign key (j) references theparent (i);",
Expected: []sql.Row{
{types.NewOkResult(0)},
},
},
{
Query: "show create table child3;",
Expected: []sql.Row{
{"child3", "CREATE TABLE `child3` (\n" +
" `j` int NOT NULL,\n" +
" PRIMARY KEY (`j`),\n" +
" CONSTRAINT `child3_ibfk_100` FOREIGN KEY (`j`) REFERENCES `theparent` (`i`),\n" +
" CONSTRAINT `child3_ibfk_101` FOREIGN KEY (`j`) REFERENCES `theparent` (`i`)\n" +
") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"},
},
},

{
// Name generation is case-sensitive
Query: "alter table child4 add constraint `CHILD4_IBFK_1` foreign key (j) references theparent (i);",
Expected: []sql.Row{
{types.NewOkResult(0)},
},
},
{
// the name collision check is case-insensitive
Query: "alter table child4 add foreign key (j) references theparent (i);",
ExpectedErr: sql.ErrForeignKeyDuplicateName,
},
{
Query: "show create table child4;",
Expected: []sql.Row{
{"child4", "CREATE TABLE `child4` (\n" +
" `j` int NOT NULL,\n" +
" PRIMARY KEY (`j`),\n" +
" CONSTRAINT `CHILD4_IBFK_1` FOREIGN KEY (`j`) REFERENCES `theparent` (`i`)\n" +
") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"},
},
},

{
Query: "alter table child5 add constraint `child5_ibfk_-2` foreign key (j) references theparent (i);",
Expected: []sql.Row{
{types.NewOkResult(0)},
},
},
{
// This adds -1, which is interpreted as 4294967295
Query: "alter table child5 add foreign key (j) references theparent (i);",
Expected: []sql.Row{
{types.NewOkResult(0)},
},
},
{
// This adds 4294967296, which overflows back to 0
Query: "alter table child5 add foreign key (j) references theparent (i);",
Expected: []sql.Row{
{types.NewOkResult(0)},
},
},
{
// This attempts to add 4294967296 again, which throws a duplicate name error
Query: "alter table child5 add foreign key (j) references theparent (i);",
ExpectedErr: sql.ErrForeignKeyDuplicateName,
},
{
// foreign keys are sorted by name
Query: "show create table child5;",
Expected: []sql.Row{
{"child5", "CREATE TABLE `child5` (\n" +
" `j` int NOT NULL,\n" +
" PRIMARY KEY (`j`),\n" +
" CONSTRAINT `child5_ibfk_-2` FOREIGN KEY (`j`) REFERENCES `theparent` (`i`),\n" +
" CONSTRAINT `child5_ibfk_0` FOREIGN KEY (`j`) REFERENCES `theparent` (`i`),\n" +
" CONSTRAINT `child5_ibfk_4294967295` FOREIGN KEY (`j`) REFERENCES `theparent` (`i`)\n" +
") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"},
},
},

// empty string constraint names are allowed if specified explicitly
{
Query: "alter table child6 add constraint `` foreign key (j) references theparent (i);",
Expected: []sql.Row{
{types.NewOkResult(0)},
},
},
{
Query: "alter table child6 add foreign key (j) references theparent (i);",
Expected: []sql.Row{
{types.NewOkResult(0)},
},
},
{
Skip: true, // we need parser changes to tell the difference between an empty string and a NULL
Query: "show create table child6;",
Expected: []sql.Row{
{"child6", "CREATE TABLE `child6` (\n" +
" `j` int NOT NULL,\n" +
" PRIMARY KEY (`j`),\n" +
" CONSTRAINT `child6_ibfk_1` FOREIGN KEY (`j`) REFERENCES `theparent` (`i`),\n" +
" CONSTRAINT `child6_ibfk_2` FOREIGN KEY (`j`) REFERENCES `theparent` (`i`)\n" +
") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"},
},
},

{
Query: "alter table child1 add constraint `child7_ibfk_1` foreign key (j) references theparent (i);",
Expected: []sql.Row{
{types.NewOkResult(0)},
},
},
{
// foreign key names are kept unique across tables
Query: "alter table child7 add foreign key (j) references theparent (i);",
ExpectedErr: sql.ErrForeignKeyDuplicateName,
},
},
},
}

var CreateForeignKeyTests = []ScriptTest{
Expand Down
42 changes: 42 additions & 0 deletions enginetest/queries/script_queries.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6330,6 +6330,48 @@ where
},
},
},
{
Name: "test index naming",
SetUpScript: []string{
"create table t (i int);",
"alter table t add index (i);",
"alter table t add index (i);",
"alter table t add index (i);",

"create table tt (i int);",
"alter table tt add index i_3(i);",
"alter table tt add index (i);",
"alter table tt add index (i);",
"alter table tt add index (i);",
},
Assertions: []ScriptTestAssertion{
{
Query: "show create table t",
Expected: []sql.Row{
{"t", "CREATE TABLE `t` (\n" +
" `i` int,\n" +
" KEY `i` (`i`),\n" +
" KEY `i_2` (`i`),\n" +
" KEY `i_3` (`i`)\n" +
") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"},
},
},
{
// MySQL preserves the other that indexes are created
// We store them in a map, so we have to sort to have some consistency
Query: "show create table tt",
Expected: []sql.Row{
{"tt", "CREATE TABLE `tt` (\n" +
" `i` int,\n" +
" KEY `i` (`i`),\n" +
" KEY `i_2` (`i`),\n" +
" KEY `i_3` (`i`),\n" +
" KEY `i_4` (`i`)\n" +
") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"},
},
},
},
},
}

var SpatialScriptTests = []ScriptTest{
Expand Down
9 changes: 6 additions & 3 deletions memory/table.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1769,6 +1769,9 @@ func (t *Table) GetDeclaredForeignKeys(ctx *sql.Context) ([]sql.ForeignKeyConstr
fks = append(fks, fk)
}
}
sort.Slice(fks, func(i, j int) bool {
return fks[i].Name < fks[j].Name
})
return fks, nil
}

Expand All @@ -1795,7 +1798,7 @@ func (t *Table) AddForeignKey(ctx *sql.Context, fk sql.ForeignKeyConstraint) err
lowerName := strings.ToLower(fk.Name)
for _, key := range data.fkColl.Keys() {
if strings.ToLower(key.Name) == lowerName {
return fmt.Errorf("Constraint %s already exists", fk.Name)
return sql.ErrForeignKeyDuplicateName.New(fk.Name)
}
}
data.fkColl.AddFK(fk)
Expand Down Expand Up @@ -1824,7 +1827,7 @@ func (t *Table) UpdateForeignKey(ctx *sql.Context, fkName string, fk sql.Foreign
lowerName := strings.ToLower(fk.Name)
for _, key := range data.fkColl.Keys() {
if strings.ToLower(key.Name) == lowerName {
return fmt.Errorf("Constraint %s already exists", fk.Name)
return sql.ErrForeignKeyDuplicateName.New(fk.Name)
}
}
data.fkColl.AddFK(fk)
Expand Down Expand Up @@ -1877,7 +1880,7 @@ func (t *Table) CreateCheck(ctx *sql.Context, check *sql.CheckDefinition) error

for _, key := range data.checks {
if key.Name == toInsert.Name {
return fmt.Errorf("constraint %s already exists", toInsert.Name)
return sql.ErrForeignKeyDuplicateName.New(toInsert.Name)
}
}

Expand Down
30 changes: 26 additions & 4 deletions sql/plan/alter_foreign_key.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package plan
import (
"fmt"
"sort"
"strconv"
"strings"

"github.com/dolthub/vitess/go/sqltypes"
Expand Down Expand Up @@ -240,10 +241,31 @@ func ResolveForeignKey(ctx *sql.Context, tbl sql.ForeignKeyTable, refTbl sql.For
if err != nil {
return err
}
fkLowerName := strings.ToLower(fkDef.Name)
for _, existingFk := range existingFks {
if fkLowerName == strings.ToLower(existingFk.Name) {
return sql.ErrForeignKeyDuplicateName.New(fkDef.Name)

if len(fkDef.Name) == 0 {
// find the next available name
// negative numbers behave weirdly
fkNamePrefix := fmt.Sprintf("%s_ibfk_", strings.ToLower(tbl.Name()))
var highest uint32
for _, existingFk := range existingFks {
if strings.HasPrefix(existingFk.Name, fkNamePrefix) {
numStr := strings.TrimPrefix(existingFk.Name, fkNamePrefix)
num, err := strconv.Atoi(numStr)
if err != nil {
continue
}
if uint32(num) > highest {
highest = uint32(num)
}
}
}
fkDef.Name = fmt.Sprintf("%s%d", fkNamePrefix, uint32(highest)+1)
} else {
fkLowerName := strings.ToLower(fkDef.Name)
for _, existingFk := range existingFks {
if fkLowerName == strings.ToLower(existingFk.Name) {
return sql.ErrForeignKeyDuplicateName.New(fkDef.Name)
}
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion sql/rowexec/ddl_iters.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1864,7 +1864,8 @@ func (b *BaseBuilder) executeAlterIndex(ctx *sql.Context, n *plan.AlterIndex) er
indexName = strings.Join(n.ColumnNames(), "")
if _, ok := indexMap[strings.ToLower(indexName)]; ok {
for i := 0; true; i++ {
newIndexName := fmt.Sprintf("%s_%d", indexName, i)
// MySQL starts at 2 for duplicate indexes
newIndexName := fmt.Sprintf("%s_%d", indexName, i+2)
if _, ok = indexMap[strings.ToLower(newIndexName)]; !ok {
indexName = newIndexName
break
Expand Down

0 comments on commit bfde35b

Please sign in to comment.