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: check if the column name already exists when we rename the column name #6284

Merged
merged 7 commits into from
Apr 16, 2018

Conversation

zimulala
Copy link
Contributor

If we want to rename the column name, we need to check whether it already exists.

@zimulala zimulala added type/bugfix This PR fixes a bug. DDL labels Apr 13, 2018
@zimulala
Copy link
Contributor Author

/run-all-test

@@ -1350,6 +1350,14 @@ func (d *ddl) getModifiableColumnJob(ctx sessionctx.Context, ident ast.Ident, or
if col == nil {
return nil, infoschema.ErrColumnNotExists.GenByArgs(originalColName, ident.Name)
}
newColName := specNewColumn.Name.Name
// If we want to rename the column name, we need to check whether it already exists.
if newColName.L != originalColName.L {
Copy link
Member

@jackysp jackysp Apr 13, 2018

Choose a reason for hiding this comment

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

Why we need to check it twice? Both in ddl_api.go and column.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.

Because it is possible to have two identical rename operations in the DDL job list at the same time.

Copy link
Member

Choose a reason for hiding this comment

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

So the go test coverage will cover the both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

@shenli
Copy link
Member

shenli commented Apr 15, 2018

/run-all-tests

@zimulala
Copy link
Contributor Author

PTAL @jackysp @shenli

@jackysp
Copy link
Member

jackysp commented Apr 16, 2018

LGTM

@zimulala zimulala added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 16, 2018
@zimulala
Copy link
Contributor Author

PTAL @shenli

@shenli
Copy link
Member

shenli commented Apr 16, 2018

LGTM

@shenli shenli added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 16, 2018
@zimulala zimulala merged commit 4024e88 into pingcap:master Apr 16, 2018
@zimulala zimulala deleted the rename-col branch April 16, 2018 06:04
ciscoxll added a commit that referenced this pull request Apr 16, 2018
@you06 you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020
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/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants