*: move config file options prepared-plan-cache.* to sysvars #34752
Conversation
|
[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
release-note
qw4990
changed the title
ti-chi-bot
removed
the
do-not-merge/work-in-progress
|
Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/4a2a2dbf9b34f88a25c0a2c0faeb29888155c110 |
|
Looking good. Please run |
Updated, PTAL |
|
Hi @qw4990 , please consider introducing an upgrade function to make sure that the 'old' configuration values can be inherited when users upgrade the their clusters to v6.1, just like: |
bb7133
added
the
compatibility-breaker
tidb-server/main.go
Outdated
| variable.EnablePreparedPlanCache.Store(config.CheckTableBeforeDrop) | ||
| // use server-memory-quota as max-plan-cache-memory | ||
| variable.PreparedPlanCacheMaxMemory.Store(cfg.Performance.ServerMemoryQuota) | ||
| total, err := memory.MemTotal() | ||
| terror.MustNil(err) | ||
| // if server-memory-quota is larger than max-system-memory or not set, use max-system-memory as max-plan-cache-memory | ||
| if variable.PreparedPlanCacheMaxMemory.Load() > total || variable.PreparedPlanCacheMaxMemory.Load() <= 0 { | ||
| variable.PreparedPlanCacheMaxMemory.Store(total) |
There was a problem hiding this comment.
It's best to remove this code. The reason being that these values will get overwritten by the SetGlobal funcs anyway when the sysvar cache is populated. So if there are behaviors that are required here they should be in SetGlobal.
The SetGlobal func is also called periodically when the cache is reloaded.
There was a problem hiding this comment.
Got it, but actually PreparedPlanCacheMaxMemory is not a system variable, it's just a pure Golang variable to store the max memory usage of the plan cache and needs to be initialized when starting. I've moved it from variable package back to planner/core package.
sessionctx/variable/sysvar.go
Outdated
| {Scope: ScopeGlobal, Name: TiDBEnablePrepPlanCache, Value: BoolToOnOff(DefTiDBEnablePrepPlanCache), Type: TypeBool, SetGlobal: func(s *SessionVars, val string) error { | ||
| EnablePreparedPlanCache.Store(TiDBOptOn(val)) | ||
| return nil | ||
| }, GetGlobal: func(s *SessionVars) (string, error) { | ||
| return BoolToOnOff(EnablePreparedPlanCache.Load()), nil | ||
| }}, | ||
| {Scope: ScopeGlobal, Name: TiDBPrepPlanCacheSize, Value: strconv.FormatUint(uint64(DefTiDBPrepPlanCacheSize), 10), Type: TypeUnsigned, MinValue: 1, MaxValue: 100000, SetGlobal: func(s *SessionVars, val string) error { | ||
| uVal, err := strconv.ParseUint(val, 10, 64) | ||
| if err == nil { | ||
| PreparedPlanCacheSize.Store(uVal) | ||
| } | ||
| return err | ||
| }, GetGlobal: func(s *SessionVars) (string, error) { | ||
| return strconv.FormatUint(PreparedPlanCacheSize.Load(), 10), nil | ||
| }}, |
There was a problem hiding this comment.
Is it possible to combine these two sysvars so that CacheSize > 0 means enabled?
planner/core/cache.go
Outdated
| } else { | ||
| atomic.StoreInt32(&preparedPlanCacheEnabledValue, preparedPlanCacheUnable) | ||
| } | ||
| variable.EnablePreparedPlanCache.Store(isEnabled) |
There was a problem hiding this comment.
Be aware that this is not permanent. When the sysvar cache is refreshed, it will call the SetGlobal func with whatever is the value of the setting in the mysql.global_variables table.
So if this is used by tests, it is better to use tk.MustExec("SET sysvar = x").
There was a problem hiding this comment.
Got it. Actually, I've tried to use tk.MustExec("set ...") in tests, but it'll cause lots of code changes. So for safety and to make it easier to review, I finally use this way and plan to refactor all plan cache tests to use tk.MustExec(set session var...) in the next Sprint after we implement session-level variables for plan-cache.
Updated, PTAL |
ti-chi-bot
added
size/XS
What problem does this PR solve?
Issue Number: ref #33769, close #30168
Previous closing PR: #33836
Problem Summary:
The options prepared-plan-cache.* have historically been config options. But based on requirements from cloud & PM they should instead be sysvars.
What is changed and how it works?
Remove them from the config list and add them to global sysvars.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.