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
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ast/expressions.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 set explicitly.
ExplicitScope bool
// Value is the variable value.
Value ExprNode
}
Expand Down
5 changes: 3 additions & 2 deletions parser/parser.y
Original file line number Diff line number Diff line change
Expand Up @@ -5008,6 +5008,7 @@ SystemVariable:
{
v := strings.ToLower($1)
var isGlobal bool
explicitScope := true
if strings.HasPrefix(v, "@@global.") {
isGlobal = true
v = strings.TrimPrefix(v, "@@global.")
Expand All @@ -5016,9 +5017,9 @@ SystemVariable:
} else if strings.HasPrefix(v, "@@local.") {
v = strings.TrimPrefix(v, "@@local.")
} else if strings.HasPrefix(v, "@@") {
v = strings.TrimPrefix(v, "@@")
v, explicitScope = strings.TrimPrefix(v, "@@"), false
}
$$ = &ast.VariableExpr{Name: v, IsGlobal: isGlobal, IsSystem: true}
$$ = &ast.VariableExpr{Name: v, IsGlobal: isGlobal, IsSystem: true, ExplicitScope: explicitScope}
}

UserVariable:
Expand Down
7 changes: 7 additions & 0 deletions plan/expression_rewriter.go
Original file line number Diff line number Diff line change
Expand Up @@ -781,6 +781,13 @@ func (er *expressionRewriter) rewriteVariable(v *ast.VariableExpr) {
}
var val string
var err error
if v.ExplicitScope {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

err = variable.ValidateGetSystemVar(name, v.IsGlobal)
if err != nil {
er.err = errors.Trace(err)
return
}
}
if v.IsGlobal {
val, err = variable.GetGlobalSystemVar(sessionVars, name)
} else {
Expand Down
27 changes: 27 additions & 0 deletions session/session_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,33 @@ func (s *testSessionSuite) TestGlobalVarAccessor(c *C) {
tk.MustExec("set @@global.autocommit=1")
}

func (s *testSessionSuite) TestGetSysVariables(c *C) {
tk := testkit.NewTestKitWithInit(c, s.store)

// Test ScopeSession
tk.MustExec("select @@warning_count")
tk.MustExec("select @@session.warning_count")
tk.MustExec("select @@local.warning_count")
_, err := tk.Exec("select @@global.warning_count")
c.Assert(terror.ErrorEqual(err, variable.ErrIncorrectScope), IsTrue)

// Test ScopeGlobal
tk.MustExec("select @@max_connections")
tk.MustExec("select @@global.max_connections")
_, err = tk.Exec("select @@session.max_connections")
c.Assert(terror.ErrorEqual(err, variable.ErrIncorrectScope), IsTrue)
_, err = tk.Exec("select @@local.max_connections")
c.Assert(terror.ErrorEqual(err, variable.ErrIncorrectScope), IsTrue)

// Test ScopeNone
tk.MustExec("select @@performance_schema_max_mutex_classes")
tk.MustExec("select @@global.performance_schema_max_mutex_classes")
_, err = tk.Exec("select @@session.performance_schema_max_mutex_classes")
c.Assert(terror.ErrorEqual(err, variable.ErrIncorrectScope), IsTrue)
_, err = tk.Exec("select @@local.performance_schema_max_mutex_classes")
c.Assert(terror.ErrorEqual(err, variable.ErrIncorrectScope), IsTrue)
}

func (s *testSessionSuite) TestRetryResetStmtCtx(c *C) {
tk := testkit.NewTestKitWithInit(c, s.store)
tk.MustExec("create table retrytxn (a int unique, b int)")
Expand Down
2 changes: 1 addition & 1 deletion sessionctx/variable/sysvar.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Member

Choose a reason for hiding this comment

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

Why not use

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. These error codes can be refined. Will fix

ErrUnknownTimeZone = terror.ClassVariable.New(CodeUnknownTimeZone, "unknown or incorrect time zone: %s")
ErrReadOnly = terror.ClassVariable.New(CodeReadOnly, "variable is read only")
)
Expand Down
19 changes: 19 additions & 0 deletions sessionctx/variable/varsutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. will fix

func ValidateGetSystemVar(name string, isGlobal bool) error {
sysVar, exists := SysVars[name]
if !exists {
return UnknownSystemVar.GenByArgs(name)
}
switch sysVar.Scope {
case ScopeGlobal, ScopeNone:
Copy link
Member

Choose a reason for hiding this comment

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

why handle ScopeGlobal and ScopeNone together?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if !isGlobal {
return ErrIncorrectScope.GenByArgs(name, "GLOBAL")
}
case ScopeSession:
if isGlobal {
return ErrIncorrectScope.GenByArgs(name, "SESSION")
}
}
return nil
}

// TiDBOptOn could be used for all tidb session variable options, we use "ON"/1 to turn on those options.
func TiDBOptOn(opt string) bool {
return strings.EqualFold(opt, "ON") || opt == "1"
Expand Down