-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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.
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
sessionctx/variable: refine TiDB specific system variables. #2915
Conversation
Added index lookup size and index lookup concurrency global variable. Organized TiDB specific system variables and added documentation.
CI failed. @coocood |
for total > 0 { | ||
if batchSize > total { | ||
batchSize = total | ||
} | ||
taskSizes = append(taskSizes, batchSize) | ||
total -= batchSize | ||
if batchSize < MaxLookupTableTaskSize { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why change the old strategy ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old strategy doesn't work well.
LGTM |
@@ -254,7 +254,7 @@ func upgrade(s Session) { | |||
func upgradeToVer2(s Session) { | |||
// Version 2 add two system variable for DistSQL concurrency controlling. | |||
// Insert distsql related system variable. | |||
distSQLVars := []string{variable.DistSQLScanConcurrencyVar, variable.DistSQLJoinConcurrencyVar} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove DistSQLJoinConcurrencyVar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not used right now, we can easily add it later if needed.
We add it before hand because we used need to update bootstrap version for adding new global variable.
{ScopeGlobal | ScopeSession, TiDBDistSQLScanConcurrency, strconv.Itoa(DefDistSQLScanConcurrency)}, | ||
{ScopeGlobal | ScopeSession, TiDBIndexLookupSize, strconv.Itoa(DefIndexLookupSize)}, | ||
{ScopeGlobal | ScopeSession, TiDBIndexLookupConcurrency, strconv.Itoa(DefIndexLookupConcurrency)}, | ||
{ScopeGlobal | ScopeSession, TiDBSkipDDLWait, boolToIntStr(DefSkipDDLWait)}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TiDBSkipDDLWait should be session scope only. It is not safe to make it global scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Third-party benchmark and loading tools create a lot of tables in a single TiDB server, we can't set session variables for them.
Make it a global variable is the only way.
We have well documented the risk to use this variable.
I think users can have the option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We should have a document for tidb specific sysvars.
Added index lookup size and index lookup concurrency global variable.
Organized TiDB specific system variables and added documentation.