-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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: fix foreign key and check constraint verify records under async commit #48524
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/ok-to-test |
Hi @jiyfhust. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #48524 +/- ##
================================================
+ Coverage 71.3984% 80.5818% +9.1833%
================================================
Files 1403 2676 +1273
Lines 406971 766530 +359559
================================================
+ Hits 290571 617684 +327113
- Misses 96445 124837 +28392
- Partials 19955 24009 +4054
Flags with carried forward coverage won't be shown. Click here to find out more.
|
/retest |
/cc @CbcWestwolf @mjonss @Benjamin2037 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to add a test case showing the effect of this PR?
I do not have good idea to reproduce the steps to show the effects. But as async commit logic, we should wait before all txns use the old schema to finish then we can verify records, just as you known that exchange partition handle it the same way. |
em... Who could help for this pr? |
1 similar comment
em... Who could help for this pr? |
For example, modify column null to not null, also use delayAsyncCommit. |
@jiyfhust: 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. |
The case:
Is there some thing wrong above? right? not right? |
@jiyfhust: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Related #23363. |
PR needs rebase. 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. |
@jiyfhust: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge this PR! Would you mind adding a test for it?
Ref https://github.com/pingcap/tidb/pull/56349/files, as I've tested, the test and failpoint in that PR should work.
It's also preferred to add more similar tests related to other situations (like check constraints
, modify column
...).
I'm pleased for it,we can also work together to make this pr more complete. |
My poor virtual host with only 4G memory has diffcuity to reproduce the case, expecially to run txn tests you added. For others, maybe case like this(this is only for note):
|
What problem does this PR solve?
Issue Number: close #48297
Problem Summary:
What is changed and how it works?
Check List
Tests
Code review instead of tests.
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.