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

spongedu · 2018-05-05T17:55:29Z

shenli · 2018-05-07T02:53:05Z

@spongedu Thanks!
Please resolve the conflicts.

tiancaiamao · 2018-05-08T06:45:28Z

ddl/ddl_api.go

+		return errors.Trace(infoschema.ErrKeyNotExists.GenByArgs(spec.FromKey.O, tb.Meta().Name))
+	}
+	if spec.FromKey.O == spec.ToKey.O {
+		return nil


What's MySQL's behavior ?

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

tiancaiamao · 2018-05-08T06:48:37Z

ddl/ddl_api.go

+	if spec.FromKey.O == spec.ToKey.O {
+		return nil
+	}
+	if toIdx := findIndexByName(spec.ToKey.L, tb.Meta().Indices); toIdx != nil && spec.FromKey.L != spec.ToKey.L {


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

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.

@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:

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)

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)

So, add comment to the code.

tiancaiamao · 2018-05-08T07:11:12Z

ddl/table.go

+		return ver, errors.Trace(err)
+	}
+
+	idx := findIndexByName(from.L, tblInfo.Indices)


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!

yes, double check is needed here.

zimulala · 2018-05-08T08:31:58Z

infoschema/infoschema.go

+	// 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'")


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

zimulala · 2018-05-08T08:50:48Z

parser/parser.y

