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

variable: fix information_schema.VARIABLES_INFO DEFAULT_VALUE not right problem #49524

Merged
merged 11 commits into from
Jan 11, 2024
Merged

variable: fix information_schema.VARIABLES_INFO DEFAULT_VALUE not right problem #49524

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b3c5c16
fix information_schema.VARIABLES_INFO DEFAULT_VALUE not right
jiyfhust Dec 17, 2023
5f3cab7
fix
jiyfhust Dec 18, 2023
0861ba6
fix
jiyfhust Dec 21, 2023
0bdec4e
add unittest
jiyfhust Dec 22, 2023
cb77afd
Merge branch 'pingcap:master' into fix_variables_info_defaultvalue
jiyfhust Jan 2, 2024
209f385
fix set global variables to default
jiyfhust Jan 2, 2024
0755997
add set global variable default value tests
jiyfhust Jan 2, 2024
21f0c53
fix test
jiyfhust Jan 2, 2024
0558302
fix
jiyfhust Jan 2, 2024
432625c
fix test
jiyfhust Jan 2, 2024
5697500
fix
jiyfhust Jan 2, 2024
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
6 changes: 5 additions & 1 deletion pkg/executor/infoschema_reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,10 +264,14 @@ func (e *memtableRetriever) setDataForVariablesInfo(ctx sessionctx.Context) erro
if sv.IsNoop {
isNoop = "YES"
}
defVal := sv.Value
if sv.HasGlobalScope() {
defVal = variable.GlobalSystemVariableInitialValue(sv.Name, defVal)
}
row := types.MakeDatums(
sv.Name, // VARIABLE_NAME
sv.Scope.String(), // VARIABLE_SCOPE
sv.Value, // DEFAULT_VALUE
defVal, // DEFAULT_VALUE
currentVal, // CURRENT_VALUE
sv.MinValue, // MIN_VALUE
sv.MaxValue, // MAX_VALUE
Expand Down
32 changes: 1 addition & 31 deletions pkg/session/bootstrap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3152,37 +3152,7 @@ func doDMLWorks(s sessiontypes.Session) {
if !v.HasGlobalScope() {
continue
}
vVal := v.Value
switch v.Name {
case variable.TiDBTxnMode:
if config.GetGlobalConfig().Store == "tikv" || config.GetGlobalConfig().Store == "unistore" {
vVal = "pessimistic"
}
case variable.TiDBEnableAsyncCommit, variable.TiDBEnable1PC:
if config.GetGlobalConfig().Store == "tikv" {
vVal = variable.On
}
case variable.TiDBMemOOMAction:
if intest.InTest {
vVal = variable.OOMActionLog
}
case variable.TiDBEnableAutoAnalyze:
if intest.InTest {
vVal = variable.Off
}
// For the following sysvars, we change the default
// FOR NEW INSTALLS ONLY. In most cases you don't want to do this.
// It is better to change the value in the Sysvar struct, so that
// all installs will have the same value.
case variable.TiDBRowFormatVersion:
vVal = strconv.Itoa(variable.DefTiDBRowFormatV2)
case variable.TiDBTxnAssertionLevel:
vVal = variable.AssertionFastStr
case variable.TiDBEnableMutationChecker:
vVal = variable.On
case variable.TiDBPessimisticTransactionFairLocking:
vVal = variable.On
}
vVal := variable.GlobalSystemVariableInitialValue(v.Name, v.Value)

// sanitize k and vVal
value := fmt.Sprintf(`("%s", "%s")`, sqlescape.EscapeString(k), sqlescape.EscapeString(vVal))
Expand Down
1 change: 1 addition & 0 deletions pkg/sessionctx/variable/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ go_library(
"//pkg/util/distrole",
"//pkg/util/execdetails",
"//pkg/util/gctuner",
"//pkg/util/intest",
"//pkg/util/kvcache",
"//pkg/util/logutil",
"//pkg/util/mathutil",
Expand Down
36 changes: 36 additions & 0 deletions pkg/sessionctx/variable/sysvar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import (
"github.com/pingcap/tidb/pkg/util/collate"
distroleutil "github.com/pingcap/tidb/pkg/util/distrole"
"github.com/pingcap/tidb/pkg/util/gctuner"
"github.com/pingcap/tidb/pkg/util/intest"
"github.com/pingcap/tidb/pkg/util/logutil"
"github.com/pingcap/tidb/pkg/util/mathutil"
"github.com/pingcap/tidb/pkg/util/memory"
Expand Down Expand Up @@ -2965,6 +2966,41 @@ var defaultSysVars = []*SysVar{
}},
}

// GlobalSystemVariableInitialValue gets the default value for a system variable including ones that are dynamically set (e.g. based on the store)
func GlobalSystemVariableInitialValue(varName, varVal string) string {
switch varName {
case TiDBTxnMode:
if config.GetGlobalConfig().Store == "tikv" || config.GetGlobalConfig().Store == "unistore" {
varVal = "pessimistic"
}
case TiDBEnableAsyncCommit, TiDBEnable1PC:
if config.GetGlobalConfig().Store == "tikv" {
varVal = On
Copy link
Member

Choose a reason for hiding this comment

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

Can we retrieve the default value from a centralized location instead of hard coding it here? This could be a problem if we ever need to change the default value in the future but forget to modify this here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

diff --git a/pkg/sessionctx/variable/sysvar.go b/pkg/sessionctx/variable/sysvar.go
index 532cd0f..e74cdf9 100644
--- a/pkg/sessionctx/variable/sysvar.go
+++ b/pkg/sessionctx/variable/sysvar.go
@@ -2093,7 +2093,7 @@ var defaultSysVars = []*SysVar{
                s.ShardAllocateStep = TidbOptInt64(val, DefTiDBShardAllocateStep)
                return nil
        }},
-       {Scope: ScopeGlobal | ScopeSession, Name: TiDBEnableAsyncCommit, Value: BoolToOnOff(DefTiDBEnableAsyncCommit), Type: TypeBool, 
+       {Scope: ScopeGlobal | ScopeSession, Name: TiDBEnableAsyncCommit, Value: defaultTiDBEnableAsyncCommit(), Type: TypeBool, SetSess
                s.EnableAsyncCommit = TiDBOptOn(val)
                return nil
        }},
@@ -2965,6 +2965,13 @@ var defaultSysVars = []*SysVar{
                }},
 }
 
+func defaultTiDBEnableAsyncCommit() string {
+       if config.GetGlobalConfig().Store == "tikv" {
+               return On
+       }
+       return BoolToOnOff(DefTiDBEnableAsyncCommit)
+}
+

I tried this way. But it seems defaultSysVars is inited early than config.GetGlobalConfig(), so it can't work.
Maybe we can set variable.GetSysVars after config.GetGlobalConfig() loaded, but it is also "hard coding".

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 we need set diffrent defalut value for diffrent store types for example TiDBEnableAsyncCommit，we need do that after config.GetGlobalConfig() loaded
  • others，we can move into defaultSysVars, for example TiDBRowFormatVersion.

/cc @JmPotato

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this PR has minimal changes to fix the bug. If we want a more elegant way to make it, some questions should be answered before:

  1. Do we need to keep the "right" default value for unistore which is only used for testing ? If we do not need it, we can make variables like TiDBEnableAsyncCommit configured statically without reading global config.
  2. If cluster upgrades from an old one. What is the right default value for the variables that don't want to change. Just like tidb_pessimistic_txn_fair_locking, it should keep the old default value 'OFF' after upgrading and 'ON' for a new cluster.

}
case TiDBMemOOMAction:
if intest.InTest {
varVal = OOMActionLog
}
case TiDBEnableAutoAnalyze:
if intest.InTest {
varVal = Off
}
// For the following sysvars, we change the default
// FOR NEW INSTALLS ONLY. In most cases you don't want to do this.
// It is better to change the value in the Sysvar struct, so that
// all installs will have the same value.
case TiDBRowFormatVersion:
varVal = strconv.Itoa(DefTiDBRowFormatV2)
case TiDBTxnAssertionLevel:
varVal = AssertionFastStr
case TiDBEnableMutationChecker:
varVal = On
case TiDBPessimisticTransactionFairLocking:
varVal = On
}
return varVal
}

