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 #7314

Closed
wants to merge 10 commits into from
Closed

sessionctx: supports set character_set_results = null #7314

wants to merge 10 commits into from

Conversation

imtbkcat
Copy link

@imtbkcat imtbkcat commented Aug 8, 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)

@CLAassistant
Copy link

CLAassistant commented Aug 8, 2018

CLA assistant check
All committers have signed the CLA.

@imtbkcat imtbkcat requested a review from coocood August 8, 2018 08:05
@imtbkcat
Copy link
Author

imtbkcat commented Aug 8, 2018

PTAL @coocood @jackysp @tiancaiamao

@imtbkcat
Copy link
Author

imtbkcat commented Aug 8, 2018

/run-all-tests

@@ -391,7 +391,7 @@ var defaultSysVars = []*SysVar{
{ScopeGlobal | ScopeSession, "binlog_direct_non_transactional_updates", "OFF"},
{ScopeGlobal, "innodb_change_buffering", "all"},
{ScopeGlobal | ScopeSession, "sql_big_selects", "ON"},
{ScopeGlobal | ScopeSession, CharacterSetResults, "latin1"},
Copy link
Member

Choose a reason for hiding this comment

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

We are planning to change it to "utf8". Will this solve your problem?

Copy link
Member

Choose a reason for hiding this comment

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

Refer to #7198

Copy link
Author

Choose a reason for hiding this comment

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

Changing it to "utf8" may not solve this problem. "show variables where variable_name='character_set_results'" result will become "utf8" after run "set character_set_results = null".

Copy link
Author

Choose a reason for hiding this comment

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

#7198 is okay, I will try to use @coocood 's suggestion.

@coocood
Copy link
Member

coocood commented Aug 8, 2018

How about setting the variable to empty string when it is null?

@imtbkcat imtbkcat changed the title mysql:let set character_set_results = null seem to work sessionctx:let set character_set_results = null seem to work Aug 8, 2018
@jackysp
Copy link
Member

jackysp commented Aug 8, 2018

Please add a test case for this pr.

@imtbkcat
Copy link
Author

imtbkcat commented Aug 9, 2018

/run-all-tests

Copy link
Member

@coocood coocood left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp
Copy link
Member

jackysp commented Aug 9, 2018

/run-all-tests

sVal = ""
} else {
sVal, err = value.ToString()
}
Copy link
Member

Choose a reason for hiding this comment

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

How about

if !value.IsNull() {
    sVal, err = value.ToString()
}

Copy link
Member

Choose a reason for hiding this comment

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

@imtbkcat please address this comment.

@jackysp
Copy link
Member

jackysp commented Aug 9, 2018

/ok-to-test

@jackysp
Copy link
Member

jackysp commented Aug 9, 2018

/rebuild

1 similar comment
@iamxy
Copy link
Member

iamxy commented Aug 9, 2018

/rebuild

@coocood coocood changed the title sessionctx:let set character_set_results = null seem to work sessionctx: supports set character_set_results = null Aug 9, 2018
@iamxy
Copy link
Member

iamxy commented Aug 10, 2018

/ok-to-test

@imtbkcat
Copy link
Author

/rebuild

@imtbkcat imtbkcat closed this Aug 10, 2018
@imtbkcat imtbkcat reopened this Aug 10, 2018
var err error
sVal, err = value.ToString()
if !value.IsNull() {
//return vars.deleteSystemVar(name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this comment?

@iamxy
Copy link
Member

iamxy commented Aug 10, 2018

/rebuild

@imtbkcat imtbkcat closed this Aug 10, 2018
@imtbkcat imtbkcat deleted the fix-#7248 branch August 10, 2018 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

set character_set_results = null seems not work
8 participants