variable: fix information_schema.VARIABLES_INFO DEFAULT_VALUE not right problem #49524
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-triage-completed
release-note-none
|
Hi @jiyfhust. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest |
|
@jiyfhust: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #49524 +/- ##
================================================
- Coverage 71.8744% 67.1941% -4.6804%
================================================
Files 1437 2574 +1137
Lines 345629 859614 +513985
================================================
+ Hits 248419 577610 +329191
- Misses 76977 258076 +181099
- Partials 20233 23928 +3695
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
/cc @dveeden |
|
/ok-to-test |
|
/check-issue-triage-complete |
ti-chi-bot
bot
added
needs-cherry-pick-release-6.5
ti-chi-bot
bot
added
size/L
| } | ||
| case TiDBEnableAsyncCommit, TiDBEnable1PC: | ||
| if config.GetGlobalConfig().Store == "tikv" { | ||
| varVal = On |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
- if we need set diffrent defalut value for diffrent store types for example
TiDBEnableAsyncCommit，we need do that afterconfig.GetGlobalConfig()loaded - others，we can move into
defaultSysVars, for exampleTiDBRowFormatVersion.
/cc @JmPotato
There was a problem hiding this comment.
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:
- 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
TiDBEnableAsyncCommitconfigured statically without reading global config. - 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.
[LGTM Timeline notifier]Timeline:
|
|
Also could close #50155, please add it on |
Added. |
ti-chi-bot
bot
added
needs-cherry-pick-release-5.4
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bb7133, easonn7, hawkingrei, lcwangchao The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
In response to a cherrypick label: new pull request created to branch |
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
In response to a cherrypick label: new pull request created to branch |
ti-chi-bot
removed
the
needs-cherry-pick-release-7.1
What problem does this PR solve?
Issue Number: close #49461, close #49968, close #50155
Problem Summary:
What changed and how does it work?
Check List
Tests
Without this pr, deploy a cluster:
With this pr:
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.