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

*: add rollbacking logic for column type change #19531

Merged
merged 30 commits into from
Sep 15, 2020

Conversation

AilinKid
Copy link
Contributor

@AilinKid AilinKid commented Aug 27, 2020

What problem does this PR solve?

Since #19059 has supported the column type change between integers.
This PR is to support rollbacking modify column job at four states including StateNone, StateDeleteOnly, StateWriteOnly, StateWriteReorganization.

Close issue: #19119

What is changed and how it works?

How it Works:
The column type change is implemented by appending an additional column and indexes into meta in the middle state. When reorganization is finished in StateWriteReorganization state, TiDB will substitute the old ones with these additional columns and indexes. These additional columns and indexes are named as changingCol and changingIndexes in our proposal.

Because of this, rolling back modify column jobs in the middle state is much easier than other DDL jobs. At any one of the middle states, we can directly remove those changingCol and changingIndex from meta. Next, we should restore some flag in the field type of the old column like PreventNullInsertFlag. Last, don't forget to add the index table into the delete range table.

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:

Check List

Tests

  • Unit test
  • Integration test

Release note

  • add rollbacking logic for column type change

@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Aug 27, 2020
@AilinKid AilinKid added require-LGT3 Indicates that the PR requires three LGTM. type/new-feature and removed type/new-feature labels Aug 27, 2020
ddl/delete_range.go Outdated Show resolved Hide resolved
ddl/ddl_worker.go Outdated Show resolved Hide resolved
ddl/column.go Outdated Show resolved Hide resolved
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

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 1, 2020
ddl/ddl_worker_test.go Show resolved Hide resolved
@AilinKid
Copy link
Contributor Author

AilinKid commented Sep 2, 2020

/run-unit-test

1 similar comment
@AilinKid
Copy link
Contributor Author

AilinKid commented Sep 2, 2020

/run-unit-test

Signed-off-by: AilinKid <314806019@qq.com>
.
Signed-off-by: AilinKid <314806019@qq.com>
Signed-off-by: AilinKid <314806019@qq.com>
Signed-off-by: AilinKid <314806019@qq.com>
Signed-off-by: AilinKid <314806019@qq.com>
.
Signed-off-by: AilinKid <314806019@qq.com>
Signed-off-by: AilinKid <314806019@qq.com>
@AilinKid AilinKid force-pushed the refine_rollback_col_type_change branch from 13e48f3 to 44347e6 Compare September 2, 2020 09:45
@sre-bot
Copy link
Contributor

sre-bot commented Sep 3, 2020

@AilinKid
Copy link
Contributor Author

AilinKid commented Sep 3, 2020

/build

Signed-off-by: AilinKid <314806019@qq.com>
@AilinKid
Copy link
Contributor Author

/run-unit-test

@AilinKid
Copy link
Contributor Author

/run-common-test

@bb7133
Copy link
Member

bb7133 commented Sep 15, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@AilinKid merge failed.

@AilinKid
Copy link
Contributor Author

/run-all-tests

@imtbkcat
Copy link

/run-common-test

@AilinKid
Copy link
Contributor Author

/run-integration-common-test

@AilinKid
Copy link
Contributor Author

/run-tics-test

@AilinKid
Copy link
Contributor Author

/run-all-tests

@AilinKid
Copy link
Contributor Author

/run-integration-common-test

@AilinKid
Copy link
Contributor Author

/run-unit-test

3 similar comments
@AilinKid
Copy link
Contributor Author

/run-unit-test

@AilinKid
Copy link
Contributor Author

/run-unit-test

@AilinKid
Copy link
Contributor Author

/run-unit-test

@AilinKid
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@AilinKid
Copy link
Contributor Author

/run-all-tests

1 similar comment
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@AilinKid merge failed.

@AilinKid
Copy link
Contributor Author

/run-unit-test

@AilinKid
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@AilinKid merge failed.

@AilinKid
Copy link
Contributor Author

/run-all-tests

@bb7133 bb7133 merged commit b7364a3 into pingcap:master Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
require-LGT3 Indicates that the PR requires three LGTM. sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT4 The PR has already had 4 LGTM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants