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.

Sign up for GitHub

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

Jump to bottom

bindinfo: avoid duplicate bindings caused by concurrent baseline capture #22182

Merged
merged 3 commits into from
Jan 8, 2021
Merged

bindinfo: avoid duplicate bindings caused by concurrent baseline capture #22182

merged 3 commits into from
Jan 8, 2021

Conversation

eurekaka
Copy link
Contributor

@eurekaka eurekaka commented Jan 5, 2021

What problem does this PR solve?

Issue Number: close #22178

Problem Summary:

Duplicate bindings for a same original_sql.

What is changed and how it works?

What's Changed:

Call CreateBindRecord instead of AddBindRecord in CaptureBaselines.

How it Works:

AddBindRecord only checks the duplicate in binding cache, while CreateBindRecord would remove duplicate bindings in mysql.bind_info unconditionally.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test

Side effects
N/A

Release note

  • Avoid duplicate bindings caused by concurrent baseline capture

@eurekaka eurekaka requested a review from a team as a code owner January 5, 2021 08:32
@eurekaka eurekaka requested review from winoros, qw4990 and Reminiscent and removed request for a team January 5, 2021 08:32
@github-actions github-actions bot added the sig/planner SIG: Planner label Jan 5, 2021
Reminiscent
Reminiscent reviewed Jan 7, 2021
View reviewed changes
Copy link
Contributor

@Reminiscent Reminiscent 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 Jan 7, 2021
@Reminiscent
Copy link
Contributor

I have a question about AddBindRecord. It seems when the AddBindRecord finds the duplicate binding, it will remove it(See the code). Why does it happen here? So how do we distinguish when to use AddBindRecord or CreateBindRecord?

@eurekaka
Copy link
Contributor Author

eurekaka commented Jan 7, 2021

@Reminiscent AddBindRecord only checks the binding cache for duplicates, if there is a concurrent capture in another tidb instance, they both would see no existing binding in the cache, the problem is that, there is a time window for the bindings to be synchronized from mysql.bind_info to the cache.

@eurekaka
Copy link
Contributor Author

eurekaka commented Jan 7, 2021

AddBindRecord is only used for baseline evolution now after applying this PR, while I think it is problematic as well, but baseline evolution is not generally available so I didn't fix it.

qw4990
qw4990 approved these changes Jan 8, 2021
View reviewed changes
Copy link
Contributor

@qw4990 qw4990 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 removed the status/LGT1 Indicates that a PR has LGTM 1. label Jan 8, 2021
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Jan 8, 2021
@qw4990
Copy link
Contributor

qw4990 commented Jan 8, 2021

/merge

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

Your auto merge job has been accepted, waiting for:

  • 22219

@ti-srebot
Copy link
Contributor

/run-all-tests

@qw4990
Copy link
Contributor

qw4990 commented Jan 8, 2021

/run-all-tests

@ti-srebot ti-srebot merged commit 0a4ec0d into pingcap:master Jan 8, 2021
@ti-srebot ti-srebot mentioned this pull request Jan 8, 2021
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #22295

@eurekaka eurekaka deleted the capture branch January 8, 2021 06:26
eurekaka added a commit to ti-srebot/tidb that referenced this pull request Jan 8, 2021
@eurekaka
cherry pick pingcap#22182 to release-4.0
0af3c50 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
eurekaka added a commit that referenced this pull request Jan 8, 2021
@ti-srebot @eurekaka
cherry pick #22182 to release-4.0 (#22295)
83603c8 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

Co-authored-by: Kenan Yao <cauchy1992@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Reminiscent Reminiscent Reminiscent left review comments

@ti-srebot ti-srebot ti-srebot approved these changes

@qw4990 qw4990 qw4990 approved these changes

@winoros winoros Awaiting requested review from winoros

Assignees
No one assigned
Labels
epic/sql-plan-management sig/planner SIG: Planner status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

duplicate bindings caused by concurrent baseline capture
4 participants
@eurekaka @Reminiscent @qw4990 @ti-srebot