-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
ddl: support create/drop expression index #14117
Changes from 13 commits
d85a46a
c89d7c1
8ee0ae7
a3b5ab1
b999122
a5bb03c
133428e
5c5e4f9
634e2a9
ca4a262
149d728
018620e
3aade76
96b9fd2
22ed09c
93b786c
3ac07f2
ea0c7fc
c109cbc
39f7032
b81d400
6fcbd90
1abd378
40b9b07
fcd8385
40a5ea1
1a40266
0fbfd50
edace5d
4938b40
8e1acba
8c19669
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,6 +51,7 @@ var _ = Suite(&testIntegrationSuite2{&testIntegrationSuite{}}) | |
var _ = Suite(&testIntegrationSuite3{&testIntegrationSuite{}}) | ||
var _ = Suite(&testIntegrationSuite4{&testIntegrationSuite{}}) | ||
var _ = Suite(&testIntegrationSuite5{&testIntegrationSuite{}}) | ||
var _ = Suite(&testIntegrationSuite6{&testIntegrationSuite{}}) | ||
|
||
type testIntegrationSuite struct { | ||
lease time.Duration | ||
|
@@ -121,6 +122,7 @@ func (s *testIntegrationSuite2) TearDownTest(c *C) { | |
type testIntegrationSuite3 struct{ *testIntegrationSuite } | ||
type testIntegrationSuite4 struct{ *testIntegrationSuite } | ||
type testIntegrationSuite5 struct{ *testIntegrationSuite } | ||
type testIntegrationSuite6 struct{ *testIntegrationSuite } | ||
|
||
func (s *testIntegrationSuite5) TestNoZeroDateMode(c *C) { | ||
tk := testkit.NewTestKit(c, s.store) | ||
|
@@ -1010,22 +1012,22 @@ func (s *testIntegrationSuite5) TestBackwardCompatibility(c *C) { | |
|
||
unique := false | ||
indexName := model.NewCIStr("idx_b") | ||
idxColName := &ast.IndexPartSpecification{ | ||
indexPartSpecification := &ast.IndexPartSpecification{ | ||
Column: &ast.ColumnName{ | ||
Schema: schemaName, | ||
Table: tableName, | ||
Name: model.NewCIStr("b"), | ||
}, | ||
Length: types.UnspecifiedLength, | ||
} | ||
idxColNames := []*ast.IndexPartSpecification{idxColName} | ||
indexPartSpecifications := []*ast.IndexPartSpecification{indexPartSpecification} | ||
var indexOption *ast.IndexOption | ||
job := &model.Job{ | ||
SchemaID: schema.ID, | ||
TableID: tbl.Meta().ID, | ||
Type: model.ActionAddIndex, | ||
BinlogInfo: &model.HistoryInfo{}, | ||
Args: []interface{}{unique, indexName, idxColNames, indexOption}, | ||
Args: []interface{}{unique, indexName, indexPartSpecifications, indexOption}, | ||
} | ||
txn, err := s.store.Begin() | ||
c.Assert(err, IsNil) | ||
|
@@ -1946,3 +1948,55 @@ func (s *testIntegrationSuite3) TestParserIssue284(c *C) { | |
tk.MustExec("drop table test.t_parser_issue_284") | ||
tk.MustExec("drop table test.t_parser_issue_284_2") | ||
} | ||
|
||
func (s *testIntegrationSuite6) TestAddExpressionIndex(c *C) { | ||
bb7133 marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add these tests to partition tables. And add a rollback test to test the add index operation, such as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
tk := testkit.NewTestKit(c, s.store) | ||
tk.MustExec("use test") | ||
tk.MustExec("drop table if exists t;") | ||
tk.MustExec("create table t (a int, b real);") | ||
tk.MustExec("insert into t values (1, 2.1);") | ||
tk.MustExec("alter table t add index idx((a+b));") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The result of
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still different, tidb>show create table t;
+-------+-------------------------------------------------------------+
| Table | Create Table |
+-------+-------------------------------------------------------------+
| t | CREATE TABLE `t` ( |
| | `a` int(11) DEFAULT NULL, |
| | `b` int(11) DEFAULT NULL, |
| | KEY `idx` ((`a` + `b`)) | -- MySQL has 3 '('.
| | ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin |
+-------+-------------------------------------------------------------+ Other problem is: tidb>create table t (a int,b int, index idx((a+b)));
-- TiDB will panic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2 |
||
|
||
tblInfo, err := s.dom.InfoSchema().TableByName(model.NewCIStr("test"), model.NewCIStr("t")) | ||
c.Assert(err, IsNil) | ||
columns := tblInfo.Meta().Columns | ||
c.Assert(len(columns), Equals, 3) | ||
c.Assert(columns[2].Hidden, IsTrue) | ||
|
||
tk.MustQuery("select * from t;").Check(testkit.Rows("1 2.1")) | ||
|
||
tk.MustExec("alter table t add index idx_multi((a+b),(a+1), b);") | ||
tblInfo, err = s.dom.InfoSchema().TableByName(model.NewCIStr("test"), model.NewCIStr("t")) | ||
c.Assert(err, IsNil) | ||
columns = tblInfo.Meta().Columns | ||
c.Assert(len(columns), Equals, 5) | ||
wjhuang2016 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
c.Assert(columns[3].Hidden, IsTrue) | ||
c.Assert(columns[4].Hidden, IsTrue) | ||
|
||
tk.MustQuery("select * from t;").Check(testkit.Rows("1 2.1")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we add a test for index data like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This pr can't use expression index, should we add test using expression index explicitly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test for index data will be added in following PR. |
||
|
||
tk.MustExec("alter table t drop index idx;") | ||
tblInfo, err = s.dom.InfoSchema().TableByName(model.NewCIStr("test"), model.NewCIStr("t")) | ||
c.Assert(err, IsNil) | ||
columns = tblInfo.Meta().Columns | ||
c.Assert(len(columns), Equals, 4) | ||
|
||
tk.MustQuery("select * from t;").Check(testkit.Rows("1 2.1")) | ||
|
||
tk.MustExec("alter table t drop index idx_multi;") | ||
tblInfo, err = s.dom.InfoSchema().TableByName(model.NewCIStr("test"), model.NewCIStr("t")) | ||
c.Assert(err, IsNil) | ||
columns = tblInfo.Meta().Columns | ||
c.Assert(len(columns), Equals, 2) | ||
|
||
tk.MustQuery("select * from t;").Check(testkit.Rows("1 2.1")) | ||
} | ||
|
||
func (s *testIntegrationSuite6) TestCreateExpressionIndexError(c *C) { | ||
tk := testkit.NewTestKit(c, s.store) | ||
tk.MustExec("use test") | ||
tk.MustExec("drop table if exists t;") | ||
tk.MustExec("create table t (a int, b real);") | ||
tk.MustGetErrCode("alter table t add primary key ((a+b));", mysql.ErrFunctionalIndexPrimaryKey) | ||
wjhuang2016 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
wjhuang2016 marked this conversation as resolved.
Show resolved
Hide resolved
bb7133 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3426,7 +3426,7 @@ func getAnonymousIndex(t table.Table, colName model.CIStr) model.CIStr { | |
} | ||
|
||
func (d *ddl) CreatePrimaryKey(ctx sessionctx.Context, ti ast.Ident, indexName model.CIStr, | ||
idxColNames []*ast.IndexPartSpecification, indexOption *ast.IndexOption) error { | ||
indexPartSpecifications []*ast.IndexPartSpecification, indexOption *ast.IndexOption) error { | ||
if !config.GetGlobalConfig().AlterPrimaryKey { | ||
return ErrUnsupportedModifyPrimaryKey.GenWithStack("Unsupported add primary key, alter-primary-key is false") | ||
} | ||
|
@@ -3445,22 +3445,30 @@ func (d *ddl) CreatePrimaryKey(ctx sessionctx.Context, ti ast.Ident, indexName m | |
return infoschema.ErrMultiplePriKey | ||
} | ||
|
||
// Primary keys cannot include expression index parts. A primary key requires the generated column to be stored, | ||
// but expression index parts are implemented as virtual generated columns, not stored generated columns. | ||
for _, idxPart := range indexPartSpecifications { | ||
if idxPart.Expr != nil { | ||
return ErrFunctionalIndexPrimaryKey | ||
} | ||
} | ||
|
||
tblInfo := t.Meta() | ||
// Check before the job is put to the queue. | ||
// This check is redundant, but useful. If DDL check fail before the job is put | ||
// to job queue, the fail path logic is super fast. | ||
// After DDL job is put to the queue, and if the check fail, TiDB will run the DDL cancel logic. | ||
// The recover step causes DDL wait a few seconds, makes the unit test painfully slow. | ||
_, err = buildIndexColumns(tblInfo.Columns, idxColNames) | ||
_, err = buildIndexColumns(tblInfo.Columns, indexPartSpecifications) | ||
if err != nil { | ||
return errors.Trace(err) | ||
} | ||
if _, err = checkPKOnGeneratedColumn(tblInfo, idxColNames); err != nil { | ||
if _, err = checkPKOnGeneratedColumn(tblInfo, indexPartSpecifications); err != nil { | ||
return err | ||
} | ||
|
||
if tblInfo.GetPartitionInfo() != nil { | ||
if err := checkPartitionKeysConstraint(tblInfo.GetPartitionInfo(), idxColNames, tblInfo, true); err != nil { | ||
if err := checkPartitionKeysConstraint(tblInfo.GetPartitionInfo(), indexPartSpecifications, tblInfo, true); err != nil { | ||
return err | ||
} | ||
} | ||
|
@@ -3478,7 +3486,7 @@ func (d *ddl) CreatePrimaryKey(ctx sessionctx.Context, ti ast.Ident, indexName m | |
SchemaName: schema.Name.L, | ||
Type: model.ActionAddPrimaryKey, | ||
BinlogInfo: &model.HistoryInfo{}, | ||
Args: []interface{}{unique, indexName, idxColNames, indexOption, sqlMode}, | ||
Args: []interface{}{unique, indexName, indexPartSpecifications, indexOption, sqlMode}, | ||
Priority: ctx.GetSessionVars().DDLReorgPriority, | ||
} | ||
|
||
|
@@ -3488,8 +3496,7 @@ func (d *ddl) CreatePrimaryKey(ctx sessionctx.Context, ti ast.Ident, indexName m | |
} | ||
|
||
func (d *ddl) CreateIndex(ctx sessionctx.Context, ti ast.Ident, keyType ast.IndexKeyType, indexName model.CIStr, | ||
idxColNames []*ast.IndexPartSpecification, indexOption *ast.IndexOption, ifNotExists bool) error { | ||
|
||
indexPartSpecifications []*ast.IndexPartSpecification, indexOption *ast.IndexOption, ifNotExists bool) error { | ||
// not support Spatial and FullText index | ||
if keyType == ast.IndexKeyTypeFullText || keyType == ast.IndexKeyTypeSpatial { | ||
return errUnsupportedIndexType.GenWithStack("FULLTEXT and SPATIAL index is not supported") | ||
|
@@ -3502,7 +3509,7 @@ func (d *ddl) CreateIndex(ctx sessionctx.Context, ti ast.Ident, keyType ast.Inde | |
|
||
// Deal with anonymous index. | ||
if len(indexName.L) == 0 { | ||
indexName = getAnonymousIndex(t, idxColNames[0].Column.Name) | ||
indexName = getAnonymousIndex(t, indexPartSpecifications[0].Column.Name) | ||
} | ||
|
||
if indexInfo := t.Meta().FindIndexByName(indexName.L); indexInfo != nil { | ||
|
@@ -3519,32 +3526,111 @@ func (d *ddl) CreateIndex(ctx sessionctx.Context, ti ast.Ident, keyType ast.Inde | |
} | ||
|
||
tblInfo := t.Meta() | ||
hiddenCols := make([]*model.ColumnInfo, 0, len(indexPartSpecifications)) | ||
// build hidden columns if necessary | ||
wjhuang2016 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
for i, idxPart := range indexPartSpecifications { | ||
if idxPart.Expr != nil { | ||
wjhuang2016 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
idxPart.Column = &ast.ColumnName{Name: model.NewCIStr(fmt.Sprintf("_v$_%s_%d", indexName, i))} | ||
wjhuang2016 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
idxPart.Length = types.UnspecifiedLength | ||
// The index part is an expression, prepare a hidden column for it. | ||
if len(idxPart.Column.Name.L) > mysql.MaxColumnNameLength { | ||
// TODO: refine the error message | ||
return ErrTooLongIdent.GenWithStackByArgs("hidden column") | ||
} | ||
// // TODO: refine the error message | ||
wjhuang2016 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if err := checkIllegalFn4GeneratedColumn("expression index", idxPart.Expr); err != nil { | ||
return errors.Trace(err) | ||
} | ||
|
||
var sb strings.Builder | ||
restoreFlags := format.RestoreStringSingleQuotes | format.RestoreKeyWordLowercase | format.RestoreNameBackQuotes | | ||
format.RestoreSpacesAroundBinaryOperation | ||
restoreCtx := format.NewRestoreCtx(restoreFlags, &sb) | ||
sb.Reset() | ||
err = idxPart.Expr.Restore(restoreCtx) | ||
if err != nil { | ||
return errors.Trace(err) | ||
} | ||
expr, err := expression.RewriteSimpleExprWithTableInfo(ctx, tblInfo, idxPart.Expr) | ||
if err != nil { | ||
// TODO: refine the error message | ||
return err | ||
} | ||
if _, ok := expr.(*expression.Column); ok { | ||
return ErrFunctionalIndexOnField | ||
wjhuang2016 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
colInfo := &model.ColumnInfo{ | ||
Name: idxPart.Column.Name, | ||
GeneratedExprString: sb.String(), | ||
GeneratedStored: false, | ||
Version: model.CurrLatestColumnInfoVersion, | ||
Dependences: make(map[string]struct{}), | ||
Hidden: true, | ||
FieldType: *expr.GetType(), | ||
} | ||
for _, colName := range findColumnNamesInExpr(idxPart.Expr) { | ||
colInfo.Dependences[colName.Name.O] = struct{}{} | ||
} | ||
if err = checkDependedColExist(colInfo.Dependences, t.Cols()); err != nil { | ||
return errors.Trace(err) | ||
} | ||
idxPart.Expr = nil | ||
wjhuang2016 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
hiddenCols = append(hiddenCols, colInfo) | ||
} | ||
} | ||
wjhuang2016 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Check before the job is put to the queue. | ||
// This check is redundant, but useful. If DDL check fail before the job is put | ||
// to job queue, the fail path logic is super fast. | ||
// After DDL job is put to the queue, and if the check fail, TiDB will run the DDL cancel logic. | ||
// The recover step causes DDL wait a few seconds, makes the unit test painfully slow. | ||
_, err = buildIndexColumns(tblInfo.Columns, idxColNames) | ||
_, err = buildIndexColumns(append(tblInfo.Columns, hiddenCols...), indexPartSpecifications) | ||
if err != nil { | ||
return errors.Trace(err) | ||
} | ||
if unique && tblInfo.GetPartitionInfo() != nil { | ||
if err := checkPartitionKeysConstraint(tblInfo.GetPartitionInfo(), idxColNames, tblInfo, false); err != nil { | ||
if err := checkPartitionKeysConstraint(tblInfo.GetPartitionInfo(), indexPartSpecifications, tblInfo, false); err != nil { | ||
return err | ||
} | ||
} | ||
// May be truncate comment here, when index comment too long and sql_mode is't strict. | ||
if _, err = validateCommentLength(ctx.GetSessionVars(), indexName.String(), indexOption); err != nil { | ||
return errors.Trace(err) | ||
} | ||
|
||
if len(hiddenCols) > 0 { | ||
// Check hidden column | ||
wjhuang2016 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if err = checkAddColumnTooManyColumns(len(t.Cols()) + len(hiddenCols)); err != nil { | ||
return errors.Trace(err) | ||
} | ||
// Check whether the hidden columns have existed. | ||
for _, colInfo := range hiddenCols { | ||
// Check whether the hidden columns have existed. | ||
wjhuang2016 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
col := table.FindCol(t.Cols(), colInfo.Name.String()) | ||
if col != nil { | ||
// TODO: use expression index related error | ||
return infoschema.ErrColumnExists.GenWithStackByArgs(colInfo.Name.String()) | ||
} | ||
if err = checkAutoIncrementRef("", colInfo.Dependences, tblInfo); err != nil { | ||
return errors.Trace(err) | ||
} | ||
} | ||
for _, indexPart := range indexPartSpecifications { | ||
if indexPart.Expr != nil { | ||
// TODO: refine the error message | ||
if err := checkIllegalFn4GeneratedColumn("", indexPart.Expr); err != nil { | ||
wjhuang2016 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return errors.Trace(err) | ||
} | ||
} | ||
} | ||
} | ||
job := &model.Job{ | ||
SchemaID: schema.ID, | ||
TableID: t.Meta().ID, | ||
SchemaName: schema.Name.L, | ||
Type: model.ActionAddIndex, | ||
BinlogInfo: &model.HistoryInfo{}, | ||
Args: []interface{}{unique, indexName, idxColNames, indexOption}, | ||
Args: []interface{}{unique, indexName, indexPartSpecifications, indexOption, hiddenCols}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you add a parameter, there may be compatibility issues when doing rolling upgrades when there are operations of this type. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we rolling upgrade, the old TiDB can't find the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha! |
||
Priority: ctx.GetSessionVars().DDLReorgPriority, | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we remove this logic? It seems only the current expression index will use this hidden column. So we needn't update this column's offset when dropping this expression index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If remove this logic, the column offset of the expression index will be wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got it. At this time, the expression index only modifies the state, and it has not been actually deleted.