+|	"RENAME" KeyOrIndex Identifier "TO" Identifier
+	{
+		$$ = &ast.AlterTableSpec{
+			Tp:    		ast.AlterTableRenameIndex,


Keep alignment.

zimulala · 2018-05-08T08:54:02Z

ddl/ddl_api.go

+		return errors.Trace(infoschema.ErrKeyNotExists.GenByArgs(spec.FromKey.O, tb.Meta().Name))
+	}
+	if spec.FromKey.O == spec.ToKey.O {
+		return nil


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

zimulala · 2018-05-08T08:58:29Z

ddl/ddl_api.go

+	if spec.FromKey.O == spec.ToKey.O {
+		return nil
+	}
+	if toIdx := findIndexByName(spec.ToKey.L, tb.Meta().Indices); toIdx != nil && spec.FromKey.L != spec.ToKey.L {


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.

alivxxx · 2018-05-09T03:10:00Z

ddl/ddl_api.go

@@ -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.


Is the comment not correct?

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

tiancaiamao · 2018-05-10T03:50:06Z

LGTM

ciscoxll · 2018-05-14T08:21:20Z

ddl/table.go

+		return ver, errors.Trace(err)
+	}
+	job.FinishTableJob(model.JobStateDone, model.StatePublic, ver, tblInfo)
+	return ver, nil


s/nil/ errors.Trace(err)

I think return nil is ok.

ciscoxll · 2018-05-14T08:24:40Z

ddl/table.go

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


Can you add job.State = model.JobStateCancelled ?

ciscoxll · 2018-05-14T08:26:14Z

ddl/table.go

+	// Double check. See function `RenameIndex` in ddl_api.go
+	idx := findIndexByName(from.L, tblInfo.Indices)
+	if idx == nil {
+		return ver, errors.Trace(infoschema.ErrKeyNotExists.GenByArgs(from, tblInfo.Name))


shenli · 2018-05-20T03:51:41Z

@zimulala PTAL

shenli · 2018-05-20T03:51:55Z

/run-all-tests

spongedu · 2018-05-21T05:31:25Z

@ciscoxll PTAL

zimulala · 2018-06-06T02:25:13Z

/run-all-tests

zimulala · 2018-06-06T02:26:56Z

ddl/table.go

+	}
+	if err != nil {
+		job.State = model.JobStateCancelled
+		return ver, err


s/err/error.Trace(err)

zimulala · 2018-06-06T02:27:19Z

ddl/ddl_api.go

+		return nil
+	}
+	if err != nil {
+		return err


s/err/error.Trace(err)

zimulala · 2018-06-06T02:28:22Z

ddl/ddl_api.go

+	return errors.Trace(err)
+}
+
+func (d *ddl) validateRenameIndex(from, to model.CIStr, tbl *model.TableInfo) (duplicate bool, err error) {


Could you put this function to the file of index.go?

spongedu · 2018-06-06T02:56:48Z

Done @zimulala PTAL :)

zimulala

LGTM

zimulala · 2018-06-06T06:25:26Z

PTAL @winkyao @shenli @ciscoxll

shenli

Rest LGTM

shenli · 2018-06-06T06:48:52Z

ddl/index.go

@@ -175,6 +176,23 @@ func dropIndexColumnFlag(tblInfo *model.TableInfo, indexInfo *model.IndexInfo) {
 	}
 }

+func validateRenameIndex(from, to model.CIStr, tbl *model.TableInfo) (duplicate bool, err error) {


duplicate is not a proper name for the first return value. It confused me. I thought it is the case that the to.L is the same as any other index name.

ignore is better

reasonable. I've fixed this. :)

shenli · 2018-06-06T10:11:49Z

LGTM

shenli · 2018-06-06T10:12:14Z

/run-all-tests

shenli · 2018-06-07T02:08:36Z

/run-all-tests

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

8ffbb10

zz-jason added the DDL label May 5, 2018

shenli added contribution This PR is from a community contributor. require-LGT3 Indicates that the PR requires three LGTM. labels May 7, 2018

tiancaiamao reviewed May 8, 2018

View reviewed changes

zimulala reviewed May 8, 2018

View reviewed changes

spongedu added 2 commits May 9, 2018 02:50

Merge remote-tracking branch 'upstream/master' into fix_6405

cc4f73d

1. add double check 2. code refine

de15d28

alivxxx reviewed May 9, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fix_6405

f1e3621

zimulala removed the require-LGT3 Indicates that the PR requires three LGTM. label May 9, 2018

spongedu added 3 commits May 9, 2018 11:37

Fix comment

fe3423e

Merge remote-tracking branch 'upstream/master' into fix_6405

259f141

Add comments

61bc9b6

tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label May 10, 2018

Merge branch 'master' into fix_6405

6249e70

ciscoxll reviewed May 14, 2018

View reviewed changes

Merge branch 'master' into fix_6405

5ff091e

spongedu added 3 commits May 20, 2018 15:52

set job.State when fail

647f31f

Merge remote-tracking branch 'upstream/master' into fix_6405

492ac8d

Merge branch 'fix_6405' of github.com:spongedu/tidb into fix_6405

d7b784d

spongedu added 2 commits May 26, 2018 20:22

Merge remote-tracking branch 'upstream/master' into fix_6405

f5b2da6

Merge remote-tracking branch 'upstream/master' into fix_6405

b5e794c

Merge branch 'master' into fix_6405

d5a175f

zimulala reviewed Jun 6, 2018

View reviewed changes

code refine

319ed7a

Merge branch 'master' into fix_6405

f43be2c

zimulala reviewed Jun 6, 2018

View reviewed changes

zimulala added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 6, 2018

shenli reviewed Jun 6, 2018

View reviewed changes

spongedu added 4 commits June 6, 2018 15:43

Merge remote-tracking branch 'upstream/master' into fix_6405

5843e9e

code refine

e7e576d

Merge branch 'fix_6405' of github.com:spongedu/tidb into fix_6405

c30b65d

Merge branch 'master' into fix_6405

ceca5ac

shenli added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Jun 6, 2018

Merge branch 'master' into fix_6405

22e0ee0

tiancaiamao approved these changes Jun 6, 2018

View reviewed changes

tiancaiamao added the all-tests-passed label Jun 7, 2018

tiancaiamao and others added 3 commits June 7, 2018 11:01

Merge branch 'master' into fix_6405

fedcf82

Merge branch 'master' into fix_6405

582a02f

Merge branch 'master' into fix_6405

27b1ca5

XuHuaiyu merged commit f56e130 into pingcap:master Jun 7, 2018

spongedu deleted the fix_6405 branch June 7, 2018 05:36

you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020

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

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

Conversation

spongedu commented May 5, 2018

shenli commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiancaiamao commented May 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shenli commented May 20, 2018

shenli commented May 20, 2018

spongedu commented May 21, 2018

zimulala commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spongedu commented Jun 6, 2018

zimulala left a comment

Choose a reason for hiding this comment

zimulala commented Jun 6, 2018

shenli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shenli commented Jun 6, 2018

shenli commented Jun 6, 2018

shenli commented Jun 7, 2018