From af061b1145367cdae7a45a74d0aedcd010b42bf4 Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Wed, 10 Nov 2021 21:57:17 -0700 Subject: [PATCH 1/2] executor: improve SET sysvar=DEFAULT handling --- executor/set.go | 24 ++++++++---------------- executor/set_test.go | 30 ++++++++++++++++++++++++++++-- expression/helper_test.go | 2 +- sessionctx/variable/sysvar.go | 11 ----------- 4 files changed, 37 insertions(+), 30 deletions(-) diff --git a/executor/set.go b/executor/set.go index 5ad8790a746f3..e38a38584aa0f 100644 --- a/executor/set.go +++ b/executor/set.go @@ -115,11 +115,11 @@ func (e *SetExecutor) setSysVariable(ctx context.Context, name string, v *expres } return variable.ErrUnknownSystemVar.GenWithStackByArgs(name) } + valStr, err := e.extractVarValue(v, sysVar) + if err != nil { + return err + } if v.IsGlobal { - valStr, err := e.getVarValue(v, sysVar) - if err != nil { - return err - } err = sessionVars.GlobalVarsAccessor.SetGlobalSysVar(name, valStr) if err != nil { return err @@ -140,10 +140,6 @@ func (e *SetExecutor) setSysVariable(ctx context.Context, name string, v *expres return err } // Set session variable - valStr, err := e.getVarValue(v, nil) - if err != nil { - return err - } getSnapshotTSByName := func() uint64 { if name == variable.TiDBSnapshot { return sessionVars.SnapshotTS @@ -274,15 +270,11 @@ func (e *SetExecutor) setCharset(cs, co string, isSetName bool) error { return errors.Trace(variable.SetSessionSystemVar(sessionVars, variable.CollationConnection, coDb)) } -func (e *SetExecutor) getVarValue(v *expression.VarAssignment, sysVar *variable.SysVar) (value string, err error) { +func (e *SetExecutor) extractVarValue(v *expression.VarAssignment, sysVar *variable.SysVar) (value string, err error) { if v.IsDefault { - // To set a SESSION variable to the GLOBAL value or a GLOBAL value - // to the compiled-in MySQL default value, use the DEFAULT keyword. - // See http://dev.mysql.com/doc/refman/5.7/en/set-statement.html - if sysVar != nil { - return sysVar.Value, nil - } - return variable.GetGlobalSystemVar(e.ctx.GetSessionVars(), v.Name) + // Set a variable to the compiled-in MySQL default value + // http://dev.mysql.com/doc/refman/5.7/en/set-statement.html + return sysVar.Value, nil } nativeVal, err := v.Expr.Eval(chunk.Row{}) if err != nil || nativeVal.IsNull() { diff --git a/executor/set_test.go b/executor/set_test.go index d53cd64b20966..8e539421131bf 100644 --- a/executor/set_test.go +++ b/executor/set_test.go @@ -116,8 +116,8 @@ func (s *testSerialSuite1) TestSetVar(c *C) { // For session tk.MustQuery(`select @@session.low_priority_updates;`).Check(testkit.Rows("0")) tk.MustExec(`set @@global.low_priority_updates="ON";`) - tk.MustExec(`set @@session.low_priority_updates=DEFAULT;`) // It will be set to global var value. - tk.MustQuery(`select @@session.low_priority_updates;`).Check(testkit.Rows("1")) + tk.MustExec(`set @@session.low_priority_updates=DEFAULT;`) // It will be set to compiled-in default value. + tk.MustQuery(`select @@session.low_priority_updates;`).Check(testkit.Rows("0")) // For mysql jdbc driver issue. tk.MustQuery(`select @@session.tx_read_only;`).Check(testkit.Rows("0")) @@ -1374,6 +1374,32 @@ func (s *testSuite5) TestEnableNoopFunctionsVar(c *C) { } +// https://github.com/pingcap/tidb/issues/29670 +func (s *testSuite5) TestDefaultBehavior(c *C) { + tk := testkit.NewTestKit(c, s.store) + + tk.MustQuery("SELECT @@default_storage_engine").Check(testkit.Rows("InnoDB")) + tk.MustExec("SET GLOBAL default_storage_engine = 'somethingweird'") + tk.MustExec("SET default_storage_engine = 'MyISAM'") + tk.MustQuery("SELECT @@default_storage_engine").Check(testkit.Rows("MyISAM")) + tk.MustExec("SET default_storage_engine = DEFAULT") // reads from compiled default + tk.MustQuery("SELECT @@default_storage_engine").Check(testkit.Rows("InnoDB")) + tk.MustExec("SET @@SESSION.default_storage_engine = @@GLOBAL.default_storage_engine") // example from MySQL manual + tk.MustQuery("SELECT @@default_storage_engine").Check(testkit.Rows("somethingweird")) + tk.MustExec("SET GLOBAL default_storage_engine = 'somethingweird2'") + tk.MustExec("SET default_storage_engine = @@GLOBAL.default_storage_engine") // variation of example + tk.MustQuery("SELECT @@default_storage_engine").Check(testkit.Rows("somethingweird2")) + tk.MustExec("SET default_storage_engine = DEFAULT") // restore default again for session global + tk.MustExec("SET GLOBAL default_storage_engine = DEFAULT") // restore default for global + tk.MustQuery("SELECT @@SESSION.default_storage_engine, @@GLOBAL.default_storage_engine").Check(testkit.Rows("InnoDB InnoDB")) + + // Try sql_mode option which has validation + err := tk.ExecToErr("SET GLOBAL sql_mode = 'DEFAULT'") // illegal now + c.Assert(err, NotNil) + c.Assert(err.Error(), Equals, `ERROR 1231 (42000): Variable 'sql_mode' can't be set to the value of 'DEFAULT'`) + tk.MustExec("SET GLOBAL sql_mode = DEFAULT") +} + func (s *testSuite5) TestRemovedSysVars(c *C) { tk := testkit.NewTestKit(c, s.store) diff --git a/expression/helper_test.go b/expression/helper_test.go index c4398b993c5d0..d83272294200b 100644 --- a/expression/helper_test.go +++ b/expression/helper_test.go @@ -44,7 +44,7 @@ func TestGetTimeValue(t *testing.T) { require.Equal(t, "2012-12-12 00:00:00", timeValue.String()) sessionVars := ctx.GetSessionVars() - err = variable.SetSessionSystemVar(sessionVars, "timestamp", "default") + err = variable.SetSessionSystemVar(sessionVars, "timestamp", "0") require.NoError(t, err) v, err = GetTimeValue(ctx, "2012-12-12 00:00:00", mysql.TypeTimestamp, types.MinFsp) require.NoError(t, err) diff --git a/sessionctx/variable/sysvar.go b/sessionctx/variable/sysvar.go index b320e3a60fd66..e492ee4eceb20 100644 --- a/sessionctx/variable/sysvar.go +++ b/sessionctx/variable/sysvar.go @@ -278,11 +278,6 @@ func (sv *SysVar) Validate(vars *SessionVars, value string, scope ScopeFlag) (st // validateFromType provides automatic validation based on the SysVar's type func (sv *SysVar) validateFromType(vars *SessionVars, value string, scope ScopeFlag) (string, error) { - // The string "DEFAULT" is a special keyword in MySQL, which restores - // the compiled sysvar value. In which case we can skip further validation. - if strings.EqualFold(value, "DEFAULT") { - return sv.Value, nil - } // Some sysvars in TiDB have a special behavior where the empty string means // "use the config file value". This needs to be cleaned up once the behavior // for instance variables is determined. @@ -667,12 +662,6 @@ var defaultSysVars = []*SysVar{ } timestamp := s.StmtCtx.GetOrStoreStmtCache(stmtctx.StmtNowTsCacheKey, time.Now()).(time.Time) return types.ToString(float64(timestamp.UnixNano()) / float64(time.Second)) - }, GetGlobal: func(s *SessionVars) (string, error) { - // The Timestamp sysvar will have GetGlobal func even though it does not have global scope. - // It's GetGlobal func will only be called when "set timestamp = default". - // Setting timestamp to DEFAULT causes its value to be the current date and time as of the time it is accessed. - // See https://dev.mysql.com/doc/refman/8.0/en/server-system-variables.html#sysvar_timestamp - return DefTimestamp, nil }}, {Scope: ScopeGlobal | ScopeSession, Name: CollationDatabase, Value: mysql.DefaultCollationName, skipInit: true, Validation: func(vars *SessionVars, normalizedValue string, originalValue string, scope ScopeFlag) (string, error) { return checkCollation(vars, normalizedValue, originalValue, scope) From 3aaeabcb13e6b9e7a97100e58b47d1c8681f7bfa Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Thu, 16 Dec 2021 15:40:10 -0600 Subject: [PATCH 2/2] implement correct fix --- executor/set.go | 24 ++++++++++++++++-------- executor/set_test.go | 12 ++++++------ 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/executor/set.go b/executor/set.go index 04a764ea31ff3..65e21d470cc78 100644 --- a/executor/set.go +++ b/executor/set.go @@ -115,11 +115,11 @@ func (e *SetExecutor) setSysVariable(ctx context.Context, name string, v *expres } return variable.ErrUnknownSystemVar.GenWithStackByArgs(name) } - valStr, err := e.extractVarValue(v, sysVar) - if err != nil { - return err - } if v.IsGlobal { + valStr, err := e.getVarValue(v, sysVar) + if err != nil { + return err + } err = sessionVars.GlobalVarsAccessor.SetGlobalSysVar(name, valStr) if err != nil { return err @@ -140,6 +140,10 @@ func (e *SetExecutor) setSysVariable(ctx context.Context, name string, v *expres return err } // Set session variable + valStr, err := e.getVarValue(v, nil) + if err != nil { + return err + } getSnapshotTSByName := func() uint64 { if name == variable.TiDBSnapshot { return sessionVars.SnapshotTS @@ -284,11 +288,15 @@ func (e *SetExecutor) setCharset(cs, co string, isSetName bool) error { return errors.Trace(variable.SetSessionSystemVar(sessionVars, variable.CollationConnection, coDb)) } -func (e *SetExecutor) extractVarValue(v *expression.VarAssignment, sysVar *variable.SysVar) (value string, err error) { +func (e *SetExecutor) getVarValue(v *expression.VarAssignment, sysVar *variable.SysVar) (value string, err error) { if v.IsDefault { - // Set a variable to the compiled-in MySQL default value - // http://dev.mysql.com/doc/refman/5.7/en/set-statement.html - return sysVar.Value, nil + // To set a SESSION variable to the GLOBAL value or a GLOBAL value + // to the compiled-in MySQL default value, use the DEFAULT keyword. + // See http://dev.mysql.com/doc/refman/5.7/en/set-statement.html + if sysVar != nil { + return sysVar.Value, nil + } + return variable.GetGlobalSystemVar(e.ctx.GetSessionVars(), v.Name) } nativeVal, err := v.Expr.Eval(chunk.Row{}) if err != nil || nativeVal.IsNull() { diff --git a/executor/set_test.go b/executor/set_test.go index c663825eb51cc..346ed44c99104 100644 --- a/executor/set_test.go +++ b/executor/set_test.go @@ -111,13 +111,13 @@ func (s *testSerialSuite1) TestSetVar(c *C) { 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.MustExec(`set @@global.low_priority_updates=DEFAULT;`) // It will be set to default var value. tk.MustQuery(`select @@global.low_priority_updates;`).Check(testkit.Rows("0")) // For session tk.MustQuery(`select @@session.low_priority_updates;`).Check(testkit.Rows("0")) tk.MustExec(`set @@global.low_priority_updates="ON";`) - tk.MustExec(`set @@session.low_priority_updates=DEFAULT;`) // It will be set to compiled-in default value. - tk.MustQuery(`select @@session.low_priority_updates;`).Check(testkit.Rows("0")) + tk.MustExec(`set @@session.low_priority_updates=DEFAULT;`) // It will be set to global var value. + tk.MustQuery(`select @@session.low_priority_updates;`).Check(testkit.Rows("1")) // For mysql jdbc driver issue. tk.MustQuery(`select @@session.tx_read_only;`).Check(testkit.Rows("0")) @@ -1394,8 +1394,8 @@ func (s *testSuite5) TestDefaultBehavior(c *C) { tk.MustExec("SET GLOBAL default_storage_engine = 'somethingweird'") tk.MustExec("SET default_storage_engine = 'MyISAM'") tk.MustQuery("SELECT @@default_storage_engine").Check(testkit.Rows("MyISAM")) - tk.MustExec("SET default_storage_engine = DEFAULT") // reads from compiled default - tk.MustQuery("SELECT @@default_storage_engine").Check(testkit.Rows("InnoDB")) + tk.MustExec("SET default_storage_engine = DEFAULT") // reads from global value + tk.MustQuery("SELECT @@default_storage_engine").Check(testkit.Rows("somethingweird")) tk.MustExec("SET @@SESSION.default_storage_engine = @@GLOBAL.default_storage_engine") // example from MySQL manual tk.MustQuery("SELECT @@default_storage_engine").Check(testkit.Rows("somethingweird")) tk.MustExec("SET GLOBAL default_storage_engine = 'somethingweird2'") @@ -1403,7 +1403,7 @@ func (s *testSuite5) TestDefaultBehavior(c *C) { tk.MustQuery("SELECT @@default_storage_engine").Check(testkit.Rows("somethingweird2")) tk.MustExec("SET default_storage_engine = DEFAULT") // restore default again for session global tk.MustExec("SET GLOBAL default_storage_engine = DEFAULT") // restore default for global - tk.MustQuery("SELECT @@SESSION.default_storage_engine, @@GLOBAL.default_storage_engine").Check(testkit.Rows("InnoDB InnoDB")) + tk.MustQuery("SELECT @@SESSION.default_storage_engine, @@GLOBAL.default_storage_engine").Check(testkit.Rows("somethingweird2 InnoDB")) // Try sql_mode option which has validation err := tk.ExecToErr("SET GLOBAL sql_mode = 'DEFAULT'") // illegal now