modify tidb_enable_historical_stats behavior

[morgo]

Enhancement

In #30646 we merged a new system variable: tidb_enable_historical_stats.

It uses the functions getTiDBTableValue() / setTiDBTableValue() to persist the saved values to the mysql.tidb table, but for new sysvars we don't need to do this. We should just rely on the built in saving/loading method.

The functions getTiDBTableValue() / setTiDBTableValue() are unfortunately required for existing variables related to garbage collection. This is because we do not have clear rules about upgrade/downgrade and need to support earlier versions of TiDB which read/write from these tables and do not use system variables.

We can not clean this up these functions until product management to define support versions/upgrade rules (cc @easonn7 ), but it does not have a use for new sysvars.

The advantage of using the built-in method is that the values are cached on each tidb-server in memory. If we add too many rows to the mysql.tidb table it will affect performance, because when SHOW VARIABLES is executed each value must be read one row at a time.

[morgo]

To whoever picks up this issue, please add deprecation warnings to the deprecated API calls:

--- a/sessionctx/variable/varsutil.go
+++ b/sessionctx/variable/varsutil.go
@@ -230,6 +230,10 @@ func SetStmtVar(vars *SessionVars, name string, value string) error {
        return vars.SetStmtVar(name, sVal)
 }
 
+// Deprecated: Read the value from the mysql.tidb table.
+// This supports the use case that a TiDB server *older* than 5.0 is a member of the cluster.
+// i.e. system variables such as tidb_gc_concurrency, tidb_gc_enable, tidb_gc_life_time
+// do not exist.
 func getTiDBTableValue(vars *SessionVars, name, defaultVal string) (string, error) {
        val, err := vars.GlobalVarsAccessor.GetTiDBTableValue(name)
        if err != nil { // handle empty result or other errors
@@ -238,6 +242,10 @@ func getTiDBTableValue(vars *SessionVars, name, defaultVal string) (string, erro
        return trueFalseToOnOff(val), nil
 }
 
+// Deprecated: Set the value from the mysql.tidb table.
+// This supports the use case that a TiDB server *older* than 5.0 is a member of the cluster.
+// i.e. system variables such as tidb_gc_concurrency, tidb_gc_enable, tidb_gc_life_time
+// do not exist.
 func setTiDBTableValue(vars *SessionVars, name, value, comment string) error {
        value = OnOffToTrueFalse(value)
        return vars.GlobalVarsAccessor.SetTiDBTableValue(name, value, comment)

[qw4990]

@An-DJ Please take a look at this issue.

[qw4990]

I have an additional question about this. If we don't persist this variable to the mysql.tidb table, how can other instances in the same cluster read its value?
According to my understanding, all global variables should be persisted into the storage(mysql.tidb here) so that other instances can load them asynchronously.
Please correct me if I miss anything @morgo

[morgo]

I have an additional question about this. If we don't persist this variable to the mysql.tidb table, how can other instances in the same cluster read its value? According to my understanding, all global variables should be persisted into the storage(mysql.tidb here) so that other instances can load them asynchronously. Please correct me if I miss anything @morgo

Global values are persisted into mysql.GLOBAL_VARIABLES. It is mainly just gc configuration that is persisted in mysql.tidb (we need to keep it there for the scenario that a TiDB 4.0 server still reads/writes it there).

But actually since ~TiDB 5.2 other servers don't read directly from mysql.GLOBAL_VARIABLES but from a cache: #24359 The cache is periodically refreshed on each TiDB instance, or when a global var is updated an etcd notification is sent to peers to start updating the cache now.

This cache is important because connectors like to do things like SHOW VARIABLES LIKE 'x' when they first connect to the TiDB server. x is not pushed down in any way, so in the previous design it read all the values from tikv one row at a time. With the cache it only reads the legacy cases which use the function getTiDBTableValue(), but if there is a bit of latency between tidb and tikv the overhead could be obersvable.