-
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
bindinfo: add warning message when the memory usage of the cache exceeds its capacity #32866
Changes from 16 commits
3f152ae
0062a52
7b37080
a40f644
10bf341
03f6b3a
538182c
fa9ef08
0dfbe1a
3a1ea99
8a09095
1da4ed2
ae595fb
7b1a3f7
7fd22f7
1094fd3
985f3af
afe84a7
77598b4
3691299
5525271
6ddf3b2
c08a031
3ecd695
9cdbb67
43db335
ee3f7ef
8ef10da
8e209b5
4425015
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
package bindinfo | ||
|
||
import ( | ||
"errors" | ||
"sync" | ||
|
||
"github.com/cznic/mathutil" | ||
|
@@ -54,7 +55,7 @@ func newBindCache() *bindCache { | |
cache := kvcache.NewSimpleLRUCache(mathutil.MaxUint, 0, 0) | ||
c := bindCache{ | ||
cache: cache, | ||
memCapacity: variable.MemQuotaBindCache.Load(), | ||
memCapacity: variable.MemQuotaBindingCache.Load(), | ||
memTracker: memory.NewTracker(memory.LabelForBindCache, -1), | ||
} | ||
return &c | ||
|
@@ -75,26 +76,30 @@ func (c *bindCache) get(key bindCacheKey) []*BindRecord { | |
|
||
// set inserts an item to the cache. It's not thread-safe. | ||
// Only other functions of the bindCache can use this function. | ||
func (c *bindCache) set(key bindCacheKey, value []*BindRecord) bool { | ||
// The set operation will return error message when the memory usage of binding_cache exceeds its capacity. | ||
func (c *bindCache) set(key bindCacheKey, value []*BindRecord) (ok bool, err error) { | ||
mem := calcBindCacheKVMem(key, value) | ||
if mem > c.memCapacity { // ignore this kv pair if its size is too large | ||
return false | ||
err = errors.New("The memory usage of the binding_cache exceeds its capacity") | ||
return | ||
} | ||
bindRecords := c.get(key) | ||
if bindRecords != nil { | ||
// Remove the origin key-value pair. | ||
mem -= calcBindCacheKVMem(key, bindRecords) | ||
} | ||
for mem+c.memTracker.BytesConsumed() > c.memCapacity { | ||
err = errors.New("The memory usage of the binding_cache exceeds its capacity") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto. If we delete those old cache entries, we won't exceed the capacity. |
||
evictedKey, evictedValue, evicted := c.cache.RemoveOldest() | ||
if !evicted { | ||
return false | ||
return | ||
} | ||
c.memTracker.Consume(-calcBindCacheKVMem(evictedKey.(bindCacheKey), evictedValue.([]*BindRecord))) | ||
} | ||
c.memTracker.Consume(mem) | ||
c.cache.Put(key, value) | ||
return true | ||
ok = true | ||
return | ||
} | ||
|
||
// delete remove an item from the cache. It's not thread-safe. | ||
|
@@ -105,8 +110,9 @@ func (c *bindCache) delete(key bindCacheKey) bool { | |
mem := calcBindCacheKVMem(key, bindRecords) | ||
c.cache.Delete(key) | ||
c.memTracker.Consume(-mem) | ||
return true | ||
} | ||
return true | ||
return false | ||
Comment on lines
+129
to
+131
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a bug here, But for now, the return value has not been used yet. |
||
} | ||
|
||
// GetBindRecord gets the BindRecord from the cache. | ||
|
@@ -140,7 +146,7 @@ func (c *bindCache) GetAllBindRecords() []*BindRecord { | |
|
||
// SetBindRecord sets the BindRecord to the cache. | ||
// The function is thread-safe. | ||
func (c *bindCache) SetBindRecord(hash string, meta *BindRecord) { | ||
func (c *bindCache) SetBindRecord(hash string, meta *BindRecord) (err error) { | ||
c.lock.Lock() | ||
defer c.lock.Unlock() | ||
cacheKey := bindCacheKey(hash) | ||
|
@@ -150,7 +156,11 @@ func (c *bindCache) SetBindRecord(hash string, meta *BindRecord) { | |
metas[i] = meta | ||
} | ||
} | ||
c.set(cacheKey, []*BindRecord{meta}) | ||
_, err = c.set(cacheKey, []*BindRecord{meta}) | ||
if err != nil { | ||
return | ||
} | ||
return | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems the if check is unnecessary. 🤣 |
||
} | ||
|
||
// RemoveBindRecord removes the BindRecord which has same originSQL with specified BindRecord. | ||
|
@@ -175,7 +185,9 @@ func (c *bindCache) RemoveBindRecord(hash string, meta *BindRecord) { | |
} | ||
} | ||
} | ||
c.set(bindCacheKey(hash), metas) | ||
// This function can guarantee the memory usage for the cache will never grow up. | ||
// So we don't need to handle the return value here. | ||
_, _ = c.set(bindCacheKey(hash), metas) | ||
} | ||
|
||
// SetMemCapacity sets the memory capacity for the cache. | ||
|
@@ -197,17 +209,22 @@ func (c *bindCache) GetMemCapacity() int64 { | |
|
||
// Copy copies a new bindCache from the origin cache. | ||
// The function is thread-safe. | ||
func (c *bindCache) Copy() *bindCache { | ||
func (c *bindCache) Copy() (newCache *bindCache, err error) { | ||
c.lock.Lock() | ||
defer c.lock.Unlock() | ||
newCache := newBindCache() | ||
newCache = newBindCache() | ||
if c.memTracker.BytesConsumed() > newCache.GetMemCapacity() { | ||
err = errors.New("The memory usage of the binding_cache exceeds its capacity") | ||
} | ||
keys := c.cache.Keys() | ||
for _, key := range keys { | ||
cacheKey := key.(bindCacheKey) | ||
v := c.get(cacheKey) | ||
bindRecords := make([]*BindRecord, len(v)) | ||
copy(bindRecords, v) | ||
newCache.set(cacheKey, bindRecords) | ||
// The memory usage of cache has been handled at the beginning of this function. | ||
// So we don't need to handle the return value here. | ||
_, _ = newCache.set(cacheKey, bindRecords) | ||
} | ||
return newCache | ||
return newCache, err | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -143,7 +143,7 @@ func (h *BindHandle) Update(fullLoad bool) (err error) { | |
return err | ||
} | ||
|
||
newCache := h.bindInfo.Value.Load().(*bindCache).Copy() | ||
newCache, memExceededErr := h.bindInfo.Value.Load().(*bindCache).Copy() | ||
defer func() { | ||
h.bindInfo.lastUpdateTime = lastUpdateTime | ||
h.bindInfo.Value.Store(newCache) | ||
|
@@ -171,12 +171,21 @@ func (h *BindHandle) Update(fullLoad bool) (err error) { | |
oldRecord := newCache.GetBindRecord(hash, meta.OriginalSQL, meta.Db) | ||
newRecord := merge(oldRecord, meta).removeDeletedBindings() | ||
if len(newRecord.Bindings) > 0 { | ||
newCache.SetBindRecord(hash, newRecord) | ||
err = newCache.SetBindRecord(hash, newRecord) | ||
if err != nil && memExceededErr != nil { | ||
memExceededErr = err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we break this loop if meet this error? |
||
} | ||
} else { | ||
newCache.RemoveBindRecord(hash, newRecord) | ||
} | ||
updateMetrics(metrics.ScopeGlobal, oldRecord, newCache.GetBindRecord(hash, meta.OriginalSQL, meta.Db), true) | ||
} | ||
if memExceededErr != nil { | ||
// When the memory capacity of bing_cache is not enough, | ||
// there will be some memory-related errors in multiple places. | ||
// Only needs to be handled once. | ||
logutil.BgLogger().Warn("[sql-bind] ", zap.Error(err)) | ||
} | ||
return nil | ||
} | ||
|
||
|
@@ -570,27 +579,43 @@ func (h *BindHandle) newBindRecord(row chunk.Row) (string, *BindRecord, error) { | |
// setBindRecord sets the BindRecord to the cache, if there already exists a BindRecord, | ||
// it will be overridden. | ||
func (h *BindHandle) setBindRecord(hash string, meta *BindRecord) { | ||
newCache := h.bindInfo.Value.Load().(*bindCache).Copy() | ||
newCache, err0 := h.bindInfo.Value.Load().(*bindCache).Copy() | ||
if err0 != nil { | ||
logutil.BgLogger().Warn("[sql-bind] ", zap.Error(err0)) | ||
} | ||
oldRecord := newCache.GetBindRecord(hash, meta.OriginalSQL, meta.Db) | ||
newCache.SetBindRecord(hash, meta) | ||
err1 := newCache.SetBindRecord(hash, meta) | ||
if err1 != nil && err0 == nil { | ||
logutil.BgLogger().Warn("[sql-bind] ", zap.Error(err1)) | ||
} | ||
h.bindInfo.Value.Store(newCache) | ||
updateMetrics(metrics.ScopeGlobal, oldRecord, meta, false) | ||
} | ||
|
||
// appendBindRecord addes the BindRecord to the cache, all the stale BindRecords are | ||
// removed from the cache after this operation. | ||
func (h *BindHandle) appendBindRecord(hash string, meta *BindRecord) { | ||
newCache := h.bindInfo.Value.Load().(*bindCache).Copy() | ||
newCache, err0 := h.bindInfo.Value.Load().(*bindCache).Copy() | ||
if err0 != nil { | ||
logutil.BgLogger().Warn("[sql-bind] ", zap.Error(err0)) | ||
} | ||
oldRecord := newCache.GetBindRecord(hash, meta.OriginalSQL, meta.Db) | ||
newRecord := merge(oldRecord, meta) | ||
newCache.SetBindRecord(hash, newRecord) | ||
err1 := newCache.SetBindRecord(hash, newRecord) | ||
if err1 != nil && err0 == nil { | ||
// Only need to handle the error once. | ||
logutil.BgLogger().Warn("[sql-bind] ", zap.Error(err1)) | ||
} | ||
h.bindInfo.Value.Store(newCache) | ||
updateMetrics(metrics.ScopeGlobal, oldRecord, newRecord, false) | ||
} | ||
|
||
// removeBindRecord removes the BindRecord from the cache. | ||
func (h *BindHandle) removeBindRecord(hash string, meta *BindRecord) { | ||
newCache := h.bindInfo.Value.Load().(*bindCache).Copy() | ||
newCache, err := h.bindInfo.Value.Load().(*bindCache).Copy() | ||
if err != nil { | ||
logutil.BgLogger().Warn("[sql-bind] ", zap.Error(err)) | ||
} | ||
oldRecord := newCache.GetBindRecord(hash, meta.OriginalSQL, meta.Db) | ||
newCache.RemoveBindRecord(hash, meta) | ||
h.bindInfo.Value.Store(newCache) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,8 @@ import ( | |
"github.com/pingcap/tidb/parser/mysql" | ||
"github.com/pingcap/tidb/sessionctx" | ||
"github.com/pingcap/tidb/types" | ||
"github.com/pingcap/tidb/util/logutil" | ||
"go.uber.org/zap" | ||
) | ||
|
||
// SessionHandle is used to handle all session sql bind operations. | ||
|
@@ -42,7 +44,10 @@ func NewSessionBindHandle(parser *parser.Parser) *SessionHandle { | |
// removed from the cache after this operation. | ||
func (h *SessionHandle) appendBindRecord(hash string, meta *BindRecord) { | ||
oldRecord := h.ch.GetBindRecord(hash, meta.OriginalSQL, meta.Db) | ||
h.ch.SetBindRecord(hash, meta) | ||
err := h.ch.SetBindRecord(hash, meta) | ||
if err != nil { | ||
logutil.BgLogger().Warn("[sql-bind] ", zap.Error(err)) | ||
} | ||
updateMetrics(metrics.ScopeSession, oldRecord, meta, false) | ||
} | ||
|
||
|
@@ -80,7 +85,10 @@ func (h *SessionHandle) DropBindRecord(originalSQL, db string, binding *Binding) | |
} else { | ||
newRecord = record | ||
} | ||
h.ch.SetBindRecord(hash, newRecord) | ||
err := h.ch.SetBindRecord(hash, newRecord) | ||
if err != nil { | ||
logutil.BgLogger().Warn("[sql-bind] ", zap.Error(err)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this case possibly happen? |
||
} | ||
updateMetrics(metrics.ScopeSession, oldRecord, newRecord, false) | ||
return nil | ||
} | ||
|
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.
I think it's a bit strange here. Actually we haven't exceeded the capacity yet here, because we didn't put this binding into the cache