session, variable: move gc special sysvars to getters/setters #24896
Conversation
ti-chi-bot
added
the
size/XL
| c.Assert(val, Equals, strconv.FormatUint(uint64(atomic.LoadUint32(&config.GetGlobalConfig().Log.RecordPlanInSlowLog)), 10)) | ||
| enabled := atomic.LoadUint32(&config.GetGlobalConfig().Log.RecordPlanInSlowLog) == 1 | ||
| c.Assert(val, Equals, BoolToOnOff(enabled)) |
There was a problem hiding this comment.
This is a boolean typed value, but it was previously returning "1" instead of "ON/OFF". Because this code cahnge now calls validation on getters, it needed to be fixed.
| // SQLModeVar is the name of the 'sql_mode' system variable. | ||
| SQLModeVar = "sql_mode" | ||
| // CharacterSetResults is the name of the 'character_set_results' system variable. | ||
| CharacterSetResults = "character_set_results" | ||
| // MaxAllowedPacket is the name of the 'max_allowed_packet' system variable. | ||
| MaxAllowedPacket = "max_allowed_packet" | ||
| // TimeZone is the name of the 'time_zone' system variable. | ||
| TimeZone = "time_zone" | ||
| // TxnIsolation is the name of the 'tx_isolation' system variable. | ||
| TxnIsolation = "tx_isolation" | ||
| // TransactionIsolation is the name of the 'transaction_isolation' system variable. | ||
| TransactionIsolation = "transaction_isolation" | ||
| // TxnIsolationOneShot is the name of the 'tx_isolation_one_shot' system variable. | ||
| TxnIsolationOneShot = "tx_isolation_one_shot" | ||
| // MaxExecutionTime is the name of the 'max_execution_time' system variable. | ||
| MaxExecutionTime = "max_execution_time" | ||
| // ReadOnly is the name of the 'read_only' system variable. | ||
| ReadOnly = "read_only" |
There was a problem hiding this comment.
These are moved from sessionctx/variable/session.go. They are technically not related to this PR, but its a small change.
| if name == variable.TiDBSlowLogMasking { | ||
| name = variable.TiDBRedactLog | ||
| } |
There was a problem hiding this comment.
This is now handled by a getter on TiDBSlowLogMasking.
| val, err := sv.GetGlobal(s) | ||
| if err != nil { | ||
| return val, err | ||
| } | ||
| // Ensure that the results from the getter are validated | ||
| // Since some are read directly from tables. | ||
| return sv.ValidateWithRelaxedValidation(s, val, ScopeGlobal), nil |
There was a problem hiding this comment.
This is required because in many case getters are reading from table values and accept user input directly without normalization. ValidateWithRelaxedValidation is the same function that is called when init'ing a new session and assigning values to the session variables.
| enabled := atomic.LoadUint32(&config.GetGlobalConfig().Log.RecordPlanInSlowLog) == 1 | ||
| return BoolToOnOff(enabled), nil |
There was a problem hiding this comment.
This is technically a bug: a bool type should return the string value "ON/OFF". It is fixed here because the validation added converts it to ON/OFF anyway, which broke a test for this value.
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
the
needs-rebase
ti-chi-bot
removed
the
needs-rebase
ti-chi-bot
added
status/LGT2
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: dd5c8a6
|
ti-chi-bot
added
the
status/can-merge
|
@morgo: Your PR was out of date, I have automatically updated it for you. At the same time I will also trigger all tests for you: /run-all-tests If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes. 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 ti-community-infra/tichi repository. |
What problem does this PR solve?
Problem Summary:
Currently setting GC sysvars is kind of invasive in the session package. This is because they required to be available as sysvars so that the mysql.tidb table could be hidden for SEM, but there was not yet a standard way in the sysvar system to write code on get/set special values.
This moves the special code directly into getters and setters. The functionality is the same, and covered by existing tests.
What is changed and how it works?
What's Changed:
Internal refactoring of code.
Related changes
Check List
Tests
Side effects
Release note