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.

Sign up for GitHub

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

Jump to bottom

ddl, executor: fix rename table compatibility #8709

Merged
merged 4 commits into from
Dec 22, 2018
Merged

ddl, executor: fix rename table compatibility #8709

merged 4 commits into from
Dec 22, 2018

Conversation

zimulala
Copy link
Contributor

@zimulala zimulala commented Dec 17, 2018 •
edited
Loading

What problem does this PR solve?

The results of alter table old_name rename new_name and rename table old_name to new_name are different.

mysql> show variables like "%lower_case_table_names%";
+------------------------+-------+
| Variable_name          | Value |
+------------------------+-------+
| lower_case_table_names | 2     |
+------------------------+-------+
1 row in set (0.01 sec)
mysql> show tables;
+--------------+
| Tables_in_db |
+--------------+
| t            |
| t1           |
+--------------+
2 rows in set (0.00 sec)

mysql>  rename table t to T;
ERROR 1050 (42S01): Table 'T' already exists
mysql>  alter table t rename T;
Query OK, 0 rows affected (0.00 sec)

mysql> show tables;
+--------------+
| Tables_in_db |
+--------------+
| t            |
| t1           |
+--------------+
2 rows in set (0.00 sec)

What is changed and how it works?

We treat alter table old_name rename new_name and rename table old_name to new_name differently.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Related changes

  • Need to cherry-pick to the release branch

This change is Reviewable

jackysp
jackysp reviewed Dec 18, 2018
View reviewed changes
executor/ddl.go
@@ -122,7 +122,8 @@ func (e *DDLExec) executeRenameTable(s *ast.RenameTableStmt) error {
}
oldIdent := ast.Ident{Schema: s.OldTable.Schema, Name: s.OldTable.Name}
newIdent := ast.Ident{Schema: s.NewTable.Schema, Name: s.NewTable.Name}
err := domain.GetDomain(e.ctx).DDL().RenameTable(e.ctx, oldIdent, newIdent)
isAlterTable := false
err := domain.GetDomain(e.ctx).DDL().RenameTable(e.ctx, oldIdent, newIdent, isAlterTable)
Copy link
Member

Choose a reason for hiding this comment

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

Why not pass false directly?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes it more readable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it is necessary.

jackysp
jackysp reviewed Dec 18, 2018
View reviewed changes
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.

It's an interesting behavior on MySQL.

winkyao
winkyao reviewed Dec 18, 2018
View reviewed changes
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

reset LGTM

@zimulala
Copy link
Contributor Author

PTAL @jackysp @winkyao

1 similar comment
@zimulala
Copy link
Contributor Author

PTAL @jackysp @winkyao

jackysp
jackysp reviewed Dec 21, 2018
View reviewed changes
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

crazycs520
crazycs520 reviewed Dec 21, 2018
View reviewed changes
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

winkyao
winkyao approved these changes Dec 21, 2018
View reviewed changes
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp
Copy link
Member

jackysp commented Dec 21, 2018

/run-all-tests

@zz-jason zz-jason added status/all tests passed status/LGT3 The PR has already had 3 LGTM. labels Dec 22, 2018
@ngaut ngaut merged commit ef7082d into pingcap:master Dec 22, 2018
@zimulala zimulala deleted the rename-table branch December 25, 2018 06:53
zimulala added a commit to zimulala/tidb that referenced this pull request Dec 25, 2018
@zimulala
ddl, executor: fix rename table compatibility (pingcap#8709)
a143321
@zimulala zimulala mentioned this pull request Dec 25, 2018
Merged
zimulala added a commit to zimulala/tidb that referenced this pull request Dec 25, 2018
@zimulala
ddl, executor: fix rename table compatibility (pingcap#8709)
4546161
@zimulala zimulala mentioned this pull request Dec 25, 2018
Merged
zimulala added a commit that referenced this pull request Dec 25, 2018
@zimulala
ddl, executor: fix rename table compatibility (#8709) (#8808)
fae6021
ngaut pushed a commit that referenced this pull request Dec 25, 2018
@zimulala @ngaut
ddl, executor: fix rename table compatibility (#8709) (#8809)
cceab82
yu34po pushed a commit to yu34po/tidb that referenced this pull request Jan 2, 2019
@zimulala @yu34po
ddl, executor: fix rename table compatibility (pingcap#8709)
27201e9
tiancaiamao added a commit that referenced this pull request Jan 2, 2019
@tiancaiamao
Revert "ddl, executor: fix rename table compatibility (#8709) (#8809)"
4eedd6d 
This reverts commit cceab82.
@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
Reviewers

@jackysp jackysp jackysp left review comments

@crazycs520 crazycs520 crazycs520 left review comments

@winkyao winkyao winkyao approved these changes

Assignees
No one assigned
Labels
sig/sql-infra SIG: SQL Infra status/LGT3 The PR has already had 3 LGTM. type/compatibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@zimulala @jackysp @winkyao @crazycs520 @ngaut @zz-jason @you06