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.

Sign up for GitHub

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

Jump to bottom

*: add scope check when get system variables #6958

Merged
merged 6 commits into from
Jul 3, 2018
Merged

*: add scope check when get system variables #6958

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8fb0a84
*: add scope check when get system variables
spongedu Jul 2, 2018
3781767
fix format
spongedu Jul 2, 2018
ded54b4
Merge branch 'master' into 0703
spongedu Jul 3, 2018
1e7805f
Refine errors
spongedu Jul 3, 2018
13f1cfd
Fix comment
spongedu Jul 3, 2018
a79d5ea
Merge remote-tracking branch 'upstream/master' into 0703
spongedu Jul 3, 2018
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
14 changes: 7 additions & 7 deletions sessionctx/variable/sysvar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,18 +59,18 @@ func GetSysVar(name string) *SysVar {
// Variable error codes.
const (
CodeUnknownStatusVar terror.ErrCode = 1
CodeUnknownSystemVar terror.ErrCode = 1193
CodeIncorrectScope terror.ErrCode = 1238
CodeUnknownTimeZone terror.ErrCode = 1298
CodeReadOnly terror.ErrCode = 1621
CodeUnknownSystemVar terror.ErrCode = mysql.ErrUnknownSystemVariable
CodeIncorrectScope terror.ErrCode = mysql.ErrIncorrectGlobalLocalVar
CodeUnknownTimeZone terror.ErrCode = mysql.ErrUnknownTimeZone
CodeReadOnly terror.ErrCode = mysql.ErrVariableIsReadonly
)

// Variable errors
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")
ErrUnknownTimeZone = terror.ClassVariable.New(CodeUnknownTimeZone, "unknown or incorrect time zone: %s")
UnknownSystemVar = terror.ClassVariable.New(CodeUnknownSystemVar, mysql.MySQLErrName[mysql.ErrUnknownSystemVariable])
ErrIncorrectScope = terror.ClassVariable.New(CodeIncorrectScope, mysql.MySQLErrName[mysql.ErrIncorrectGlobalLocalVar])
ErrUnknownTimeZone = terror.ClassVariable.New(CodeUnknownTimeZone, mysql.MySQLErrName[mysql.ErrUnknownTimeZone])
ErrReadOnly = terror.ClassVariable.New(CodeReadOnly, "variable is read only")
)

Expand Down
19 changes: 19 additions & 0 deletions sessionctx/variable/varsutil.go
View file
Open in desktop
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