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, executor: Add correctness check when set system variables #7117

Merged
merged 19 commits into from Jul 24, 2018

Conversation

@spongedu
Copy link
Contributor

commented Jul 20, 2018

What have you changed? (mandatory)

Add correctness check when set system variables

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)

Unittest

@shenli

This comment has been minimized.

Copy link
Member

commented Jul 22, 2018

@spongedu Thanks! Is it the same behaviour with MySQL?

@shenli

This comment has been minimized.

Copy link
Member

commented Jul 22, 2018

/run-all-tests

@shenli shenli added the contribution label Jul 22, 2018
@spongedu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2018

@shenli Yes,this pr add the same type & value check as MySQL do.
Not all system variables are checked because there are too much of them, but after this PR we can fix the others concurrently.
Currently, if we give wrong value to TiDB specific system variable, a default value will be set and no warning or error is raised. I suggest that we deal with TiDB specific system variable the same way as MySQL system variable, that would be less confusing if the user use wrong value.

@winkyao

This comment has been minimized.

Copy link
Member

commented Jul 23, 2018

Please fix ci.

Copy link
Contributor

left a comment

Great job~
Does all the mysql system vars has been tested in MySQL5.7?

@@ -156,6 +165,181 @@ func ValidateGetSystemVar(name string, isGlobal bool) error {
return nil
}

// ValidateSetSystemVar checks if system variable satisfies specific restriction.
func ValidateSetSystemVar(name string, value string) (string, error, error) {

This comment has been minimized.

Copy link
@XuHuaiyu

XuHuaiyu Jul 23, 2018

Contributor

We may not need to return warn?

This comment has been minimized.

Copy link
@spongedu

spongedu Jul 23, 2018

Author Contributor

yes, If we take the SessionVars as a parameter into function ValidateSetSystemVar, we can directly append warning in this function. I'll fix this

@XuHuaiyu XuHuaiyu added this to the 2.1 milestone Jul 23, 2018
if val := GetSysVar(name); val != nil {
return val.Value, nil, nil
}
return value, nil, nil

This comment has been minimized.

Copy link
@XuHuaiyu

XuHuaiyu Jul 23, 2018

Contributor

Any case cover this line?

This comment has been minimized.

Copy link
@spongedu

spongedu Jul 23, 2018

Author Contributor

In set_test.go, line 93 - line 97 covers this:

	tk.MustQuery(`select @@global.low_priority_updates;`).Check(testkit.Rows("0"))
	tk.MustExec(`set @@global.low_priority_updates="ON";`)
	tk.MustQuery(`select @@global.low_priority_updates;`).Check(testkit.Rows("1"))
	tk.MustExec(`set @@global.low_priority_updates=DEFAULT;`) // It will be set to compiled-in default value.
	tk.MustQuery(`select @@global.low_priority_updates;`).Check(testkit.Rows("0"))
	

This comment has been minimized.

Copy link
@XuHuaiyu

XuHuaiyu Jul 23, 2018

Contributor

Yeah, this case covers line 172, but seems there is no case that covers line 174?

This comment has been minimized.

Copy link
@spongedu

spongedu Jul 23, 2018

Author Contributor

It seems so. I'll add some related tests..

This comment has been minimized.

Copy link
@spongedu

spongedu Jul 23, 2018

Author Contributor

@XuHuaiyu the condition that we fail to GetSysVar should never happen. We'd better panic here ?

@spongedu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2018

return val.Value, nil
}
// should never happen
panic(fmt.Sprintf("Error happened when ValidateSetSystemVar. Invalid system variable: %s", name))

This comment has been minimized.

Copy link
@XuHuaiyu

XuHuaiyu Jul 24, 2018

Contributor

Will it be more friendly if we return UnknownSystemVar.GenByArgs(name) here

This comment has been minimized.

Copy link
@spongedu

spongedu Jul 24, 2018

Author Contributor

ok

Copy link
Contributor

left a comment

Addressing the comment will make a LGTM ^_^

@@ -156,6 +162,197 @@ func ValidateGetSystemVar(name string, isGlobal bool) error {
return nil
}

// ValidateSetSystemVar checks if system variable satisfies specific restriction.
func ValidateSetSystemVar(vars *SessionVars, name string, value string) (string, error) {
if strings.EqualFold(value, "DEFAULT") {

This comment has been minimized.

Copy link
@zhexuany

zhexuany Jul 24, 2018

Member

Maybe we need to consider System? Currently, set time_zone='System' does not work.

This comment has been minimized.

Copy link
@spongedu

spongedu Jul 24, 2018

Author Contributor

@zhexuany it's not a common check, I think we'd better by refine parseTimeZone in varsutil.go to :

func parseTimeZone(s string) (*time.Location, error) {
	if strings.EqualFold(s, "SYSTEM") {
	.......
}

This comment has been minimized.

Copy link
@spongedu

spongedu Jul 24, 2018

Author Contributor

It seems that we should validate the result format in ValidateSetSystemVar. Only modify parseTimeZone is not enough. I'll fix here.

This comment has been minimized.

Copy link
@spongedu

spongedu Jul 24, 2018

Author Contributor
tidb> set time_zone='System';                                                                                                                                               Query OK, 0 rows affected (0.00 sec)

tidb> select @@time_zone;
+-------------+
| @@time_zone |
+-------------+
| System      |
+-------------+
1 row in set (0.00 sec)

In MySQL:

mysql> set time_zone='System';                                                                                                                                              Query OK, 0 rows affected (0.00 sec)

mysql> select @@time_zone;                                                                                                                                                  
+-------------+
| @@time_zone |
+-------------+
| SYSTEM      |
+-------------+
1 row in set (0.01 sec)

This comment has been minimized.

Copy link
@spongedu

spongedu Jul 24, 2018

Author Contributor

@zhexuany Done here. PTAL

spongedu added 2 commits Jul 24, 2018
@shenli

This comment has been minimized.

Copy link
Member

commented Jul 24, 2018

Copy link
Contributor

left a comment

LGTM

Copy link
Member

left a comment

LGTM

@XuHuaiyu XuHuaiyu merged commit 437933d into pingcap:master Jul 24, 2018
4 checks passed
4 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
@spongedu spongedu deleted the spongedu:dc_0720 branch Jul 24, 2018
zz-jason added a commit that referenced this pull request Feb 22, 2019
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.