Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

have generated index and foreign key names match mysql #2438

Merged
merged 15 commits into from
Apr 9, 2024
202 changes: 201 additions & 1 deletion enginetest/queries/foreign_key_queries.go
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);",
jycor marked this conversation as resolved.
Show resolved Hide resolved
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
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
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
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
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)
jycor marked this conversation as resolved.
Show resolved Hide resolved
if _, ok = indexMap[strings.ToLower(newIndexName)]; !ok {
indexName = newIndexName
break
Expand Down