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

util, types: don't let SPM be affected by charset #23161

Merged
merged 26 commits into from
Mar 12, 2021

Conversation

rebelice
Copy link
Contributor

@rebelice rebelice commented Mar 8, 2021

What problem does this PR solve?

Issue Number: close #xxx

Different default charsets in different drivers(or versions) make SQL bind cannot work.
So we don't let SPM be affected by charset.

We add a flag RestoreStringWithoutCharset to control this.

Need to merge parser pr first

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test

Side effects

Release note

  • don't let SPM be affected by charset

@rebelice rebelice requested a review from a team as a code owner March 8, 2021 06:25
@rebelice rebelice requested review from XuHuaiyu and removed request for a team March 8, 2021 06:25
@ti-chi-bot ti-chi-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 8, 2021
@rebelice
Copy link
Contributor Author

rebelice commented Mar 8, 2021

/run-check_dev_2 parser=pr/1184

@rebelice
Copy link
Contributor Author

rebelice commented Mar 8, 2021

/rebuild parser=pr/1184

@rebelice
Copy link
Contributor Author

rebelice commented Mar 8, 2021

/cc @qw4990, @eurekaka

@rebelice
Copy link
Contributor Author

rebelice commented Mar 8, 2021

/sig planner

@ti-chi-bot ti-chi-bot added the sig/planner SIG: Planner label Mar 8, 2021
@rebelice
Copy link
Contributor Author

rebelice commented Mar 8, 2021

/cc @qw4990

@ti-chi-bot ti-chi-bot requested a review from qw4990 March 8, 2021 06:33
@rebelice
Copy link
Contributor Author

rebelice commented Mar 8, 2021

/cc @eurekaka

@ti-chi-bot ti-chi-bot requested a review from eurekaka March 8, 2021 06:33
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.

Please add test for this PR.

@eurekaka eurekaka added priority/release-blocker This issue blocks a release. Please solve it ASAP. needs-cherry-pick-4.0 labels Mar 12, 2021
@rebelice rebelice requested a review from a team as a code owner March 12, 2021 07:02
@ti-chi-bot ti-chi-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 12, 2021
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.

We need an upgrade function as well.

@ti-chi-bot
Copy link
Member

Merge canceled because a new commit is pushed.

@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed status/can-merge Indicates a PR has been approved by a committer. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 12, 2021
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 12, 2021
@rebelice
Copy link
Contributor Author

/run-all-tests

@rebelice
Copy link
Contributor Author

/run-all-tests

1 similar comment
@rebelice
Copy link
Contributor Author

/run-all-tests

@rebelice
Copy link
Contributor Author

/run-all-tests

@@ -545,9 +553,10 @@ func (s *testBootstrapSuite) TestUpdateBindInfo(c *C) {
bindCase.bindText,
bindCase.db,
)
fmt.Println("[sql]", sql)
Copy link
Contributor

Choose a reason for hiding this comment

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

useless ?

@@ -543,12 +544,13 @@ var (
// We will redo upgradeToVer58 in upgradeToVer64, it is skipped here.
upgradeToVer59,
upgradeToVer60,
upgradeToVer61,
// We will redo upgradeToVer61 in upgradeToVer6, it is skipped here.
Copy link
Contributor

Choose a reason for hiding this comment

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

upgradeToVer6 ?

@rebelice
Copy link
Contributor Author

/run-all-tests

@XuHuaiyu
Copy link
Contributor

/lgtm

@XuHuaiyu XuHuaiyu merged commit 2bea06e into pingcap:master Mar 12, 2021
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Mar 12, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #23295

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/release-blocker This issue blocks a release. Please solve it ASAP. sig/planner SIG: Planner sig/sql-infra SIG: SQL Infra size/L Denotes a PR that changes 100-499 lines, ignoring generated files. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants