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

*: add scope check when get system variables #6958

Merged
merged 6 commits into from Jul 3, 2018

Conversation

@spongedu
Copy link
Contributor

commented Jul 2, 2018

What have you changed? (mandatory)

Add scope check when get system variables.
session variables cant' be accessed via SELECT @@global.xxxx. The same, global variables can'e be accessed via SELECT @@session.xxx or SELECT @@local.xxx.

What are the type of the changes (mandatory)?

enhancement

How has this PR been tested (mandatory)?

Unittest

@shenli shenli added the contribution label Jul 2, 2018
@@ -944,6 +944,8 @@ type VariableExpr struct {
IsGlobal bool
// IsSystem indicates whether this variable is a system variable in current session.
IsSystem bool
// ExplicitScope indicates whether this variable scope is explicit set.

This comment has been minimized.

Copy link
@shenli

shenli Jul 2, 2018

Member

is set explicitly

This comment has been minimized.

Copy link
@spongedu

spongedu Jul 2, 2018

Author Contributor

ok

@@ -5008,6 +5008,7 @@ SystemVariable:
{
v := strings.ToLower($1)
var isGlobal bool
explicitScope := true

This comment has been minimized.

Copy link
@shenli

shenli Jul 2, 2018

Member

alignment

This comment has been minimized.

Copy link
@spongedu

spongedu Jul 2, 2018

Author Contributor

ok

@@ -781,6 +781,13 @@ func (er *expressionRewriter) rewriteVariable(v *ast.VariableExpr) {
}
var val string
var err error
if v.ExplicitScope {

This comment has been minimized.

Copy link
@shenli

shenli Jul 2, 2018

Member

Why should we care if it is set explicitly or not?

This comment has been minimized.

Copy link
@spongedu

spongedu Jul 2, 2018

Author Contributor

if the scope constraint is not set explicitly, the behavior is to choose the right scope automatically. So the scope check should't happen here, logically. For example:
for a global variable max_connections, select @@max_connections works, and for a session variable warning_count, select @@warnging_count works as well.

This comment has been minimized.

Copy link
@shenli

shenli Jul 3, 2018

Member

You are right. I got something here:

For a reference to a system variable in an expression as @@var_name (rather than with @@global. or @@session.), MySQL returns the session value if it exists and the global value otherwise. This differs from SET @@var_name = expr, which always refers to the session value.
@shenli

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

/run-all-tests

@shenli

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

LGTM

@shenli

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

/run-all-tests

@shenli shenli added the status/LGT1 label Jul 3, 2018
@shenli

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

return UnknownSystemVar.GenByArgs(name)
}
switch sysVar.Scope {
case ScopeGlobal, ScopeNone:

This comment has been minimized.

Copy link
@zz-jason

zz-jason Jul 3, 2018

Member

why handle ScopeGlobal and ScopeNone together?

This comment has been minimized.

Copy link
@spongedu

spongedu Jul 3, 2018

Author Contributor

ScopeNone variables are global variables that can't be modified dynamically

@@ -69,7 +69,7 @@ const (
var (
UnknownStatusVar = terror.ClassVariable.New(CodeUnknownStatusVar, "unknown status variable")
UnknownSystemVar = terror.ClassVariable.New(CodeUnknownSystemVar, "unknown system variable '%s'")
ErrIncorrectScope = terror.ClassVariable.New(CodeIncorrectScope, "Incorrect variable scope")
ErrIncorrectScope = terror.ClassVariable.New(CodeIncorrectScope, "Variable '%s' is a %s variable")

This comment has been minimized.

Copy link
@jackysp

jackysp Jul 3, 2018

Member

Why not use

ErrIncorrectGlobalLocalVar: "Variable '%-.192s' is a %s variable",
?

This comment has been minimized.

Copy link
@spongedu

spongedu Jul 3, 2018

Author Contributor

Indeed. These error codes can be refined. Will fix

spongedu added 2 commits Jul 3, 2018
@spongedu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2018

@jackysp PTAL

Copy link
Member

left a comment

LGTM

@jackysp jackysp added status/LGT2 and removed status/LGT1 labels Jul 3, 2018
@@ -137,6 +137,25 @@ func SetSessionSystemVar(vars *SessionVars, name string, value types.Datum) erro
return vars.SetSystemVar(name, sVal)
}

// ValidateGetSystemVar check if system variable exists and validate it's scope when get system variable.

This comment has been minimized.

Copy link
@zimulala

zimulala Jul 3, 2018

Member

s/check/checks
"validates its scope when getting system variable" ?

This comment has been minimized.

Copy link
@spongedu

spongedu Jul 3, 2018

Author Contributor

ok. will fix

Copy link
Member

left a comment

LGTM

@zz-jason zz-jason merged commit 490af37 into pingcap:master Jul 3, 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:0703 branch Jul 3, 2018
zhexuany added a commit to zhexuany/tidb that referenced this pull request Jul 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.