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: improve compatibility for ALTER TABLE algorithms #19270

Merged
merged 11 commits into from
Aug 21, 2020

Conversation

xhebox
Copy link
Contributor

@xhebox xhebox commented Aug 18, 2020

What problem does this PR solve?

Issue Number: close #19162

ALGORITHM=x means "this or better", whereby INSTANT > INPLACE > COPY. As long the definition of supported field follows this order, the loop will always choose the best algorithm.

The comparison works due to how types are defined in the parser. Its definition follows the order default < copy < inplace < instant.

So this loop means that if there is an equal or better algorithm, it is chosen. Maybe I should have a LessOrEqual method on this type, and not to rely on the enum definition.

If the chosen algorithm is not same as the specified one, an error is returned for TiDB to report the warning. So an error may come from the better algorithm choosing strategy, which is actually a successful case.

As a result, the fail case, no matching algorithm is identified by AlgorithmTypeDefault and an error.

Check List

Tests

  • Unit test
  • Integration test

Release note

  • Improve compatibility for ALTER TABLE algorithms

@ti-srebot ti-srebot added the contribution This PR is from a community contributor. label Aug 18, 2020
@xhebox
Copy link
Contributor Author

xhebox commented Aug 18, 2020

@ti-srebot /run-unit-test

@ti-srebot
Copy link
Contributor

/run-unit-test

@xhebox xhebox requested a review from a team as a code owner August 18, 2020 16:20
@xhebox xhebox requested review from wshwsh12 and removed request for a team August 18, 2020 16:20
@djshow832 djshow832 requested review from bb7133 and removed request for wshwsh12 August 19, 2020 03:32
@bb7133 bb7133 changed the title Improve compatibility for ALTER TABLE algorithms ddl: improve compatibility for ALTER TABLE algorithms Aug 19, 2020
@bb7133
Copy link
Member

bb7133 commented Aug 19, 2020

A kind remind: please add the name of the submodule in your PR title, more details can be found here: https://github.com/pingcap/community/blob/master/contributors/commit-message-pr-style.md#pull-request-title-style

I've changed the title BTW.

ddl/ddl_algorithm.go Outdated Show resolved Hide resolved
xhebox and others added 2 commits August 19, 2020 16:43
Co-authored-by: bb7133 <bb7133@gmail.com>
Co-authored-by: djshow832 <zhangming@pingcap.com>
Copy link
Contributor

@djshow832 djshow832 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 Aug 19, 2020
@bb7133
Copy link
Member

bb7133 commented Aug 19, 2020

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Aug 19, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 19, 2020
@bb7133 bb7133 added needs-cherry-pick-4.0 require-LGT3 Indicates that the PR requires three LGTM. labels Aug 19, 2020
Copy link
Member

@wjhuang2016 wjhuang2016 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 status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Aug 19, 2020
@bb7133
Copy link
Member

bb7133 commented Aug 19, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 19, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@xhebox merge failed.

@bb7133
Copy link
Member

bb7133 commented Aug 19, 2020

/run-common-test

@xhebox
Copy link
Contributor Author

xhebox commented Aug 19, 2020

@ti-srebot /run-check_dev_2

@ti-srebot
Copy link
Contributor

/run-check_dev_2

@bb7133
Copy link
Member

bb7133 commented Aug 21, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit f851898 into pingcap:master Aug 21, 2020
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Aug 21, 2020
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #19364

jebter pushed a commit that referenced this pull request Aug 21, 2020
* cherry pick #19270 to release-4.0

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

* fix conflict

Co-authored-by: xhe <xw897002528@gmail.com>
Co-authored-by: bb7133 <bb7133@gmail.com>
@xhebox xhebox deleted the 19162 branch August 24, 2020 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. 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/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve compatibility for ALTER TABLE algorithms
5 participants