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: fix cancel drop column ddl error #8545

Merged
merged 23 commits into from
Dec 28, 2018
Merged

ddl: fix cancel drop column ddl error #8545

merged 23 commits into from
Dec 28, 2018

Conversation

ciscoxll
Copy link
Contributor

@ciscoxll ciscoxll commented Dec 2, 2018 •
edited
Loading

What problem does this PR solve?

Releate PR #8171 , The old tidb version cancels the drop of the column, which may cause the column state to be neither StatePublic nor StateNone. For the client, the column is not visible, but adding a column with the same name to the client will fail because the name of the "column" column already exists.

What is changed and how it works?

We can only cancel drop column job when the column state is on StatePublic, otherwise, cancel column and make the column state rollback to public may cause data consistency.

Check List

  • Unit test

Side effects

  • Increased code complexity

Related changes

  • Need to cherry-pick to the release branch

This change is Reviewable

crazycs520
crazycs520 reviewed Dec 5, 2018
View reviewed changes
ddl/rollingback.go Outdated Show resolved Hide resolved
crazycs520
crazycs520 reviewed Dec 5, 2018
View reviewed changes
ddl/db_test.go Outdated Show resolved Hide resolved
crazycs520
crazycs520 reviewed Dec 5, 2018
View reviewed changes
ddl/db_test.go Outdated Show resolved Hide resolved
crazycs520
crazycs520 reviewed Dec 6, 2018
View reviewed changes
ddl/rollingback.go Outdated Show resolved Hide resolved
crazycs520
crazycs520 reviewed Dec 6, 2018
View reviewed changes
Copy link
Contributor

@crazycs520 crazycs520 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Mostly LGTM, please address comment.

@ciscoxll
Copy link
Contributor Author

ciscoxll commented Dec 7, 2018

@crazycs520 PTAL

@ciscoxll
Copy link
Contributor Author

@crazycs520 PTAL

winkyao
winkyao reviewed Dec 11, 2018
View reviewed changes
ddl/rollingback.go Outdated Show resolved Hide resolved
winkyao
winkyao reviewed Dec 11, 2018
View reviewed changes
ddl/db_test.go Outdated Show resolved Hide resolved
crazycs520
crazycs520 reviewed Dec 11, 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.

@ciscoxll ciscoxll added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 11, 2018
crazycs520
crazycs520 reviewed Dec 11, 2018
View reviewed changes
ddl/db_test.go Outdated Show resolved Hide resolved
crazycs520
crazycs520 reviewed Dec 11, 2018
View reviewed changes
util/admin/admin.go Outdated Show resolved Hide resolved
zimulala
zimulala reviewed Dec 18, 2018
View reviewed changes
util/admin/admin.go Outdated Show resolved Hide resolved
ddl/rollingback.go Outdated Show resolved Hide resolved
winkyao
winkyao reviewed Dec 18, 2018
View reviewed changes
ddl/ddl_worker_test.go Outdated Show resolved Hide resolved
@shenli
Copy link
Member

shenli commented Dec 26, 2018

Please resolve the conflicts.

winkyao
winkyao reviewed Dec 26, 2018
View reviewed changes
ddl/ddl_worker_test.go Outdated Show resolved Hide resolved
winkyao
winkyao reviewed Dec 26, 2018
View reviewed changes
ddl/rollingback.go Outdated Show resolved Hide resolved
winkyao
winkyao reviewed Dec 27, 2018
View reviewed changes
ddl/column.go Outdated Show resolved Hide resolved
zimulala
zimulala reviewed Dec 27, 2018
View reviewed changes
ddl/rollingback.go Outdated Show resolved Hide resolved
winkyao
winkyao reviewed Dec 27, 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

@ciscoxll ciscoxll added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 27, 2018
zimulala
zimulala approved these changes Dec 28, 2018
View reviewed changes
Copy link
Contributor

@zimulala zimulala 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
Copy link
Contributor

/run-all-tests

@ciscoxll ciscoxll added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Dec 28, 2018
@ciscoxll ciscoxll merged commit eaeaca7 into pingcap:master Dec 28, 2018
@ciscoxll ciscoxll deleted the cancel-drop-column branch December 28, 2018 03:28
AndrewDi pushed a commit to AndrewDi/tidb that referenced this pull request Dec 28, 2018
@ciscoxll @AndrewDi
ddl: fix cancel drop column ddl error (pingcap#8545)
d96b806
This was referenced Dec 28, 2018
Merged
Merged
yu34po pushed a commit to yu34po/tidb that referenced this pull request Jan 2, 2019
@ciscoxll @yu34po
ddl: fix cancel drop column ddl error (pingcap#8545)
2419899
@ciscoxll ciscoxll mentioned this pull request Feb 19, 2019
Merged
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019
@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

@crazycs520 crazycs520 crazycs520 left review comments

@winkyao winkyao winkyao left review comments

@zimulala zimulala zimulala approved these changes

Assignees
No one assigned
Labels
contribution This PR is from a community contributor. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ciscoxll @shenli @crazycs520 @winkyao @zimulala @you06 @sre-bot