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

ddl, parser: support 'ALTER TABLE RENAME KEY TO' syntax #6475

Merged
merged 33 commits into from
Jun 7, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
8ffbb10
ddl, parser: support 'ALTER TABLE RENAME KEY TO' syntax
spongedu May 5, 2018
cc4f73d
Merge remote-tracking branch 'upstream/master' into fix_6405
spongedu May 8, 2018
de15d28
1. add double check 2. code refine
spongedu May 8, 2018
f1e3621
Merge remote-tracking branch 'upstream/master' into fix_6405
spongedu May 9, 2018
fe3423e
Fix comment
spongedu May 9, 2018
259f141
Merge remote-tracking branch 'upstream/master' into fix_6405
spongedu May 9, 2018
61bc9b6
Add comments
spongedu May 9, 2018
6249e70
Merge branch 'master' into fix_6405
spongedu May 11, 2018
5ff091e
Merge branch 'master' into fix_6405
shenli May 20, 2018
647f31f
set job.State when fail
spongedu May 20, 2018
492ac8d
Merge remote-tracking branch 'upstream/master' into fix_6405
spongedu May 20, 2018
d7b784d
Merge branch 'fix_6405' of github.com:spongedu/tidb into fix_6405
spongedu May 20, 2018
f5b2da6
Merge remote-tracking branch 'upstream/master' into fix_6405
spongedu May 26, 2018
b5e794c
Merge remote-tracking branch 'upstream/master' into fix_6405
spongedu May 26, 2018
696798f
Merge remote-tracking branch 'upstream/master' into fix_6405
spongedu May 28, 2018
bb28681
Merge remote-tracking branch 'upstream/master' into fix_6405
spongedu May 31, 2018
ccffedf
Merge remote-tracking branch 'upstream/master' into fix_6405
spongedu May 31, 2018
9b4c344
Merge remote-tracking branch 'upstream/master' into fix_6405
spongedu Jun 3, 2018
70d8304
Merge remote-tracking branch 'upstream/master' into fix_6405
spongedu Jun 3, 2018
afa6bbf
Merge branch 'master' into fix_6405
spongedu Jun 5, 2018
6237a56
Merge remote-tracking branch 'upstream/master' into fix_6405
spongedu Jun 5, 2018
1ba167b
1. code refine 2. add tests in parser_test.go
spongedu Jun 5, 2018
d5a175f
Merge branch 'master' into fix_6405
spongedu Jun 5, 2018
319ed7a
code refine
spongedu Jun 6, 2018
f43be2c
Merge branch 'master' into fix_6405
spongedu Jun 6, 2018
5843e9e
Merge remote-tracking branch 'upstream/master' into fix_6405
spongedu Jun 6, 2018
e7e576d
code refine
spongedu Jun 6, 2018
c30b65d
Merge branch 'fix_6405' of github.com:spongedu/tidb into fix_6405
spongedu Jun 6, 2018
ceca5ac
Merge branch 'master' into fix_6405
spongedu Jun 6, 2018
22e0ee0
Merge branch 'master' into fix_6405
tiancaiamao Jun 6, 2018
fedcf82
Merge branch 'master' into fix_6405
tiancaiamao Jun 7, 2018
582a02f
Merge branch 'master' into fix_6405
tiancaiamao Jun 7, 2018
27b1ca5
Merge branch 'master' into fix_6405
XuHuaiyu Jun 7, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions ast/ddl.go
Original file line number Diff line number Diff line change
Expand Up @@ -717,6 +717,7 @@ const (
AlterTableAlterColumn
AlterTableLock
AlterTableAlgorithm
AlterTableRenameIndex

// TODO: Add more actions
)
Expand Down Expand Up @@ -747,6 +748,8 @@ type AlterTableSpec struct {
Position *ColumnPosition
LockType LockType
Comment string
FromKey model.CIStr
ToKey model.CIStr
}

