From 737e6290e63af6b9fcd7922c82150b44ff16b394 Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Tue, 7 Dec 2021 19:58:36 -0700 Subject: [PATCH 01/14] sessionctx/variable: change tidb_store_limit to global only --- sessionctx/variable/sysvar.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/sessionctx/variable/sysvar.go b/sessionctx/variable/sysvar.go index a1fcf7e025053..14fecee0a7b5c 100644 --- a/sessionctx/variable/sysvar.go +++ b/sessionctx/variable/sysvar.go @@ -1587,10 +1587,7 @@ var defaultSysVars = []*SysVar{ } return nil }}, - {Scope: ScopeGlobal | ScopeSession, Name: TiDBStoreLimit, Value: strconv.FormatInt(atomic.LoadInt64(&config.GetGlobalConfig().TiKVClient.StoreLimit), 10), Type: TypeInt, MinValue: 0, MaxValue: math.MaxInt64, SetSession: func(s *SessionVars, val string) error { - tikvstore.StoreLimit.Store(tidbOptInt64(val, DefTiDBStoreLimit)) - return nil - }, GetSession: func(s *SessionVars) (string, error) { + {Scope: ScopeGlobal, Name: TiDBStoreLimit, Value: strconv.FormatInt(atomic.LoadInt64(&config.GetGlobalConfig().TiKVClient.StoreLimit), 10), Type: TypeInt, MinValue: 0, MaxValue: math.MaxInt64, GetGlobal: func(s *SessionVars) (string, error) { return strconv.FormatInt(tikvstore.StoreLimit.Load(), 10), nil }, SetGlobal: func(s *SessionVars, val string) error { tikvstore.StoreLimit.Store(tidbOptInt64(val, DefTiDBStoreLimit)) From 81b0905b35ddec25e4f45a4ae921ea1628d26332 Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Tue, 7 Dec 2021 21:19:30 -0700 Subject: [PATCH 02/14] fix broken test --- executor/set_test.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/executor/set_test.go b/executor/set_test.go index 2595b4a3131f5..425edf1c62ceb 100644 --- a/executor/set_test.go +++ b/executor/set_test.go @@ -366,16 +366,11 @@ func (s *testSerialSuite1) TestSetVar(c *C) { tk.MustExec("set @@tidb_expensive_query_time_threshold=70") tk.MustQuery("select @@tidb_expensive_query_time_threshold;").Check(testkit.Rows("70")) - tk.MustQuery("select @@tidb_store_limit;").Check(testkit.Rows("0")) - tk.MustExec("set @@tidb_store_limit = 100") - tk.MustQuery("select @@tidb_store_limit;").Check(testkit.Rows("100")) - tk.MustQuery("select @@session.tidb_store_limit;").Check(testkit.Rows("100")) tk.MustQuery("select @@global.tidb_store_limit;").Check(testkit.Rows("0")) - tk.MustExec("set @@tidb_store_limit = 0") - + tk.MustExec("set @@global.tidb_store_limit = 100") + tk.MustQuery("select @@global.tidb_store_limit;").Check(testkit.Rows("100")) + tk.MustExec("set @@global.tidb_store_limit = 0") tk.MustExec("set global tidb_store_limit = 100") - tk.MustQuery("select @@tidb_store_limit;").Check(testkit.Rows("100")) - tk.MustQuery("select @@session.tidb_store_limit;").Check(testkit.Rows("100")) tk.MustQuery("select @@global.tidb_store_limit;").Check(testkit.Rows("100")) tk.MustQuery("select @@session.tidb_metric_query_step;").Check(testkit.Rows("60")) From fa6f9c75600bde748aa643f08400478a54599483 Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Wed, 8 Dec 2021 16:51:18 -0700 Subject: [PATCH 03/14] docs: add design doc for instance scoped variables --- docs/design/2021-12-08-instance-scope.md | 187 +++++++++++++++++++++++ 1 file changed, 187 insertions(+) create mode 100644 docs/design/2021-12-08-instance-scope.md diff --git a/docs/design/2021-12-08-instance-scope.md b/docs/design/2021-12-08-instance-scope.md new file mode 100644 index 0000000000000..eef40298b7987 --- /dev/null +++ b/docs/design/2021-12-08-instance-scope.md @@ -0,0 +1,187 @@ +# TiDB Design Documents + +- Author(s): [morgo](http://github.com/morgo) +- Discussion PR: https://github.com/pingcap/tidb/pull/XXX +- Tracking Issue: https://github.com/pingcap/tidb/issues/30366 + +## Table of Contents + +* [Introduction](#introduction) +* [Motivation or Background](#motivation-or-background) +* [Detailed Design](#detailed-design) +* [Test Design](#test-design) +* [Impacts & Risks](#impacts--risks) +* [Investigation & Alternatives](#investigation--alternatives) +* [Unresolved Questions](#unresolved-questions) + +## Introduction + +Currently, TiDB has two primary methods of configuration: + +- The configuration file (in `.toml` format) ([Docs](https://docs.pingcap.com/tidb/stable/tidb-configuration-file)) +- Using MySQL compatible system variables (`SET GLOBAL sysvar=x`, `SET SESSION sysvar=x`) ([Docs](https://docs.pingcap.com/tidb/stable/system-variables)) + +Both the **semantics** and **naming conventions** differ between these two methods: + +1. Configuration file settings only apply to a single TiDB server instance. Making changes to settings managed by the configuration file requires restarting TiDB server. +2. System variables _natively_ manage settings that have _session_ (connection) or _global_ (cluster-wide) scope. However, some _session_ scoped variables are used to allow instance configuration. This is not a feature that is natively supported, and the usage is often confusing. +3. The configuration file uses a heirachy of sections such as `[performance]` or `[experimental]`. +4. System variables use are flat naming convention (but often use a prefix of `tidb_feature_XXX`). Thus mapping between the two is not straight forward. + +This proposal introduces _native_ support for `INSTANCE` scoped variables, such that system variables offer the superset of functionality of configuration files. It does this using a much simplified implementation from earlier proposals: an individual system variable **must not** permit both `GLOBAL` and `INSTANCE` scope. Implementing with this restriction is key to this proposal; since it avoids a confusing set of precedence rules which are hard to understand. For alternative proposals see "investigation and alternatives". + +## Motivation or Background + +The motivation for this proposal is ease of use and maintainability: + +1. It's not clear if each setting should be a configuration file setting or a system variable (we usually chose system variable, but there is no clear rule). After this proposal is implemented, we can make every setting available as a system variable (even if read-only). +2. We are being asked to make additional configuration file settings dynamically configuration (via a system variable). When we do this, the naming is inconsistent. After this proposal is implemented, we can allow the `.toml` file to support an `[instance]` section where the flat named for system variables can be used. This makes it identical to how system variables are configured in MySQL. +3. The current system is hard for users. It's hard to explain the two systems, and hard to explain the differences between them. Often "INSTANCE" scoped system variables are incorrectly documented as `SESSION`, since it's not a native behavior. Explaining how `SESSION` scope is hijacked is difficult for MySQL users to understand, because usually changes in a different session should not affect your session. + +## Detailed Design + +### Stage 1: Initial Implementation + +From a user-oriented point of view, setting an instance scoped sysvar is the same as setting a `GLOBAL` variable: + +```sql +SET GLOBAL max_connections=1234; +``` + +Because `INSTANCE` and `GLOBAL` are mutually exclusive, the only semantic difference is that changes to `INSTANCE` scoped variables are not persisted and are not propagated to other TiDB servers. + +From a sysvar framework perspective the changes required are quite minimal. A new scope is added, and validating if setting permits `GLOBAL` scope also permits `INSTANCE` scope: + +``` ++++ b/sessionctx/variable/sysvar.go +@@ -56,6 +56,8 @@ const ( + ScopeGlobal ScopeFlag = 1 << 0 + // ScopeSession means the system variable can only be changed in current session. + ScopeSession ScopeFlag = 1 << 1 ++ // ScopeInstance means it is similar to global but doesn't propagate to other TiDB servers. ++ ScopeInstance ScopeFlag = 1 << 2 + + // TypeStr is the default + TypeStr TypeFlag = 0 +@@ -257,6 +259,11 @@ func (sv *SysVar) HasGlobalScope() bool { + return sv.Scope&ScopeGlobal != 0 + } + ++// HasInstanceScope returns true if the scope for the sysVar includes global or instance ++func (sv *SysVar) HasInstanceScope() bool { ++ return sv.Scope&ScopeInstance != 0 ++} ++ + // Validate checks if system variable satisfies specific restriction. + func (sv *SysVar) Validate(vars *SessionVars, value string, scope ScopeFlag) (string, error) { + // Check that the scope is correct first. +@@ -313,7 +320,7 @@ func (sv *SysVar) validateScope(scope ScopeFlag) error { + if sv.ReadOnly || sv.Scope == ScopeNone { + return ErrIncorrectScope.FastGenByArgs(sv.Name, "read only") + } +- if scope == ScopeGlobal && !sv.HasGlobalScope() { ++ if scope == ScopeGlobal && !(sv.HasGlobalScope() || sv.HasInstanceScope()) { +``` + +The session package handles loading/saving GLOBAL variable values. It will need minor changes to make Instance Scope a noop operation (since persistence is not supported, and in the initial implementation a `GetGlobal()` function will be used to retrieve the instance value: + +``` ++++ b/session/session.go +@@ -1103,6 +1103,10 @@ func (s *session) GetGlobalSysVar(name string) (string, error) { + return "", variable.ErrUnknownSystemVar.GenWithStackByArgs(name) + } + ++ if sv.HasInstanceScope() { // has INSTANCE scope only, not pure global ++ return "", errors.New("variable has only instance scope and no GetGlobal func. Not sure how to handle yet.") ++ } ++ + sysVar, err := domain.GetDomain(s).GetGlobalVar(name) + if err != nil { + // The sysvar exists, but there is no cache entry yet. +@@ -1121,6 +1125,7 @@ func (s *session) GetGlobalSysVar(name string) (string, error) { + } + + // SetGlobalSysVar implements GlobalVarAccessor.SetGlobalSysVar interface. ++// it is used for setting instance scope as well. + func (s *session) SetGlobalSysVar(name, value string) (err error) { + sv := variable.GetSysVar(name) + if sv == nil { +@@ -1132,6 +1137,9 @@ func (s *session) SetGlobalSysVar(name, value string) (err error) { + if err = sv.SetGlobalFromHook(s.sessionVars, value, false); err != nil { + return err + } ++ if sv.HasInstanceScope() { // skip for INSTANCE scope ++ return nil ++ } + if sv.GlobalConfigName != "" { + domain.GetDomain(s).NotifyGlobalConfigChange(sv.GlobalConfigName, variable.OnOffToTrueFalse(value)) + } +@@ -1148,6 +1156,9 @@ func (s *session) SetGlobalSysVarOnly(name, value string) (err error) { + if err = sv.SetGlobalFromHook(s.sessionVars, value, true); err != nil { + return err + } ++ if !sv.HasInstanceScope() { // skip for INSTANCE scope ++ return nil ++ } + return s.replaceGlobalVariablesTableValue(context.TODO(), sv.Name, value) + } +``` + +A "non-native" `INSTANCE` variable can be changed to a native one as follows: + +``` ++ {Scope: ScopeInstance, Name: TiDBGeneralLog, Value: BoolToOnOff(DefTiDBGeneralLog), Type: TypeBool, skipInit: true, SetGlobal: func(s *SessionVars, val string) error { + ProcessGeneralLog.Store(TiDBOptOn(val)) + return nil +- }, GetSession: func(s *SessionVars) (string, error) { ++ }, GetGlobal: func(s *SessionVars) (string, error) { + return BoolToOnOff(ProcessGeneralLog.Load()), nil + }}, +``` + +This introduces a compatibility issue, since users who have previously configured instance scope with `SET [SESSION] tidb_general_log = x` will receive an error because the scope is wrong. This can be fixed by adding a legacy mode to the sysvar framework which allows `INSTANCE` scoped variables to be set with either `SET GLOBAL` or `SET SESSION`: + +```sql +SET GLOBAL tidb_enable_legacy_instance_scope = 1; +``` + +We can default to `TRUE` for now, but should revisit this in the future. + +### Stage 2: Mapping All Configuration File Settings to System Variables + +The second stage is to map all configuration file settings to system variable names. In MySQL any configuration file setting is also available as a system variable (even if it is read only), which makes it possible to get the configuration of the cluster with `SHOW GLOBAL VARIABLES`. Once this step is complete, it should also be possible to offer an `[instance]` section of the configuration file which accepts the system variable names (and finally consistent naming between the two systems). We can then modify the example configuration files to be based on the flat `[instance]` configuration: + +``` +# tidb.toml +[instance] +max_connections = 1234 +tidb_general_log = "/path/to/file" +``` + +### Stage 3: Refactoring + +The use of a `GetGlobal()` and `SetGlobal()` func for each instance scoped system variable is not ideal. It is possible to refactor the system variable framework so that instance scope is stored in a map, and the values are updated automatically by `SET GLOBAL` on an instance scoped variable. On startup, as the configuration file is parsed it will update the values in the map. This seems like a better approach than the current use of Setters/Getters, and because there is a prescribed way of doing it we can correctly handle the data races that are common with our current incorrect usage of calling `config.GetGlobalConfig()`. + +Thus, the source of truth for instance scoped variables moves from the `config` package to another part of the server (likely `domain`). See [issue #30366](https://github.com/pingcap/tidb/issues/30366). + +## Impacts & Risks + +Biggest risk is that we can not agree on a reduced scope of implementation and the project extends to cover increased scope. Configuration management is a classic example of a [bikeshed problem](https://en.wikipedia.org/wiki/Law_of_triviality), and we will likely need to make some tradeoffs to get anywhere. + +Many of the alternatives are difficult to implement because they break compatibility (we need to live with `GLOBAL` means cluster `GLOBAL`, and there is no `SET CLUSTER`) or they break rolling upgrade scenarios because we currently have no specific rules of the minimum version of TiDB which can be upgraded from. + +## Investigation & Alternatives + +- There is no equiverlant functionality to reference in MySQL since it does not have the native concept of a cluster. +- An [earlier proposal](https://docs.google.com/document/d/1RuajYAFVsjJCwCBpIYF--9jtgxDZZcz3cbP-iVuLxJI/edit) for `INSTANCE` scope, with rules for precedence (same author) +- CockroachDB has cluster level settings and node-level settings. Most settings are cluster level, and [node-level](https://www.cockroachlabs.com/docs/v21.2/cockroach-start) needs to be parsed as arguments when starting the server. + +## Unresolved Questions + +There are **very few** known scenarios where a system variable is *currently both* instance and global scoped, but these individually would need to be handled: + +- `tidb_store_limit`: Suggestion is to convert to global only ([issue #30515](https://github.com/pingcap/tidb/issues/30515)) +- `tidb_stmt_summary_XXX`: Suggestion is to convert to global only. +- `tidb_enable_stmt_summary`: Convert to instance scope only. + +Thus, turning on statement summary is a per-server decision, but the configuration for statement summary is a per-cluster decision. From 23d3d533fbb88ffd8b7e06d48854baa67245e018 Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Wed, 8 Dec 2021 16:54:22 -0700 Subject: [PATCH 04/14] Remove rogue files, add link --- docs/design/2021-12-08-instance-scope.md | 2 +- executor/set_test.go | 11 ++++++++--- sessionctx/variable/sysvar.go | 5 ++++- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/docs/design/2021-12-08-instance-scope.md b/docs/design/2021-12-08-instance-scope.md index eef40298b7987..d78eadbe29b1b 100644 --- a/docs/design/2021-12-08-instance-scope.md +++ b/docs/design/2021-12-08-instance-scope.md @@ -1,7 +1,7 @@ # TiDB Design Documents - Author(s): [morgo](http://github.com/morgo) -- Discussion PR: https://github.com/pingcap/tidb/pull/XXX +- Discussion PR: https://github.com/pingcap/tidb/pull/30558 - Tracking Issue: https://github.com/pingcap/tidb/issues/30366 ## Table of Contents diff --git a/executor/set_test.go b/executor/set_test.go index 425edf1c62ceb..2595b4a3131f5 100644 --- a/executor/set_test.go +++ b/executor/set_test.go @@ -366,11 +366,16 @@ func (s *testSerialSuite1) TestSetVar(c *C) { tk.MustExec("set @@tidb_expensive_query_time_threshold=70") tk.MustQuery("select @@tidb_expensive_query_time_threshold;").Check(testkit.Rows("70")) + tk.MustQuery("select @@tidb_store_limit;").Check(testkit.Rows("0")) + tk.MustExec("set @@tidb_store_limit = 100") + tk.MustQuery("select @@tidb_store_limit;").Check(testkit.Rows("100")) + tk.MustQuery("select @@session.tidb_store_limit;").Check(testkit.Rows("100")) tk.MustQuery("select @@global.tidb_store_limit;").Check(testkit.Rows("0")) - tk.MustExec("set @@global.tidb_store_limit = 100") - tk.MustQuery("select @@global.tidb_store_limit;").Check(testkit.Rows("100")) - tk.MustExec("set @@global.tidb_store_limit = 0") + tk.MustExec("set @@tidb_store_limit = 0") + tk.MustExec("set global tidb_store_limit = 100") + tk.MustQuery("select @@tidb_store_limit;").Check(testkit.Rows("100")) + tk.MustQuery("select @@session.tidb_store_limit;").Check(testkit.Rows("100")) tk.MustQuery("select @@global.tidb_store_limit;").Check(testkit.Rows("100")) tk.MustQuery("select @@session.tidb_metric_query_step;").Check(testkit.Rows("60")) diff --git a/sessionctx/variable/sysvar.go b/sessionctx/variable/sysvar.go index 14fecee0a7b5c..a1fcf7e025053 100644 --- a/sessionctx/variable/sysvar.go +++ b/sessionctx/variable/sysvar.go @@ -1587,7 +1587,10 @@ var defaultSysVars = []*SysVar{ } return nil }}, - {Scope: ScopeGlobal, Name: TiDBStoreLimit, Value: strconv.FormatInt(atomic.LoadInt64(&config.GetGlobalConfig().TiKVClient.StoreLimit), 10), Type: TypeInt, MinValue: 0, MaxValue: math.MaxInt64, GetGlobal: func(s *SessionVars) (string, error) { + {Scope: ScopeGlobal | ScopeSession, Name: TiDBStoreLimit, Value: strconv.FormatInt(atomic.LoadInt64(&config.GetGlobalConfig().TiKVClient.StoreLimit), 10), Type: TypeInt, MinValue: 0, MaxValue: math.MaxInt64, SetSession: func(s *SessionVars, val string) error { + tikvstore.StoreLimit.Store(tidbOptInt64(val, DefTiDBStoreLimit)) + return nil + }, GetSession: func(s *SessionVars) (string, error) { return strconv.FormatInt(tikvstore.StoreLimit.Load(), 10), nil }, SetGlobal: func(s *SessionVars, val string) error { tikvstore.StoreLimit.Store(tidbOptInt64(val, DefTiDBStoreLimit)) From d538a7d7cd29e3c84144c5c77f6311393480f054 Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Wed, 8 Dec 2021 17:59:45 -0700 Subject: [PATCH 05/14] add code to show if a sysvar is scoped both instance and global --- docs/design/2021-12-08-instance-scope.md | 35 ++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/docs/design/2021-12-08-instance-scope.md b/docs/design/2021-12-08-instance-scope.md index d78eadbe29b1b..e2ae994386fdb 100644 --- a/docs/design/2021-12-08-instance-scope.md +++ b/docs/design/2021-12-08-instance-scope.md @@ -180,8 +180,43 @@ Many of the alternatives are difficult to implement because they break compatibi There are **very few** known scenarios where a system variable is *currently both* instance and global scoped, but these individually would need to be handled: +``` +package main + +import ( + "fmt" + + _ "github.com/go-sql-driver/mysql" + "github.com/pingcap/tidb/sessionctx/variable" +) + +func main() { + for _, v := range variable.GetSysVars() { + if v.HasGlobalScope() && v.HasSessionScope() && (v.GetSession != nil || v.SetSession != nil) && (v.SetGlobal != nil || v.SetGlobal != nil) { + fmt.Printf("%s\n", v.Name) + } + } +} +``` + +Output: + +``` +tidb_store_limit +tidb_stmt_summary_internal_query +tidb_enable_stmt_summary +tidb_stmt_summary_max_sql_length +tidb_stmt_summary_max_stmt_count +tidb_capture_plan_baselines +tidb_stmt_summary_refresh_interval +tidb_stmt_summary_history_size +``` + +The following suggestions are provided (to be discussed): + - `tidb_store_limit`: Suggestion is to convert to global only ([issue #30515](https://github.com/pingcap/tidb/issues/30515)) - `tidb_stmt_summary_XXX`: Suggestion is to convert to global only. - `tidb_enable_stmt_summary`: Convert to instance scope only. +- `tidb_capture_plan_baselines`: Convert to instance scope only. Thus, turning on statement summary is a per-server decision, but the configuration for statement summary is a per-cluster decision. From 253926ef4dac066ba79359be0df80652d91a70e3 Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Wed, 8 Dec 2021 18:15:18 -0700 Subject: [PATCH 06/14] add clarification of default scope in stage 2 --- docs/design/2021-12-08-instance-scope.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/design/2021-12-08-instance-scope.md b/docs/design/2021-12-08-instance-scope.md index e2ae994386fdb..f49b9dab1a16b 100644 --- a/docs/design/2021-12-08-instance-scope.md +++ b/docs/design/2021-12-08-instance-scope.md @@ -158,6 +158,8 @@ max_connections = 1234 tidb_general_log = "/path/to/file" ``` +The default will be that we map variables as read-only, so we don't need to think about the specific cases (i.e. you can't easily change the socket or port). This helps us achieve completeness first, and then we can then evaluate which ones can be made dynamic. + ### Stage 3: Refactoring The use of a `GetGlobal()` and `SetGlobal()` func for each instance scoped system variable is not ideal. It is possible to refactor the system variable framework so that instance scope is stored in a map, and the values are updated automatically by `SET GLOBAL` on an instance scoped variable. On startup, as the configuration file is parsed it will update the values in the map. This seems like a better approach than the current use of Setters/Getters, and because there is a prescribed way of doing it we can correctly handle the data races that are common with our current incorrect usage of calling `config.GetGlobalConfig()`. From 154c98ed5ccd0fdd422735fd4d52d3d213327c86 Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Wed, 8 Dec 2021 20:27:01 -0700 Subject: [PATCH 07/14] update variable to GLOBAL per eason feedback --- docs/design/2021-12-08-instance-scope.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/design/2021-12-08-instance-scope.md b/docs/design/2021-12-08-instance-scope.md index f49b9dab1a16b..ad28a9ebac5a5 100644 --- a/docs/design/2021-12-08-instance-scope.md +++ b/docs/design/2021-12-08-instance-scope.md @@ -219,6 +219,6 @@ The following suggestions are provided (to be discussed): - `tidb_store_limit`: Suggestion is to convert to global only ([issue #30515](https://github.com/pingcap/tidb/issues/30515)) - `tidb_stmt_summary_XXX`: Suggestion is to convert to global only. - `tidb_enable_stmt_summary`: Convert to instance scope only. -- `tidb_capture_plan_baselines`: Convert to instance scope only. +- `tidb_capture_plan_baselines`: Convert to global scope only. Thus, turning on statement summary is a per-server decision, but the configuration for statement summary is a per-cluster decision. From ef2eefc8f96b57128e6925511cd76e8895d8bc6d Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Thu, 9 Dec 2021 09:10:02 -0700 Subject: [PATCH 08/14] Fix sample code --- docs/design/2021-12-08-instance-scope.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/design/2021-12-08-instance-scope.md b/docs/design/2021-12-08-instance-scope.md index ad28a9ebac5a5..10f850883c36f 100644 --- a/docs/design/2021-12-08-instance-scope.md +++ b/docs/design/2021-12-08-instance-scope.md @@ -194,7 +194,7 @@ import ( func main() { for _, v := range variable.GetSysVars() { - if v.HasGlobalScope() && v.HasSessionScope() && (v.GetSession != nil || v.SetSession != nil) && (v.SetGlobal != nil || v.SetGlobal != nil) { + if v.HasGlobalScope() && v.HasSessionScope() && (v.GetSession != nil || v.SetSession != nil) && (v.GetGlobal != nil || v.SetGlobal != nil) { fmt.Printf("%s\n", v.Name) } } From 8843d8947e304d77e98b27bb8900a9d6352e557f Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Thu, 9 Dec 2021 14:14:43 -0700 Subject: [PATCH 09/14] Fix typos and spelling --- docs/design/2021-12-08-instance-scope.md | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/docs/design/2021-12-08-instance-scope.md b/docs/design/2021-12-08-instance-scope.md index ad28a9ebac5a5..8e892826a71d0 100644 --- a/docs/design/2021-12-08-instance-scope.md +++ b/docs/design/2021-12-08-instance-scope.md @@ -23,18 +23,18 @@ Currently, TiDB has two primary methods of configuration: Both the **semantics** and **naming conventions** differ between these two methods: -1. Configuration file settings only apply to a single TiDB server instance. Making changes to settings managed by the configuration file requires restarting TiDB server. +1. Configuration file settings only apply to a single TiDB server instance. Making changes to settings managed by the configuration file requires restarting the TiDB server. 2. System variables _natively_ manage settings that have _session_ (connection) or _global_ (cluster-wide) scope. However, some _session_ scoped variables are used to allow instance configuration. This is not a feature that is natively supported, and the usage is often confusing. -3. The configuration file uses a heirachy of sections such as `[performance]` or `[experimental]`. -4. System variables use are flat naming convention (but often use a prefix of `tidb_feature_XXX`). Thus mapping between the two is not straight forward. +3. The configuration file uses a hierarchy of sections such as `[performance]` or `[experimental]`. +4. System variables use a flat naming convention (but often use a prefix of `tidb_feature_XXX`). Thus mapping between the two is not straight forward. -This proposal introduces _native_ support for `INSTANCE` scoped variables, such that system variables offer the superset of functionality of configuration files. It does this using a much simplified implementation from earlier proposals: an individual system variable **must not** permit both `GLOBAL` and `INSTANCE` scope. Implementing with this restriction is key to this proposal; since it avoids a confusing set of precedence rules which are hard to understand. For alternative proposals see "investigation and alternatives". +This proposal introduces _native_ support for `INSTANCE` scoped variables, such that system variables offer the superset of functionality of configuration files. It does this using a much simplified implementation from earlier proposals: an individual system variable **must not** permit both `GLOBAL` and `INSTANCE` scope. Implementing this restriction is key to this proposal; since it avoids a confusing set of precedence rules which are hard to understand. For alternative proposals see "investigation and alternatives". ## Motivation or Background The motivation for this proposal is ease of use and maintainability: -1. It's not clear if each setting should be a configuration file setting or a system variable (we usually chose system variable, but there is no clear rule). After this proposal is implemented, we can make every setting available as a system variable (even if read-only). +1. It's not clear if each setting should be a configuration file setting or a system variable (we usually choose system variables, but there is no clear rule). After this proposal is implemented, we can make every setting available as a system variable (even if read-only). 2. We are being asked to make additional configuration file settings dynamically configuration (via a system variable). When we do this, the naming is inconsistent. After this proposal is implemented, we can allow the `.toml` file to support an `[instance]` section where the flat named for system variables can be used. This makes it identical to how system variables are configured in MySQL. 3. The current system is hard for users. It's hard to explain the two systems, and hard to explain the differences between them. Often "INSTANCE" scoped system variables are incorrectly documented as `SESSION`, since it's not a native behavior. Explaining how `SESSION` scope is hijacked is difficult for MySQL users to understand, because usually changes in a different session should not affect your session. @@ -83,7 +83,7 @@ From a sysvar framework perspective the changes required are quite minimal. A ne + if scope == ScopeGlobal && !(sv.HasGlobalScope() || sv.HasInstanceScope()) { ``` -The session package handles loading/saving GLOBAL variable values. It will need minor changes to make Instance Scope a noop operation (since persistence is not supported, and in the initial implementation a `GetGlobal()` function will be used to retrieve the instance value: +The session package handles loading/saving `GLOBAL` variable values. It will need minor changes to make `INSTANCE` Scope a noop operation (since persistence is not supported, and in the initial implementation a `GetGlobal()` function will be used to retrieve the instance value: ``` +++ b/session/session.go @@ -168,13 +168,13 @@ Thus, the source of truth for instance scoped variables moves from the `config` ## Impacts & Risks -Biggest risk is that we can not agree on a reduced scope of implementation and the project extends to cover increased scope. Configuration management is a classic example of a [bikeshed problem](https://en.wikipedia.org/wiki/Law_of_triviality), and we will likely need to make some tradeoffs to get anywhere. +The biggest risk is that we can not agree on a reduced scope of implementation and the project extends to cover increased scope. Configuration management is a classic example of a [bikeshed problem](https://en.wikipedia.org/wiki/Law_of_triviality), and we will likely need to make some tradeoffs to get anywhere. Many of the alternatives are difficult to implement because they break compatibility (we need to live with `GLOBAL` means cluster `GLOBAL`, and there is no `SET CLUSTER`) or they break rolling upgrade scenarios because we currently have no specific rules of the minimum version of TiDB which can be upgraded from. ## Investigation & Alternatives -- There is no equiverlant functionality to reference in MySQL since it does not have the native concept of a cluster. +- There is no equivalent functionality to reference in MySQL since it does not have the native concept of a cluster. - An [earlier proposal](https://docs.google.com/document/d/1RuajYAFVsjJCwCBpIYF--9jtgxDZZcz3cbP-iVuLxJI/edit) for `INSTANCE` scope, with rules for precedence (same author) - CockroachDB has cluster level settings and node-level settings. Most settings are cluster level, and [node-level](https://www.cockroachlabs.com/docs/v21.2/cockroach-start) needs to be parsed as arguments when starting the server. @@ -201,7 +201,7 @@ func main() { } ``` -Output: +**Output:** ``` tidb_store_limit @@ -216,7 +216,7 @@ tidb_stmt_summary_history_size The following suggestions are provided (to be discussed): -- `tidb_store_limit`: Suggestion is to convert to global only ([issue #30515](https://github.com/pingcap/tidb/issues/30515)) +- `tidb_store_limit`: Suggestion is to convert to global only (Currently does not work correctly, see [issue #30515](https://github.com/pingcap/tidb/issues/30515)) - `tidb_stmt_summary_XXX`: Suggestion is to convert to global only. - `tidb_enable_stmt_summary`: Convert to instance scope only. - `tidb_capture_plan_baselines`: Convert to global scope only. From 3274f697de454870ceb70f2d78a833023ef56e46 Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Fri, 10 Dec 2021 10:45:33 -0700 Subject: [PATCH 10/14] Incorporate feedback --- docs/design/2021-12-08-instance-scope.md | 73 ++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 5 deletions(-) diff --git a/docs/design/2021-12-08-instance-scope.md b/docs/design/2021-12-08-instance-scope.md index ce61f0488f6a9..696710bc52a42 100644 --- a/docs/design/2021-12-08-instance-scope.md +++ b/docs/design/2021-12-08-instance-scope.md @@ -9,6 +9,7 @@ * [Introduction](#introduction) * [Motivation or Background](#motivation-or-background) * [Detailed Design](#detailed-design) +* [Documentation Changes](#documentation-changes) * [Test Design](#test-design) * [Impacts & Risks](#impacts--risks) * [Investigation & Alternatives](#investigation--alternatives) @@ -160,12 +161,76 @@ tidb_general_log = "/path/to/file" The default will be that we map variables as read-only, so we don't need to think about the specific cases (i.e. you can't easily change the socket or port). This helps us achieve completeness first, and then we can then evaluate which ones can be made dynamic. +The mapping system should consider all current instance variable mappings such as `log.enable-slow-log` to `tidb_enable_slow_log`. The earlier instance scoped variables proposal has an [example mapping table](https://docs.google.com/document/d/1RuajYAFVsjJCwCBpIYF--9jtgxDZZcz3cbP-iVuLxJI/edit?n=2020-09-15_Design_Doc_for_Instance_Variables#) which can be used as a guideline for how to create new system variable names. + +Because variables can be configured through either an "sysvar name" (under `[instance]`) or a heirachical name, we will need to decide which is the alias versus the actual name (the actual name appears in results like `SELECT * FROM INFORMATION_SCHEMA.CLUSTER_CONFIG`). I propose for this stage the original name is still the source of truth, but in refactoring (stage 3) we switch it. + ### Stage 3: Refactoring The use of a `GetGlobal()` and `SetGlobal()` func for each instance scoped system variable is not ideal. It is possible to refactor the system variable framework so that instance scope is stored in a map, and the values are updated automatically by `SET GLOBAL` on an instance scoped variable. On startup, as the configuration file is parsed it will update the values in the map. This seems like a better approach than the current use of Setters/Getters, and because there is a prescribed way of doing it we can correctly handle the data races that are common with our current incorrect usage of calling `config.GetGlobalConfig()`. Thus, the source of truth for instance scoped variables moves from the `config` package to another part of the server (likely `domain`). See [issue #30366](https://github.com/pingcap/tidb/issues/30366). +Because in this stage the source of truth is now no longer the `config` package, we will also need to decide how to handle features like `INFORMATION_SCHEMA.CLUSTER_CONFIG`. If it refers to the config file, it will not necessarily reflect the current configuration of the cluster. Because every instance setting will now have a system variable name (which becomes the unified name), I recommend that we deprecate `CLUSTER_CONFIG` for TiDB. We can change `CLUSTER_CONFIG` to read from the new source of truth and maintain both for some versions to support upgrades. + +## Documentation Changes + +For the purposes of user-communication we dont need to use the terminology `INSTANCE`. We will remove more of the confusing by instead refering to the difference between these as whether it "persists to [the] cluster". This also aligns closer to the syntax that users will be using. Consider the following example changes which are easier to read (the current text also doesn't actually explain that to set an `INSTANCE` variable you must use `SET SESSION`, so it actually communicates a lot more in fewer words): + +``` ++++ b/system-variables.md +@@ -6,13 +6,11 @@ aliases: ['/tidb/dev/tidb-specific-system-variables','/docs/dev/system-variables + + # System Variables + +-TiDB system variables behave similar to MySQL with some differences, in that settings might apply on a `SESSION`, `INSTANCE`, or `GLOBAL` scope, or on a scope that combines `SESSION`, `INSTANCE`, or `GLOBAL`. ++TiDB system variables behave similar to MySQL, in that settings apply on a `SESSION` or `GLOBAL` scope: + +-- Changes to `GLOBAL` scoped variables **only apply to new connection sessions with TiDB**. Currently active connection sessions are not affected. These changes are persisted and valid after restarts. +-- Changes to `INSTANCE` scoped variables apply to all active or new connection sessions with the current TiDB instance immediately after the changes are made. Other TiDB instances are not affected. These changes are not persisted and become invalid after TiDB restarts. +-- Variables can also have `NONE` scope. These variables are read-only, and are typically used to convey static information that will not change after a TiDB server has started. +- +-Variables can be set with the [`SET` statement](/sql-statements/sql-statement-set-variable.md) on a per-session, instance or global basis: ++- Changes on a `SESSION` scope will only affect the current session. ++- Changes on a `GLOBAL` scope apply immediately, provided that the variable is not also `SESSION` scoped. In which case all sessions (including your session) will continue to use their current session value. ++- Changes are made using the [`SET` statement](/sql-statements/sql-statement-set-variable.md): + + ```sql + # These two identical statements change a session variable +@@ -26,9 +24,9 @@ SET GLOBAL tidb_distsql_scan_concurrency = 10; + + > **Note:** + > +-> Executing `SET GLOBAL` applies immediately on the TiDB server where the statement was issued. A notification is then sent to all TiDB servers to refresh their system variable cache, which will start immediately as a background operation. Because there is a risk that some TiDB servers might miss the notification, the system variable cache is also refreshed automatically every 30 seconds. This helps ensure that all servers are operating with the same configuration. ++> Several `GLOBAL` variables persist to the TiDB cluster. For variables that specify `Persists to Cluster: Yes` a notification is sent to all TiDB servers to refresh their system variable cache when the global variable is changed. Adding additional TiDB servers (or restarting existing TiDB servers) will automatically use the persisted configuration value. For variables that specify `Persists to Cluster: No` any changes only apply to the local TiDB instance that you are connected to. In order to retain any values set, you will need to specify them in your `tidb.toml` configuration file. + > +-> TiDB differs from MySQL in that `GLOBAL` scoped variables **persist** through TiDB server restarts. Additionally, TiDB presents several MySQL variables as both readable and settable. This is required for compatibility, because it is common for both applications and connectors to read MySQL variables. For example, JDBC connectors both read and set query cache settings, despite not relying on the behavior. ++> Additionally, TiDB presents several MySQL variables as both readable and settable. This is required for compatibility, because it is common for both applications and connectors to read MySQL variables. For example, JDBC connectors both read and set query cache settings, despite not relying on the behavior. + + > **Note:** + > +@@ -47,6 +45,7 @@ SET GLOBAL tidb_distsql_scan_concurrency = 10; + ### allow_auto_random_explicit_insert New in v4.0.3 + + - Scope: SESSION | GLOBAL ++- Persists to cluster: Yes + - Default value: `OFF` + - Determines whether to allow explicitly specifying the values of the column with the `AUTO_RANDOM` attribute in the `INSERT` statement. + +@@ -166,7 +165,8 @@ mysql> SELECT * FROM t1; + + ### ddl_slow_threshold + +-- Scope: INSTANCE ++- Scope: GLOBAL ++- Persists to cluster: No + - Default value: `300` + - Unit: Milliseconds + - Log DDL operations whose execution time exceeds the threshold value. +``` + +What this does mean is that each variable that includes `GLOBAL` or `INSTANCE` scope needs a new line added in Docs. But this can be [auto-generated](https://github.com/pingcap/docs/pull/5720) from the sysvar source code. + ## Impacts & Risks The biggest risk is that we can not agree on a reduced scope of implementation and the project extends to cover increased scope. Configuration management is a classic example of a [bikeshed problem](https://en.wikipedia.org/wiki/Law_of_triviality), and we will likely need to make some tradeoffs to get anywhere. @@ -174,7 +239,7 @@ Many of the alternatives are difficult to implement because they break compatibi ## Investigation & Alternatives -- There is no equivalent functionality to reference in MySQL since it does not have the native concept of a cluster. +- There is no equivalent functionality to reference in MySQL since it does not have the native concept of a cluster _in the context of configuration_. - An [earlier proposal](https://docs.google.com/document/d/1RuajYAFVsjJCwCBpIYF--9jtgxDZZcz3cbP-iVuLxJI/edit) for `INSTANCE` scope, with rules for precedence (same author) - CockroachDB has cluster level settings and node-level settings. Most settings are cluster level, and [node-level](https://www.cockroachlabs.com/docs/v21.2/cockroach-start) needs to be parsed as arguments when starting the server. @@ -218,7 +283,5 @@ The following suggestions are provided (to be discussed): - `tidb_store_limit`: Suggestion is to convert to global only (Currently does not work correctly, see [issue #30515](https://github.com/pingcap/tidb/issues/30515)) - `tidb_stmt_summary_XXX`: Suggestion is to convert to global only. -- `tidb_enable_stmt_summary`: Convert to instance scope only. -- `tidb_capture_plan_baselines`: Convert to global scope only. - -Thus, turning on statement summary is a per-server decision, but the configuration for statement summary is a per-cluster decision. +- `tidb_enable_stmt_summary`: Convert to global scope only (updated 11 Dec 2021) +- `tidb_capture_plan_baselines`: Convert to global scope only. \ No newline at end of file From 914779469b04dab9036d7a024cc1983b5a3cc090 Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Fri, 10 Dec 2021 10:50:49 -0700 Subject: [PATCH 11/14] Small typos --- docs/design/2021-12-08-instance-scope.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/design/2021-12-08-instance-scope.md b/docs/design/2021-12-08-instance-scope.md index 696710bc52a42..282cdaaa4cda1 100644 --- a/docs/design/2021-12-08-instance-scope.md +++ b/docs/design/2021-12-08-instance-scope.md @@ -175,7 +175,7 @@ Because in this stage the source of truth is now no longer the `config` package, ## Documentation Changes -For the purposes of user-communication we dont need to use the terminology `INSTANCE`. We will remove more of the confusing by instead refering to the difference between these as whether it "persists to [the] cluster". This also aligns closer to the syntax that users will be using. Consider the following example changes which are easier to read (the current text also doesn't actually explain that to set an `INSTANCE` variable you must use `SET SESSION`, so it actually communicates a lot more in fewer words): +For the purposes of user-communication we don't need to use the terminology `INSTANCE`. We will remove more of the confusion by instead refering to the difference between these as whether it "persists to [the] cluster". This also aligns closer to the syntax that users will be using. Consider the following example changes which are easier to read (the current text also doesn't actually explain that to set an `INSTANCE` variable you must use `SET SESSION`, so it actually communicates a lot more in fewer words): ``` +++ b/system-variables.md From 9eda156bb5173129ce5fc9300f53588588f032c7 Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Wed, 15 Dec 2021 19:10:45 -0600 Subject: [PATCH 12/14] move behavior changes out of unresolved questions (we have a clear proposal) --- docs/design/2021-12-08-instance-scope.md | 91 +++++++++++++----------- 1 file changed, 48 insertions(+), 43 deletions(-) diff --git a/docs/design/2021-12-08-instance-scope.md b/docs/design/2021-12-08-instance-scope.md index 282cdaaa4cda1..ad07ab34f551b 100644 --- a/docs/design/2021-12-08-instance-scope.md +++ b/docs/design/2021-12-08-instance-scope.md @@ -9,6 +9,7 @@ * [Introduction](#introduction) * [Motivation or Background](#motivation-or-background) * [Detailed Design](#detailed-design) +* [Behavior Changes](#behavior-changes) * [Documentation Changes](#documentation-changes) * [Test Design](#test-design) * [Impacts & Risks](#impacts--risks) @@ -163,7 +164,7 @@ The default will be that we map variables as read-only, so we don't need to thin The mapping system should consider all current instance variable mappings such as `log.enable-slow-log` to `tidb_enable_slow_log`. The earlier instance scoped variables proposal has an [example mapping table](https://docs.google.com/document/d/1RuajYAFVsjJCwCBpIYF--9jtgxDZZcz3cbP-iVuLxJI/edit?n=2020-09-15_Design_Doc_for_Instance_Variables#) which can be used as a guideline for how to create new system variable names. -Because variables can be configured through either an "sysvar name" (under `[instance]`) or a heirachical name, we will need to decide which is the alias versus the actual name (the actual name appears in results like `SELECT * FROM INFORMATION_SCHEMA.CLUSTER_CONFIG`). I propose for this stage the original name is still the source of truth, but in refactoring (stage 3) we switch it. +Because variables can be configured through either an "sysvar name" (under `[instance]`) or a hierarchical name, we will need to decide which is the alias versus the actual name (the actual name appears in results like `SELECT * FROM INFORMATION_SCHEMA.CLUSTER_CONFIG`). I propose for this stage the original name is still the source of truth, but in refactoring (stage 3) we switch it. ### Stage 3: Refactoring @@ -171,11 +172,54 @@ The use of a `GetGlobal()` and `SetGlobal()` func for each instance scoped syste Thus, the source of truth for instance scoped variables moves from the `config` package to another part of the server (likely `domain`). See [issue #30366](https://github.com/pingcap/tidb/issues/30366). -Because in this stage the source of truth is now no longer the `config` package, we will also need to decide how to handle features like `INFORMATION_SCHEMA.CLUSTER_CONFIG`. If it refers to the config file, it will not necessarily reflect the current configuration of the cluster. Because every instance setting will now have a system variable name (which becomes the unified name), I recommend that we deprecate `CLUSTER_CONFIG` for TiDB. We can change `CLUSTER_CONFIG` to read from the new source of truth and maintain both for some versions to support upgrades. +Because at this stage the source of truth is now no longer the `config` package, we will also need to decide how to handle features like `INFORMATION_SCHEMA.CLUSTER_CONFIG`. If it refers to the config file, it will not necessarily reflect the current configuration of the cluster. Because every instance setting will now have a system variable name (which becomes the unified name), I recommend that we deprecate `CLUSTER_CONFIG` for TiDB. We can change `CLUSTER_CONFIG` to read from the new source of truth and maintain both for some versions to support upgrades. + +## Behavior Changes + +There are **very few** known scenarios where a system variable is *currently both* instance and global scoped, but each of these will require behavior changes. Here is a script to detect them: + +``` +package main + +import ( + "fmt" + + _ "github.com/go-sql-driver/mysql" + "github.com/pingcap/tidb/sessionctx/variable" +) + +func main() { + for _, v := range variable.GetSysVars() { + if v.HasGlobalScope() && v.HasSessionScope() && (v.GetSession != nil || v.SetSession != nil) && (v.GetGlobal != nil || v.SetGlobal != nil) { + fmt.Printf("%s\n", v.Name) + } + } +} +``` + +**Output:** + +``` +tidb_store_limit +tidb_stmt_summary_internal_query +tidb_enable_stmt_summary +tidb_stmt_summary_max_sql_length +tidb_stmt_summary_max_stmt_count +tidb_capture_plan_baselines +tidb_stmt_summary_refresh_interval +tidb_stmt_summary_history_size +``` + +Changes can be grouped into the following: + +1. `tidb_store_limit`: This has now been converted to global-only, see [issue #30515 (merged)](https://github.com/pingcap/tidb/issues/30515). +2. `tidb_stmt_summary_XXX`, `tidb_enable_stmt_summary` and `tidb_capture_plan_baselines` (features work together): The recommendation discussed with the feature maintainers is to convert to global only. + +The change to `tidb_store_limit` is unlikely to affect users, since the feature was not working correctly. However, the change to statement summary and capture plan baselines is a behavior change which might affect some users. ## Documentation Changes -For the purposes of user-communication we don't need to use the terminology `INSTANCE`. We will remove more of the confusion by instead refering to the difference between these as whether it "persists to [the] cluster". This also aligns closer to the syntax that users will be using. Consider the following example changes which are easier to read (the current text also doesn't actually explain that to set an `INSTANCE` variable you must use `SET SESSION`, so it actually communicates a lot more in fewer words): +For the purposes of user-communication we don't need to use the terminology `INSTANCE`. We will remove more of the confusion by instead referring to the difference between these as whether it "persists to [the] cluster". This also aligns closer to the syntax that users will be using. Consider the following example changes which are easier to read (the current text also doesn't actually explain that to set an `INSTANCE` variable you must use `SET SESSION`, so it actually communicates a lot more in fewer words): ``` +++ b/system-variables.md @@ -245,43 +289,4 @@ Many of the alternatives are difficult to implement because they break compatibi ## Unresolved Questions -There are **very few** known scenarios where a system variable is *currently both* instance and global scoped, but these individually would need to be handled: - -``` -package main - -import ( - "fmt" - - _ "github.com/go-sql-driver/mysql" - "github.com/pingcap/tidb/sessionctx/variable" -) - -func main() { - for _, v := range variable.GetSysVars() { - if v.HasGlobalScope() && v.HasSessionScope() && (v.GetSession != nil || v.SetSession != nil) && (v.GetGlobal != nil || v.SetGlobal != nil) { - fmt.Printf("%s\n", v.Name) - } - } -} -``` - -**Output:** - -``` -tidb_store_limit -tidb_stmt_summary_internal_query -tidb_enable_stmt_summary -tidb_stmt_summary_max_sql_length -tidb_stmt_summary_max_stmt_count -tidb_capture_plan_baselines -tidb_stmt_summary_refresh_interval -tidb_stmt_summary_history_size -``` - -The following suggestions are provided (to be discussed): - -- `tidb_store_limit`: Suggestion is to convert to global only (Currently does not work correctly, see [issue #30515](https://github.com/pingcap/tidb/issues/30515)) -- `tidb_stmt_summary_XXX`: Suggestion is to convert to global only. -- `tidb_enable_stmt_summary`: Convert to global scope only (updated 11 Dec 2021) -- `tidb_capture_plan_baselines`: Convert to global scope only. \ No newline at end of file +None \ No newline at end of file From 2519b6d6de3b95709a984a1c355853f7a848786f Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Mon, 17 Jan 2022 13:42:09 -0700 Subject: [PATCH 13/14] Add unresolved question --- docs/design/2021-12-08-instance-scope.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/docs/design/2021-12-08-instance-scope.md b/docs/design/2021-12-08-instance-scope.md index ad07ab34f551b..beba0560126d8 100644 --- a/docs/design/2021-12-08-instance-scope.md +++ b/docs/design/2021-12-08-instance-scope.md @@ -289,4 +289,11 @@ Many of the alternatives are difficult to implement because they break compatibi ## Unresolved Questions -None \ No newline at end of file +A small behavior change (still with the restriction that `INSTANCE` and `GLOBAL` are mutually exclusive) proposes adding `SET INSTANCE` as explicit syntax to set an instance-scoped variable. This would require the following modifications to be complete: +- `SHOW INSTANCE VARIABLES` will need to return variables that have `NONE` or `INSTANCE` scope (and `SHOW VARIABLES` continues to return all variables). +- The parser will need to support `SET INSTANCE` syntax and the `@@instance.variable` syntax +- Various parser ast code will also need modifying because a boolean can no longer reflect the scope. + +The advantage of this proposal is that the documentation and usage is clearer. The disadvantage is that because MySQL does not have an instance scope (global scope in MySQL is instance-like) it could create strange behaviors with compatibility sysvars which are implemented. This **only** affects `INSTANCE` variables which are *not already* `ScopeNone` (so Port, Socket, GrantTables, LogBin are not affected). The only currently known example that is affected is `max_connections`, which is currently a noop in system variables, but available in the configuration file as `max-server-connections` (it is presumed it would be [mapped](https://docs.google.com/document/d/1RuajYAFVsjJCwCBpIYF--9jtgxDZZcz3cbP-iVuLxJI/edit?n=2020-09-15_Design_Doc_for_Instance_Variables#) to `max_connections` in Stage 2). + +This means that `SET GLOBAL max_connections` would need to return an error in TiDB, because the correct syntax is `SET INSTANCE max_connections`. There is a known failover use-case to execute `SET GLOBAL max_connections = 1` + `SET GLOBAL [super_]read_only = 1` to disable writes. However, this already does not work in TiDB as there is no support for `SET GLOBAL [super_]read_only=1`. Thus, removing this compatibility does not appear to be a blocking issue. \ No newline at end of file From e54cff3674a94c95ae8c36b2e47b5b2a31b48cbe Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Mon, 17 Jan 2022 23:09:55 -0700 Subject: [PATCH 14/14] Reject SET INSTANCE proposal --- docs/design/2021-12-08-instance-scope.md | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/docs/design/2021-12-08-instance-scope.md b/docs/design/2021-12-08-instance-scope.md index beba0560126d8..9b6a58199b297 100644 --- a/docs/design/2021-12-08-instance-scope.md +++ b/docs/design/2021-12-08-instance-scope.md @@ -287,7 +287,7 @@ Many of the alternatives are difficult to implement because they break compatibi - An [earlier proposal](https://docs.google.com/document/d/1RuajYAFVsjJCwCBpIYF--9jtgxDZZcz3cbP-iVuLxJI/edit) for `INSTANCE` scope, with rules for precedence (same author) - CockroachDB has cluster level settings and node-level settings. Most settings are cluster level, and [node-level](https://www.cockroachlabs.com/docs/v21.2/cockroach-start) needs to be parsed as arguments when starting the server. -## Unresolved Questions +### SET INSTANCE syntax A small behavior change (still with the restriction that `INSTANCE` and `GLOBAL` are mutually exclusive) proposes adding `SET INSTANCE` as explicit syntax to set an instance-scoped variable. This would require the following modifications to be complete: - `SHOW INSTANCE VARIABLES` will need to return variables that have `NONE` or `INSTANCE` scope (and `SHOW VARIABLES` continues to return all variables). @@ -296,4 +296,10 @@ A small behavior change (still with the restriction that `INSTANCE` and `GLOBAL` The advantage of this proposal is that the documentation and usage is clearer. The disadvantage is that because MySQL does not have an instance scope (global scope in MySQL is instance-like) it could create strange behaviors with compatibility sysvars which are implemented. This **only** affects `INSTANCE` variables which are *not already* `ScopeNone` (so Port, Socket, GrantTables, LogBin are not affected). The only currently known example that is affected is `max_connections`, which is currently a noop in system variables, but available in the configuration file as `max-server-connections` (it is presumed it would be [mapped](https://docs.google.com/document/d/1RuajYAFVsjJCwCBpIYF--9jtgxDZZcz3cbP-iVuLxJI/edit?n=2020-09-15_Design_Doc_for_Instance_Variables#) to `max_connections` in Stage 2). -This means that `SET GLOBAL max_connections` would need to return an error in TiDB, because the correct syntax is `SET INSTANCE max_connections`. There is a known failover use-case to execute `SET GLOBAL max_connections = 1` + `SET GLOBAL [super_]read_only = 1` to disable writes. However, this already does not work in TiDB as there is no support for `SET GLOBAL [super_]read_only=1`. Thus, removing this compatibility does not appear to be a blocking issue. \ No newline at end of file +This means that `SET GLOBAL max_connections` would need to return an error in TiDB, because the correct syntax is `SET INSTANCE max_connections`. There is a known failover use-case to execute `SET GLOBAL max_connections = 1` + `SET GLOBAL [super_]read_only = 1` to disable writes. However, this already does not work in TiDB as there is no support for `SET GLOBAL [super_]read_only=1`. Thus, removing this compatibility does not appear to be a blocking issue. + +However, the IBG support team rejects this proposal based on the expectation that there will be further tool compatibility issues not yet discovered. As MySQL does not have `SET INSTANCE` scope, users will also not be used to using this syntax. + +## Unresolved Questions + +- None \ No newline at end of file