-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
txn: fix the panic when memdb grows beyond quota #39621
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
Signed-off-by: ekexium <[email protected]>
57071fe
to
8e727e4
Compare
/build |
Signed-off-by: ekexium <[email protected]>
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.
LGTM.
@gengliqi Please help confirm whether the issue is fixed. Thanks.
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.
My test result shows this PR has fixed #39611
@gengliqi: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments. 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 ti-community-infra/tichi repository. |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 446b779
|
/run-unit-test |
/run-unit-test |
In response to a cherrypick label: new pull request created: #39640. |
TiDB MergeCI notify🔴 Bad News! New failing [2] after this pr merged.
|
Signed-off-by: ekexium [email protected]
What problem does this PR solve?
Issue Number: close #39611
Problem Summary:
The sequence of events that leads to the panic is:
membuffer.set -> enlarge -> onMemChange -> memory tracker decides to panic -> deferred releasing current staging buffer in AddRecord -> revert to last checkpoint
When reverting to last checkpoint, memdb iterates over the nodes backwards, here is why it panics:
The latest block of the arena is in an intermediate state: it is just allocated but no data has been written yet. So its length is 0. While during the iteration we assume the length cannot be 0.
What is changed and how it works?
Don't let memdb call the callback when the block is in intermediate state. Instead call it after any content is inserted after a new block is allocated.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.