*: fix check constraintInfo state change #44455
Conversation
Co-authored-by: CbcWestwolf <1004626265@qq.com>
|
Skipping CI for Draft Pull Request. |
|
/test all |
|
/test all |
|
/test check_dev_2 |
|
@fzzf678: The specified target(s) for
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test check-dev2 |
ddl/constraint.go
Outdated
| ver, err = updateVersionAndTableInfoWithCheck(d, t, job, tblInfo, originalState != constraintInfoInMeta.State) | ||
| case model.StateWriteReorganization: | ||
| skipCheck := false | ||
| failpoint.Inject("mockVerifyRemainDataSuccess", func(val failpoint.Value) { |
There was a problem hiding this comment.
Maybe we can put it to verifyRemainRecordsForCheckConstraint, and if mockVerifyRemainDataSuccess is ture, we can return this function. Then we needn't add skipCheck.
| job.SchemaState = model.StateWriteReorganization | ||
| constraintInfoInMeta.State = model.StateWriteReorganization | ||
| ver, err = updateVersionAndTableInfoWithCheck(d, t, job, tblInfo, originalState != constraintInfoInMeta.State) | ||
| case model.StateWriteReorganization: |
There was a problem hiding this comment.
Why we add a new state for this operation？
There was a problem hiding this comment.
Because I want to make sure that the state of all tidb nodes is changed to StateWriteOnly before doing remaining data verification, so that no data is written that violates the check constraint.
If doing remaining data verification in StateWriteOnly state, the state of some nodes may be still in StateNone ( am i right?), so even if the remaining data verification passes, there are still StateNone nodes that can write data that violates the check constraint. Hence I added a state to ensure that all nodes change at least to StateWriteOnly before verifying the data. Is this right or not, please give some advice, thks
There was a problem hiding this comment.
Ok, there was no PR description before, I don't know why it suddenly added a status, so ask.
There was a problem hiding this comment.
Seems here we missed delete-only state, that is before writeonly state. write reorg means will reorg table data.
|
/cc @wjhuang2016 PTAL |
|
@fzzf678: GitHub didn't allow me to request PR reviews from the following users: PTAL. Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test check-dev2 |
|
@fzzf678: The specified target(s) for
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test check-dev2 |
| dbInfo, tblInfo, constraintInfo, enforced, err := checkAlterCheckConstraint(t, job) | ||
| if err != nil { | ||
| return ver, errors.Trace(err) | ||
| } | ||
|
|
||
| // enforced will fetch table data and check the constraint. | ||
| if constraintInfo.Enforced != enforced && enforced { | ||
| err = w.verifyRemainRecordsForCheckConstraint(dbInfo, tblInfo, constraintInfo, job) |
There was a problem hiding this comment.
In the check constraint, when changing from not enforced to enforced, remaining data verification is required. All nodes in tidb may have two states, enforced and not enforced.If the remaining data verification is carried out directly, data that violates the check constraint may also be written after passing. Therefore, state changes are added to ensure that all nodes have reached the enforced state before remaining data verification.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CbcWestwolf, TonsnakeLin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
|
/retest |
What problem does this PR solve?
Issue Number: ref #41711
Problem Summary:
What is changed and how it works?
In the process of changing the schemaState of the check constraint, it is stateNone- > stateWriteOnly - > statePublic. The remaining data will be verified during
sateWriteOnly. At this time, if there is a tidb node still instateNonestate, it is possible to insert data that violates the check constraint.To avoid this problem, I added a
stateWriteReorgstate after thestateWriteOnlystate, where the remaining data is checked. Make sure other tidb nodes are at least instateWriteOnlystateCheck List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.