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

planner: DELETE cannot delete data in some cases when the database name is capitalized (#21202) #21205

Merged
merged 2 commits into from
Nov 23, 2020

Conversation

ti-srebot
Copy link
Contributor

cherry-pick #21202 to release-3.0


What problem does this PR solve?

Issue Number: close #21200

Problem Summary:

The CurrentDB in session vars is stored the original string representation. (i.e. if we create the database name as tEsT1, the CurrentDB would be tEsT1.

However, in the codes that we changed in the pr, we treat the CurrentDB as its lower representation. This leads to wrong string comparison result. So DELETE might not delete the data correctly.

What is changed and how it works?

Proposal: xxx

What's Changed:

Wrap CIStr upon the CurrentDB.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test

Release note

  • DELETE may not delete data correctly when the database name is not in pure lower representation.

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@winoros you're already a collaborator in bot's repo.

executor/delete_test.go Outdated Show resolved Hide resolved
executor/delete_test.go Outdated Show resolved Hide resolved
"github.com/pingcap/tidb/util/testkit"
)

func (s *testSuite8) TestDeleteLockKey(c *C) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@winoros this test is unexpected?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

Copy link
Contributor

@lzmhhh123 lzmhhh123 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 Nov 23, 2020
Copy link
Contributor

@eurekaka eurekaka 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/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 23, 2020
@eurekaka
Copy link
Contributor

/merge

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

/run-all-tests

@ti-srebot ti-srebot merged commit 67446a8 into pingcap:release-3.0 Nov 23, 2020
@winoros winoros deleted the release-3.0-1cdc15baaa08 branch November 23, 2020 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution 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. type/3.0-cherry-pick
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants