*: add scope check when get system variables #6958
Conversation
ast/expressions.go
Outdated
| @@ -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. | |||
parser/parser.y
Outdated
| @@ -5008,6 +5008,7 @@ SystemVariable: | |||
| { | |||
| v := strings.ToLower($1) | |||
| var isGlobal bool | |||
| explicitScope := true | |||
| @@ -781,6 +781,13 @@ func (er *expressionRewriter) rewriteVariable(v *ast.VariableExpr) { | |||
| } | |||
| var val string | |||
| var err error | |||
| if v.ExplicitScope { | |||
There was a problem hiding this comment.
Why should we care if it is set explicitly or not?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
/run-all-tests |
|
LGTM |
|
/run-all-tests |
|
@tiancaiamao @lysu PTAL |
| return UnknownSystemVar.GenByArgs(name) | ||
| } | ||
| switch sysVar.Scope { | ||
| case ScopeGlobal, ScopeNone: |
There was a problem hiding this comment.
why handle ScopeGlobal and ScopeNone together?
There was a problem hiding this comment.
ScopeNone variables are global variables that can't be modified dynamically
sessionctx/variable/sysvar.go
Outdated
| @@ -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") | |||
There was a problem hiding this comment.
Why not use
Line 256 in 3db7594
There was a problem hiding this comment.
Indeed. These error codes can be refined. Will fix
|
@jackysp PTAL |
jackysp
added
status/LGT2
sessionctx/variable/varsutil.go
Outdated
| @@ -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. | |||
There was a problem hiding this comment.
s/check/checks
"validates its scope when getting system variable" ?
What have you changed? (mandatory)
Add scope check when get system variables.
sessionvariables cant' be accessed viaSELECT @@global.xxxx. The same,globalvariables can'e be accessed viaSELECT @@session.xxxorSELECT @@local.xxx.What are the type of the changes (mandatory)?
enhancementHow has this PR been tested (mandatory)?
Unittest