// Accept implements Node Accept interface.
Expand Down
38 changes: 38 additions & 0 deletions ddl/ddl_api.go
Original file line number Diff line number Diff line change
Expand Up @@ -991,6 +991,8 @@ func (d *ddl) AlterTable(ctx sessionctx.Context, ident ast.Ident, specs []*ast.A
err = d.RenameTable(ctx, ident, newIdent)
case ast.AlterTableDropPrimaryKey:
err = ErrUnsupportedModifyPrimaryKey.GenByArgs("drop")
case ast.AlterTableRenameIndex:
err = d.RenameIndex(ctx, ident, spec)
case ast.AlterTableOption:
for _, opt := range spec.Options {
switch opt.Tp {
Expand Down Expand Up @@ -1559,6 +1561,42 @@ func (d *ddl) AlterTableComment(ctx sessionctx.Context, ident ast.Ident, spec *a
return errors.Trace(err)
}

// RenameIndex updates the table comment information.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the comment not correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lamxTyler ennnnn..... Forget to change this.. Will fix.

func (d *ddl) RenameIndex(ctx sessionctx.Context, ident ast.Ident, spec *ast.AlterTableSpec) error {
is := d.infoHandle.Get()
schema, ok := is.SchemaByName(ident.Schema)
if !ok {
return infoschema.ErrDatabaseNotExists.GenByArgs(ident.Schema)
}

tb, err := is.TableByName(ident.Schema, ident.Name)
if err != nil {
return errors.Trace(infoschema.ErrTableNotExists.GenByArgs(ident.Schema, ident.Name))
}

if fromIdx := findIndexByName(spec.FromKey.L, tb.Meta().Indices); fromIdx == nil {
return errors.Trace(infoschema.ErrKeyNotExists.GenByArgs(spec.FromKey.O, tb.Meta().Name))
}
if spec.FromKey.O == spec.ToKey.O {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's MySQL's behavior ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the MySQL's behavior. You can see the test case in line 2660.

}
if toIdx := findIndexByName(spec.ToKey.L, tb.Meta().Indices); toIdx != nil && spec.FromKey.L != spec.ToKey.L {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spec.FromKey.L != spec.ToKey.L ?
I'm confused with .L and .O

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TiDB is case-insensitive.
So I think we should use spec.FromKey.L == spec.ToKey.L in line 1580, and we can remove this check here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tiancaiamao @zimulala although index check is case-insensitive (index a and index A are considered the same index when check), but indexes' names, or say indexes' representations, are still case-sensitive, we can rename a index a to A.
The logic from line 1580 to line 1586 are as follows:

  1. in line 1580, we just do nothing but return if the FromKey and ToKey are exactly the same. (rename key a to a ends here)
  2. in line 1583, we check if an index is renamed as an already-exists index (rename key a to exist-key ends here, rename key a to non-exists-key and rename key a to A pass the check)

Take an example in MySQL, pay attention to KEY a:

mysql> create table t1 (pk int primary key, i int, j int, key a(i));
Query OK, 0 rows affected (0.03 sec)

mysql> show create table t1;
+-------+------------------------------------------------------------------------------------------------------------------------------------
-----------------------------------------------+
| Table | Create Table                                                                                                                       
                                               |
+-------+------------------------------------------------------------------------------------------------------------------------------------
-----------------------------------------------+
| t1    | CREATE TABLE `t1` (
  `pk` int(11) NOT NULL,
  `i` int(11) DEFAULT NULL,
  `j` int(11) DEFAULT NULL,
  PRIMARY KEY (`pk`),
  KEY `a` (`i`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1 |
+-------+------------------------------------------------------------------------------------------------------------------------------------
-----------------------------------------------+
1 row in set (0.00 sec)

mysql> alter table t1 rename key a to A;
Query OK, 0 rows affected (0.02 sec)
Records: 0  Duplicates: 0  Warnings: 0

mysql> show create table t1;
+-------+------------------------------------------------------------------------------------------------------------------------------------
-----------------------------------------------+
| Table | Create Table                                                                                                                       
                                               |
+-------+------------------------------------------------------------------------------------------------------------------------------------
-----------------------------------------------+
| t1    | CREATE TABLE `t1` (
  `pk` int(11) NOT NULL,
  `i` int(11) DEFAULT NULL,
  `j` int(11) DEFAULT NULL,
  PRIMARY KEY (`pk`),
  KEY `A` (`i`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1 |
+-------+------------------------------------------------------------------------------------------------------------------------------------
-----------------------------------------------+
1 row in set (0.00 sec)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, add comment to the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

return errors.Trace(infoschema.ErrKeyNameDuplicate.GenByArgs(toIdx.Name.O))
}

job := &model.Job{
SchemaID: schema.ID,
TableID: tb.Meta().ID,
Type: model.ActionRenameIndex,
BinlogInfo: &model.HistoryInfo{},
Args: []interface{}{spec.FromKey, spec.ToKey},
}

err = d.doDDLJob(ctx, job)
err = d.callHookOnChanged(err)
return errors.Trace(err)
}

// DropTable will proceed even if some table in the list does not exists.
func (d *ddl) DropTable(ctx sessionctx.Context, ti ast.Ident) (err error) {
is := d.getInformationSchema()
Expand Down
2 changes: 2 additions & 0 deletions ddl/ddl_worker.go
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,8 @@ func (d *ddl) runDDLJob(t *meta.Meta, job *model.Job) (ver int64, err error) {
ver, err = d.onShardRowID(t, job)
case model.ActionModifyTableComment:
ver, err = d.onModifyTableComment(t, job)
case model.ActionRenameIndex:
ver, err = d.onRenameIndex(t, job)
default:
// Invalid job, cancel it.
job.State = model.JobStateCancelled
Expand Down
25 changes: 25 additions & 0 deletions ddl/table.go
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,31 @@ func (d *ddl) onModifyTableComment(t *meta.Meta, job *model.Job) (ver int64, _ e
return ver, nil
}

func (d *ddl) onRenameIndex(t *meta.Meta, job *model.Job) (ver int64, _ error) {
var from, to model.CIStr
if err := job.DecodeArgs(&from, &to); err != nil {
job.State = model.JobStateCancelled
return ver, errors.Trace(err)
}

tblInfo, err := getTableInfo(t, job, job.SchemaID)
if err != nil {
return ver, errors.Trace(err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add job.State = model.JobStateCancelled ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

}

idx := findIndexByName(from.L, tblInfo.Indices)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid we have to double check here if we don't take @coocood 's suggestion.
Take this scenario for example:
TiDB client1, rename idx1 to idx3, check success, put into queue
TiDB client2, rename idx2 to idx3, check success, put into queue

When the code runs to here, those two request would both success!
We lost a index, no error!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, double check is needed here.

if idx == nil {
return ver, errors.Trace(infoschema.ErrKeyNotExists.GenByArgs(from, tblInfo.Name))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

}
idx.Name = to
ver, err = updateVersionAndTableInfo(t, job, tblInfo, true)
if err != nil {
return ver, errors.Trace(err)
}
job.FinishTableJob(model.JobStateDone, model.StatePublic, ver, tblInfo)
return ver, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/nil/ errors.Trace(err)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think return nil is ok.

}

func checkTableNotExists(t *meta.Meta, job *model.Job, schemaID int64, tableName string) error {
// Check this table's database.
tables, err := t.ListTables(schemaID)
Expand Down
45 changes: 45 additions & 0 deletions executor/executor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2635,6 +2635,51 @@ func (s *testSuite) TestCheckIndex(c *C) {
// table data (handle, data): (1, 10), (2, 20), (4, 40)
}

func (s *testSuite) TestRenameIndex(c *C) {
s.ctx = mock.NewContext()
s.ctx.Store = s.store
_, err := session.BootstrapSession(s.store)
c.Assert(err, IsNil)
se, err := session.CreateSession4Test(s.store)
c.Assert(err, IsNil)
defer se.Close()

_, err = se.Execute(context.Background(), "create database test_rename_index")
c.Assert(err, IsNil)
_, err = se.Execute(context.Background(), "use test_rename_index")
c.Assert(err, IsNil)
_, err = se.Execute(context.Background(), "create table t (pk int primary key, c int default 1, c1 int default 1, unique key k1(c), key k2(c1))")
c.Assert(err, IsNil)

// Test rename success
_, err = se.Execute(context.Background(), "alter table t rename index k1 to k3")
c.Assert(err, IsNil)
_, err = se.Execute(context.Background(), "admin check index t k3")
c.Assert(err, IsNil)

// Test rename to the same name
_, err = se.Execute(context.Background(), "alter table t rename index k3 to k3")
c.Assert(err, IsNil)
_, err = se.Execute(context.Background(), "admin check index t k3")
c.Assert(err, IsNil)

// Test rename on non-exists keys
_, err = se.Execute(context.Background(), "alter table t rename index x to x")
c.Assert(err, NotNil)
c.Assert(strings.Contains(err.Error(), "Key 'x' doesn't exist in table 't'"), IsTrue)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use testErrorCode in this file.

Copy link
Contributor Author

@spongedu spongedu Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll move these tests to ddl/ddl_db_test.go


// Test rename on already-exists keys
_, err = se.Execute(context.Background(), "alter table t rename index k3 to k2")
c.Assert(err, NotNil)
c.Assert(strings.Contains(err.Error(), "Duplicate key name 'k2'"), IsTrue)

_, err = se.Execute(context.Background(), "alter table t rename index k2 to K2")
c.Assert(err, IsNil)
_, err = se.Execute(context.Background(), "alter table t rename key k3 to K2")
c.Assert(err, NotNil)
c.Assert(strings.Contains(err.Error(), "Duplicate key name 'K2'"), IsTrue)
}

func setColValue(c *C, txn kv.Transaction, key kv.Key, v types.Datum) {
row := []types.Datum{v, {}}
colIDs := []int64{2, 3}
Expand Down
22 changes: 15 additions & 7 deletions infoschema/infoschema.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ var (
ErrColumnExists = terror.ClassSchema.New(codeColumnExists, "Duplicate column name '%s'")
// ErrIndexExists returns for index already exists.
ErrIndexExists = terror.ClassSchema.New(codeIndexExists, "Duplicate Index")
// ErrKeyNameDuplicate returns for index duplicate when rename index.
ErrKeyNameDuplicate = terror.ClassSchema.New(codeKeyNameDuplicate, "Duplicate key name '%s'")
// ErrKeyNotExists returns for index not exists.
ErrKeyNotExists = terror.ClassSchema.New(codeKeyNotExists, " Key '%s' doesn't exist in table '%s'")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need replace " Key" with "Key"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes.

// ErrMultiplePriKey returns for multiple primary keys.
ErrMultiplePriKey = terror.ClassSchema.New(codeMultiplePriKey, "Multiple primary key defined")
// ErrTooManyKeyParts returns for too many key parts.
Expand Down Expand Up @@ -279,13 +283,15 @@ const (
codeForeignKeyNotExists = 1091
codeWrongFkDef = 1239

codeDatabaseExists = 1007
codeTableExists = 1050
codeBadTable = 1051
codeColumnExists = 1060
codeIndexExists = 1831
codeMultiplePriKey = 1068
codeTooManyKeyParts = 1070
codeDatabaseExists = 1007
codeTableExists = 1050
codeBadTable = 1051
codeColumnExists = 1060
codeIndexExists = 1831
codeMultiplePriKey = 1068
codeTooManyKeyParts = 1070
codeKeyNameDuplicate = 1061
codeKeyNotExists = 1176
)

func init() {
Expand All @@ -304,6 +310,8 @@ func init() {
codeIndexExists: mysql.ErrDupIndex,
codeMultiplePriKey: mysql.ErrMultiplePriKey,
codeTooManyKeyParts: mysql.ErrTooManyKeyParts,
codeKeyNameDuplicate: mysql.ErrDupKeyName,
codeKeyNotExists: mysql.ErrKeyDoesNotExist,
}
terror.ErrClassToMySQLCodes[terror.ClassSchema] = schemaMySQLErrCodes
initInfoSchemaDB()
Expand Down
2 changes: 2 additions & 0 deletions model/ddl.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ const (
ActionSetDefaultValue ActionType = 15
ActionShardRowID ActionType = 16
ActionModifyTableComment ActionType = 17
ActionRenameIndex ActionType = 18
)

var actionMap = map[ActionType]string{
Expand All @@ -66,6 +67,7 @@ var actionMap = map[ActionType]string{
ActionSetDefaultValue: "set default value",
ActionShardRowID: "shard row ID",
ActionModifyTableComment: "modify table comment",
ActionRenameIndex: "rename index",
}

// String return current ddl action in string
Expand Down
8 changes: 8 additions & 0 deletions parser/parser.y
Original file line number Diff line number Diff line change
Expand Up @@ -1036,6 +1036,14 @@ AlterTableSpec:
NewTable: $3.(*ast.TableName),
}
}
| "RENAME" KeyOrIndex Identifier "TO" Identifier
{
$$ = &ast.AlterTableSpec{
Tp: ast.AlterTableRenameIndex,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep alignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

FromKey: model.NewCIStr($3),
ToKey: model.NewCIStr($5),
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests in parser_test.go

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

| LockClause
{
$$ = &ast.AlterTableSpec{
Expand Down