func setTiFlashComputeDispatchPolicy(s *SessionVars, val string) error {
p, err := tiflashcompute.GetDispatchPolicyByStr(val)
if err != nil {
Expand Down
58 changes: 58 additions & 0 deletions pkg/sessionctx/variable/sysvar_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1453,3 +1453,61 @@ func TestSetTiDBCloudStorageURI(t *testing.T) {
require.Len(t, val, 0)
cancel()
}

func TestGlobalSystemVariableInitialValue(t *testing.T) {
vars := []struct {
name string
val string
initVal string
}{
{
TiDBTxnMode,
DefTiDBTxnMode,
"pessimistic",
},
{
TiDBEnableAsyncCommit,
BoolToOnOff(DefTiDBEnableAsyncCommit),
BoolToOnOff(DefTiDBEnableAsyncCommit),
},
{
TiDBEnable1PC,
BoolToOnOff(DefTiDBEnable1PC),
BoolToOnOff(DefTiDBEnable1PC),
},
{
TiDBMemOOMAction,
DefTiDBMemOOMAction,
OOMActionLog,
},
{
TiDBEnableAutoAnalyze,
BoolToOnOff(DefTiDBEnableAutoAnalyze),
Off,
},
{
TiDBRowFormatVersion,
strconv.Itoa(DefTiDBRowFormatV1),
strconv.Itoa(DefTiDBRowFormatV2),
},
{
TiDBTxnAssertionLevel,
DefTiDBTxnAssertionLevel,
AssertionFastStr,
},
{
TiDBEnableMutationChecker,
BoolToOnOff(DefTiDBEnableMutationChecker),
On,
},
{
TiDBPessimisticTransactionFairLocking,
BoolToOnOff(DefTiDBPessimisticTransactionFairLocking),
On,
},
}
for _, v := range vars {
initVal := GlobalSystemVariableInitialValue(v.name, v.val)
require.Equal(t, v.initVal, initVal)
}
}
11 changes: 11 additions & 0 deletions tests/integrationtest/r/executor/infoschema_reader.result
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,3 +283,14 @@ SELECT * FROM information_schema.user_privileges WHERE grantee="'usageuser'@'%'"
GRANTEE TABLE_CATALOG PRIVILEGE_TYPE IS_GRANTABLE
'usageuser'@'%' def BACKUP_ADMIN NO
'usageuser'@'%' def SELECT YES
select VARIABLE_NAME from information_schema.VARIABLES_INFO where DEFAULT_VALUE = CURRENT_VALUE and variable_name in ('tidb_txn_mode', 'tidb_enable_async_commit','tidb_enable_1pc', 'tidb_mem_oom_action', 'tidb_enable_auto_analyze', 'tidb_row_format_version', 'tidb_txn_assertion_level', 'tidb_enable_mutation_checker', 'tidb_pessimistic_txn_fair_locking') order by VARIABLE_NAME
VARIABLE_NAME
tidb_enable_1pc
tidb_enable_async_commit
tidb_enable_auto_analyze
tidb_enable_mutation_checker
tidb_mem_oom_action
tidb_pessimistic_txn_fair_locking
tidb_row_format_version
tidb_txn_assertion_level
tidb_txn_mode
3 changes: 3 additions & 0 deletions tests/integrationtest/t/executor/infoschema_reader.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,3 +239,6 @@ SELECT * FROM information_schema.user_privileges WHERE grantee="'usageuser'@'%'"

connection default;
disconnect conn1;

# test information_schema.VARIABLES_INFO DEFAULT_VALUE
select VARIABLE_NAME from information_schema.VARIABLES_INFO where DEFAULT_VALUE = CURRENT_VALUE and variable_name in ('tidb_txn_mode', 'tidb_enable_async_commit','tidb_enable_1pc', 'tidb_mem_oom_action', 'tidb_enable_auto_analyze', 'tidb_row_format_version', 'tidb_txn_assertion_level', 'tidb_enable_mutation_checker', 'tidb_pessimistic_txn_fair_locking') order by VARIABLE_NAME