plan/validator: return an error when alter auto_incerment (#3041) #3072

bobotu · 2017-04-17T14:39:01Z

Refer to #3041

CLAassistant · 2017-04-17T14:39:07Z

All committers have signed the CLA.

hanfei1991 · 2017-04-17T14:43:00Z

Thanks. Please sign the CLA

hanfei1991 · 2017-04-18T07:52:44Z

plan/validator.go

@@ -290,3 +301,5 @@ func checkDuplicateColumnName(indexColNames []*ast.IndexColName) error {
 	}
 	return nil
 }
+
+const codeAlterAutoID terror.ErrCode = 2


add error code to planbuilder.go

tiancaiamao

PTAL @zimulala

tiancaiamao · 2017-04-18T10:32:40Z

plan/validator.go

@@ -271,6 +271,16 @@ func (v *validator) checkAlterTableGrammar(stmt *ast.AlterTableStmt) {
 			default:
 				// Nothing to do now.
 			}
+		case ast.AlterTableOption:
+			for _, opt := range spec.Options {
+				switch opt.Tp {


why not if else ?

I don't know if there will be any code about other TableOption.Tp in the future, so I use switch instead of if-else

I think it's enough to use if-else here. We may remove it when we intend to support this type in the future. And if we need to handle other types in the future, then we can update it at that time.

I'll change the code to use if-else

hanfei1991 · 2017-04-18T10:46:02Z

plan/planbuilder.go

@@ -45,6 +46,7 @@ const (
 	CodeAmbiguous       terror.ErrCode = 1052
 	CodeUnknownColumn   terror.ErrCode = 1054
 	CodeWrongArguments  terror.ErrCode = 1210
+	CodeAlterAutoID     terror.ErrCode = 2


move it to line 46 and set it to 3

shenli · 2017-04-18T11:08:11Z

plan/planbuilder.go

@@ -36,6 +36,7 @@ var (
 	ErrUnknownColumn        = terror.ClassOptimizerPlan.New(CodeUnknownColumn, "Unknown column '%s' in '%s'")
 	ErrWrongArguments       = terror.ClassOptimizerPlan.New(CodeWrongArguments, "Incorrect arguments to EXECUTE")
 	ErrAmbiguous            = terror.ClassOptimizerPlan.New(CodeAmbiguous, "Column '%s' in field list is ambiguous")
+	ErrAlterAutoID          = terror.ClassAutoid.New(CodeAlterAutoID, "No support for setting auto_increment using alter_table")


Is there a MySQL standard error code for this error?

I have searched in MySQL 8.0 Server Error Codes and Messages and got these error mentioned AUTO_INCREMENT or AUTOINC. In my opinon there have no appropriate MySQL standard error code to describe this error. Because this problem caused by the implementation of autoid module, so I put this error under ClassAutoid.

Error: 1164 SQLSTATE: 42000 (ER_TABLE_CANT_HANDLE_AUTO_INCREMENT)
Message: The used table type doesn't support AUTO_INCREMENT columns

Error: 1467 SQLSTATE: HY000 (ER_AUTOINC_READ_FAILED)
Message: Failed to read auto-increment value from storage engine

Error: 1569 SQLSTATE: HY000 (ER_DUP_ENTRY_AUTOINCREMENT_CASE)
Message: ALTER TABLE causes auto_increment resequencing, resulting in duplicate entry '%s' for key '%s'

Error: 1671 SQLSTATE: HY000 (ER_BINLOG_UNSAFE_AUTOINC_COLUMNS)
Message: Statement is unsafe because it invokes a trigger or a stored function that inserts into an AUTO_INCREMENT column. Inserted values cannot be logged correctly.

Error: 1722 SQLSTATE: HY000 (ER_BINLOG_UNSAFE_WRITE_AUTOINC_SELECT)
Message: Statements writing to a table with an auto-increment column after selecting from another table are unsafe because the order in which rows are retrieved determines what (if any) rows will be written. This order cannot be predicted and may differ on master and the slave.

Error: 1723 SQLSTATE: HY000 (ER_BINLOG_UNSAFE_CREATE_SELECT_AUTOINC)
Message: CREATE TABLE... SELECT... on a table with an auto-increment column is unsafe because the order in which rows are retrieved by the SELECT determines which (if any) rows are inserted. This order cannot be predicted and may differ on master and the slave.

Error: 1727 SQLSTATE: HY000 (ER_BINLOG_UNSAFE_AUTOINC_NOT_FIRST)
Message: INSERT into autoincrement field which is not the first part in the composed primary key is unsafe.

Error: 1854 SQLSTATE: HY000 (ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_AUTOINC)
Message: Adding an auto-increment column requires a lock

Error: 1869 SQLSTATE: HY000 (ER_AUTO_INCREMENT_CONFLICT)
Message: Auto-increment value in UPDATE conflicts with internally generated values

Error: 3109 SQLSTATE: HY000 (ER_GENERATED_COLUMN_REF_AUTO_INC)
Message: Generated column '%s' cannot refer to auto-increment column.

zimulala · 2017-04-19T02:35:40Z

Please add some tests.

bobotu · 2017-04-19T04:56:13Z

There is already a test in this RP which check the returned error message when user commit a alter table t auto_increment=1. I don't know is there have any other tests should I adde. Please tell me and I'll add it. @zimulala

zimulala · 2017-04-19T05:12:26Z

Sorry, I just saw it. @bobotu

zimulala · 2017-04-19T08:39:39Z

plan/validator_test.go

@@ -71,6 +71,7 @@ func (s *testValidatorSuite) TestValidator(c *C) {
 			errors.New("[schema:1068]Multiple primary key defined")},
 		{"create table t(c1 int not null, c2 int not null, primary key(c1), primary key(c2))", true,
 			errors.New("[schema:1068]Multiple primary key defined")},
+		{"alter table t auto_increment=1", true, errors.New("[autoid:3]No support for setting auto_increment using alter_table")},


Add the test like alter table t add column c int auto_increment key, auto_increment=10;.

I have added two tests. I think validator should return a nil in the validation of alter table t add column c int auto_increment key, because there are existed guard to avoid user commit something like alter table t add column c int auto_increment key or alter table t modify c int auto_increment key in ddl module.

zimulala · 2017-04-19T12:51:42Z

LGTM
PTAL @tiancaiamao @coocood

hanfei1991

LGTM

plan/validator: return an error when alter auto_incerment (pingcap#3041)

b4af403

hanfei1991 added the contribution This PR is from a community contributor. label Apr 17, 2017

Merge branch 'master' into iss3041

9978002

hanfei1991 reviewed Apr 18, 2017

View reviewed changes

tuzi_li added 3 commits April 18, 2017 17:47

fix: update according to review

68f7902

Merge branch 'master' into iss3041

f5ed6b4

Merge branch 'master' into iss3041

97cdaf8

tiancaiamao reviewed Apr 18, 2017

View reviewed changes

hanfei1991 reviewed Apr 18, 2017

View reviewed changes

tuzi_li added 2 commits April 18, 2017 19:06

fix: update according to review

f7c36d6

Merge branch 'master' into iss3041

8483010

shenli reviewed Apr 18, 2017

View reviewed changes

fix: use if instead of switch according to review

e1a51fa

zimulala reviewed Apr 19, 2017

View reviewed changes

fix: add some tests

e326866

hanfei1991 approved these changes Apr 19, 2017

View reviewed changes

hanfei1991 added 2 commits April 19, 2017 21:59

Merge branch 'master' into iss3041

2ddfc2b

Merge branch 'master' into iss3041

ffb41e5

hanfei1991 merged commit 7b381c0 into pingcap:master Apr 19, 2017

bobotu deleted the iss3041 branch April 21, 2017 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plan/validator: return an error when alter auto_incerment (#3041) #3072

plan/validator: return an error when alter auto_incerment (#3041) #3072

bobotu commented Apr 17, 2017

CLAassistant commented Apr 17, 2017 •

edited

Loading

hanfei1991 commented Apr 17, 2017

hanfei1991 Apr 18, 2017

tiancaiamao left a comment

tiancaiamao Apr 18, 2017

bobotu Apr 18, 2017

tiancaiamao Apr 19, 2017

zimulala Apr 19, 2017

bobotu Apr 19, 2017

hanfei1991 Apr 18, 2017

shenli Apr 18, 2017

bobotu Apr 18, 2017

zimulala commented Apr 19, 2017

bobotu commented Apr 19, 2017

zimulala commented Apr 19, 2017 •

edited

Loading

zimulala Apr 19, 2017

bobotu Apr 19, 2017

zimulala commented Apr 19, 2017

hanfei1991 left a comment

plan/validator: return an error when alter auto_incerment (#3041) #3072

plan/validator: return an error when alter auto_incerment (#3041) #3072

Conversation

bobotu commented Apr 17, 2017

CLAassistant commented Apr 17, 2017 • edited Loading

hanfei1991 commented Apr 17, 2017

Choose a reason for hiding this comment

tiancaiamao 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

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

zimulala commented Apr 19, 2017

bobotu commented Apr 19, 2017

zimulala commented Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimulala commented Apr 19, 2017

hanfei1991 left a comment

Choose a reason for hiding this comment

CLAassistant commented Apr 17, 2017 •

edited

Loading

zimulala commented Apr 19, 2017 •

edited

Loading