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: fix a bug when adding an index to a generated column with an expression error #9216

Merged
merged 4 commits into from
Feb 12, 2019

Conversation

zimulala
Copy link
Contributor

…ression error

What problem does this PR solve?

Fix the #9214

What is changed and how it works?

If we encounter a decoding error when adding an index, then we directly return the error of adding this index.

Check List

Tests

  • Unit test

Related changes

  • Need to cherry-pick to the release branch

@zimulala
Copy link
Contributor Author

/run-all-tests

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 12, 2019
@zimulala
Copy link
Contributor Author

PTAL @winkyao @ciscoxll @crazycs520

Copy link
Contributor

@xiekeyi98 xiekeyi98 left a comment

Choose a reason for hiding this comment

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

rest,LGTM.

ddl/ddl.go Outdated
@@ -76,6 +76,7 @@ var (
errInvalidWorker = terror.ClassDDL.New(codeInvalidWorker, "invalid worker")
// errNotOwner means we are not owner and can't handle DDL jobs.
errNotOwner = terror.ClassDDL.New(codeNotOwner, "not Owner")
errCantDecodeIndex = terror.ClassDDL.New(codeCantDecodeIndex, "can't decode index value, because %s")
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Cant/Cannnot

Copy link
Contributor Author

@zimulala zimulala Feb 12, 2019

Choose a reason for hiding this comment

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

MySQL uses it to name the error like 'ErrCantDropFieldOrKey'.

Copy link
Contributor

@xiekeyi98 xiekeyi98 Feb 12, 2019

Choose a reason for hiding this comment

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

But in TiDB, I think we use Cannot is better.
F.Y.R:

xiekeyi@ubuntu:~/gopath/src/github.com/pingcap/tidb$ grep -rn "ErrCannot"
匹配到二进制文件 bin/tidb-server
匹配到二进制文件 cmd/explaintest/explain_test
executor/simple_test.go:185:	c.Assert(terror.ErrorEqual(err, executor.ErrCannotUser.GenWithStackByArgs("DROP USER", "")), IsTrue, Commentf("err %v", err))
executor/simple_test.go:194:	c.Assert(terror.ErrorEqual(err, executor.ErrCannotUser.GenWithStackByArgs("DROP USER", "")), IsTrue, Commentf("err %v", err))
executor/simple.go:243:		return ErrCannotUser.GenWithStackByArgs("ALTER USER", strings.Join(failedUsers, ","))
executor/simple.go:302:		return ErrCannotUser.GenWithStackByArgs("DROP USER", strings.Join(failedUsers, ","))
executor/errors.go:43:	ErrCannotUser                  = terror.ClassExecutor.New(mysql.ErrCannotUser, mysql.MySQLErrName[mysql.ErrCannotUser])
executor/errors.go:57:		mysql.ErrCannotUser:                  mysql.ErrCannotUser,
vendor/github.com/pingcap/parser/mysql/errname.go:233:	ErrCannotAddForeign:                         "Cannot add foreign key constraint",
vendor/github.com/pingcap/parser/mysql/errname.go:414:	ErrCannotUser:                               "Operation %s failed for %.256s",
vendor/github.com/pingcap/parser/mysql/errname.go:746:	ErrCannotLoadFromTableV2:                                 "Cannot load from %s.%s. The table is probably corrupted",
vendor/github.com/pingcap/parser/mysql/errcode.go:235:	ErrCannotAddForeign                                             = 1215
vendor/github.com/pingcap/parser/mysql/errcode.go:416:	ErrCannotUser                                                   = 1396
vendor/github.com/pingcap/parser/mysql/errcode.go:748:	ErrCannotLoadFromTableV2                                        = 1728
kv/kv.go:97:	// v must NOT be nil or empty, otherwise it returns ErrCannotSetNilValue.
kv/error.go:55:	// ErrCannotSetNilValue is the error when sets an empty value.
kv/error.go:56:	ErrCannotSetNilValue = terror.ClassKV.New(codeCantSetNilValue, "can not set nil value")
kv/memdb_buffer.go:91:		return errors.Trace(ErrCannotSetNilValue)
infoschema/infoschema.go:39:	// ErrCannotAddForeign returns for foreign key exists.
infoschema/infoschema.go:40:	ErrCannotAddForeign = terror.ClassSchema.New(codeCannotAddForeign, "Cannot add foreign key constraint")
infoschema/infoschema.go:329:		codeCannotAddForeign:    mysql.ErrCannotAddForeign,
util/admin/admin.go:92:			return ErrCannotCancelDDLJob.GenWithStackByArgs(id)
util/admin/admin.go:96:			return ErrCannotCancelDDLJob.GenWithStackByArgs(id)
util/admin/admin.go:102:			return ErrCannotCancelDDLJob.GenWithStackByArgs(id)
util/admin/admin.go:743:	// ErrCannotCancelDDLJob returns when cancel a almost finished ddl job, because cancel in now may cause data inconsistency.
util/admin/admin.go:744:	ErrCannotCancelDDLJob = terror.ClassAdmin.New(codeCannotCancelDDLJob, "This job:%v is almost finished, can't be cancelled now")
ddl/db_test.go:461:			err1 := admin.ErrCannotCancelDDLJob.GenWithStackByArgs(jobID)
ddl/db_test.go:615:			c.Assert(checkErr.Error(), Equals, admin.ErrCannotCancelDDLJob.GenWithStackByArgs(jobID).Error())
ddl/db_test.go:1022:			c.Assert(checkErr.Error(), Equals, admin.ErrCannotCancelDDLJob.GenWithStackByArgs(jobID).Error())
ddl/ddl_worker_test.go:375:		{act: model.ActionDropColumn, jobIDs: []int64{firstID + 13}, cancelRetErrs: []error{admin.ErrCannotCancelDDLJob.GenWithStackByArgs(firstID + 13)}, cancelState: model.StateDeleteOnly, ddlRetErr: err},
ddl/ddl_worker_test.go:376:		{act: model.ActionDropColumn, jobIDs: []int64{firstID + 14}, cancelRetErrs: []error{admin.ErrCannotCancelDDLJob.GenWithStackByArgs(firstID + 14)}, cancelState: model.StateWriteOnly, ddlRetErr: err},
ddl/ddl_worker_test.go:377:		{act: model.ActionDropColumn, jobIDs: []int64{firstID + 15}, cancelRetErrs: []error{admin.ErrCannotCancelDDLJob.GenWithStackByArgs(firstID + 15)}, cancelState: model.StateWriteReorganization, ddlRetErr: err},
匹配到二进制文件 ddl/.ddl_api.go.swp
ddl/ddl_api.go:765:			return infoschema.ErrCannotAddForeign
ddl/ddl_api.go:847:					return nil, infoschema.ErrCannotAddForeign
ddl/ddl_api.go:870:				return nil, infoschema.ErrCannotAddForeign

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh,I am sorry for that.
Indeed, cant is more used.

xiekeyi@ubuntu:~/gopath/src/github.com/pingcap/tidb$ grep -rn  "ErrCant" | wc -l
130
xiekeyi@ubuntu:~/gopath/src/github.com/pingcap/tidb$ grep -rn  "ErrCannot" | wc -l
36
xiekeyi@ubuntu:~/gopath/src/github.com/pingcap/tidb$ 
···

ddl/ddl.go Outdated Show resolved Hide resolved
@crazycs520 crazycs520 changed the title ddl: fix a bug when adding an index to a generated column with an exp… ddl: fix a bug when adding an index to a generated column with an expression error Feb 12, 2019
Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

LGTM

@crazycs520 crazycs520 added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 12, 2019
@codecov-io
Copy link

codecov-io commented Feb 12, 2019

Codecov Report

Merging #9216 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9216      +/-   ##
==========================================
+ Coverage   67.22%   67.23%   +<.01%     
==========================================
  Files         371      371              
  Lines       77271    77271              
==========================================
+ Hits        51948    51952       +4     
+ Misses      20682    20680       -2     
+ Partials     4641     4639       -2
Impacted Files Coverage Δ
ddl/ddl.go 89.53% <ø> (ø) ⬆️
ddl/index.go 80.45% <100%> (+1.13%) ⬆️
store/tikv/lock_resolver.go 41.7% <0%> (-0.95%) ⬇️
executor/join.go 78.38% <0%> (-0.53%) ⬇️
executor/distsql.go 72.53% <0%> (-0.47%) ⬇️
infoschema/infoschema.go 77.63% <0%> (+1.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8431d11...f1c241f. Read the comment docs.

Copy link
Contributor

@xiekeyi98 xiekeyi98 left a comment

Choose a reason for hiding this comment

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

LGTM.

@xiekeyi98 xiekeyi98 added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Feb 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/sql-infra SIG: SQL Infra status/LGT3 The PR has already had 3 LGTM. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants