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

sessionctx: supports `set character_set_results = null` #7353

Merged
merged 3 commits into from Aug 11, 2018

Conversation

@imtbkcat
Copy link
Contributor

commented Aug 10, 2018

What have you changed? (mandatory)

When tidb executes "set character_set_results = null", it will delete "character_set_results" variable because we set it to null. (see executor/set.go:79)

if value.IsNull() {
    	delete(sessionVars.Users, name)
}

This will delete "character_set_results". When we use show variable to show it, executor can not find it.
Then ShowExecutor will go to variable.SysVars to find its default value. (see executor/show.go:434)

varValue, ok := systemVars[varName]
if !ok {
    varValue = variable.SysVars[varName].Value
}

Originally, variable.SysVars["character_set_results"] is "latin1", so the return value of "show variables where variable_name='character_set_results'" is "latin1". But "character_set_results" in TiDB is always "utf8", set variable can not effect it.

This PR just fix compatibility with mysql when execute "set character_set_results = null".

What is the type of the changes? (mandatory)

  • Improvement (non-breaking change which is an improvement to an existing feature)

How has this PR been tested? (mandatory)

unit test

After change:

mysql> set character_set_results = null;
Query OK, 0 rows affected (0.00 sec)

mysql> show variables where variable_name='character_set_results';
+-----------------------+-------+
| Variable_name         | Value |
+-----------------------+-------+
| character_set_results |       |
+-----------------------+-------+
1 row in set (0.01 sec)

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

No

Does this PR affect tidb-ansible update? (mandatory)

No

Does this PR need to be added to the release notes? (mandatory)

No

Refer to a related PR or issue link (optional)

Fix #7284

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

@imtbkcat imtbkcat requested review from jackysp and coocood Aug 10, 2018
@imtbkcat

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2018

/run-all-tests

@coocood

This comment has been minimized.

Copy link
Member

commented Aug 10, 2018

LGTM

@@ -133,7 +133,8 @@ func (s *testSuite) TestSetVar(c *C) {
c.Assert(charset, Equals, "utf8")
c.Assert(collation, Equals, "utf8_bin")

tk.MustExec("set @@character_set_results = NULL")
tk.MustExec("set character_set_results = NULL")
tk.MustQuery("select @@character_set_results").Check(testkit.Rows(""))

This comment has been minimized.

Copy link
@lysu

lysu Aug 10, 2018

Member

In MySQL, it's very strange that will return NULL instead of "" in select @@ but return "" in show variable orz

mysql> select @@character_set_results ;
+-------------------------+
| @@character_set_results |
+-------------------------+
| utf8                    |
+-------------------------+
1 row in set (0.00 sec)

mysql> set character_set_results = NULL;
Query OK, 0 rows affected (0.00 sec)

mysql> select @@character_set_results ;
+-------------------------+
| @@character_set_results |
+-------------------------+
| NULL                    |
+-------------------------+
1 row in set (0.00 sec)

mysql> show variables where variable_name='character_set_results';
+-----------------------+-------+
| Variable_name         | Value |
+-----------------------+-------+
| character_set_results |       |
+-----------------------+-------+
1 row in set (0.01 sec)

This comment has been minimized.

Copy link
@jackysp

jackysp Aug 11, 2018

Member

I think system variables in MySQL have their own types.

Copy link
Member

left a comment

LGTM

zz-jason and others added 2 commits Aug 10, 2018
@jackysp jackysp merged commit 8762592 into pingcap:master Aug 11, 2018
3 of 4 checks passed
3 of 4 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
ci/circleci Your tests passed on CircleCI!
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.