Skip to content
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

Merged
merged 30 commits into from
Mar 15, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
3f152ae
bindinfo: add warning message when the memory usage of the binding_ca…
Reminiscent Mar 7, 2022
0062a52
Merge branch 'master' of github.com:pingcap/tidb into addWarning4bind…
Reminiscent Mar 7, 2022
7b37080
add more test cases
Reminiscent Mar 7, 2022
a40f644
remove useless line
Reminiscent Mar 7, 2022
10bf341
fix ut
Reminiscent Mar 7, 2022
03f6b3a
Merge branch 'master' of github.com:pingcap/tidb into addWarning4bind…
Reminiscent Mar 7, 2022
538182c
Merge branch 'master' of github.com:pingcap/tidb into addWarning4bind…
Reminiscent Mar 7, 2022
fa9ef08
rename TiDBMemQuotaBindCache to TiDBMemQuotaBindingCache
Reminiscent Mar 7, 2022
0dfbe1a
fix ut
Reminiscent Mar 7, 2022
3a1ea99
fix ut
Reminiscent Mar 8, 2022
8a09095
Merge branch 'master' of github.com:pingcap/tidb into addWarning4bind…
Reminiscent Mar 8, 2022
1da4ed2
address comments
Reminiscent Mar 10, 2022
ae595fb
Merge branch 'master' of github.com:pingcap/tidb into addWarning4bind…
Reminiscent Mar 10, 2022
7b1a3f7
fix ut
Reminiscent Mar 10, 2022
7fd22f7
fix ut
Reminiscent Mar 10, 2022
1094fd3
Merge branch 'master' of github.com:pingcap/tidb into addWarning4bind…
Reminiscent Mar 10, 2022
985f3af
address comments
Reminiscent Mar 10, 2022
afe84a7
address comments
Reminiscent Mar 14, 2022
77598b4
Merge branch 'master' of github.com:pingcap/tidb into addWarning4bind…
Reminiscent Mar 14, 2022
3691299
address comments
Reminiscent Mar 15, 2022
5525271
Merge branch 'master' of github.com:pingcap/tidb into addWarning4bind…
Reminiscent Mar 15, 2022
6ddf3b2
Merge branch 'master' into addWarning4bindCache
ti-chi-bot Mar 15, 2022
c08a031
fix ut
Reminiscent Mar 15, 2022
3ecd695
Merge branch 'master' of github.com:pingcap/tidb into addWarning4bind…
Reminiscent Mar 15, 2022
9cdbb67
Merge remote-tracking branch 'origin/addWarning4bindCache' into addWa…
Reminiscent Mar 15, 2022
43db335
Merge branch 'master' into addWarning4bindCache
ti-chi-bot Mar 15, 2022
ee3f7ef
Merge branch 'master' into addWarning4bindCache
ti-chi-bot Mar 15, 2022
8ef10da
Merge branch 'master' of github.com:pingcap/tidb into addWarning4bind…
Reminiscent Mar 15, 2022
8e209b5
fix ut
Reminiscent Mar 15, 2022
4425015
Merge remote-tracking branch 'origin/addWarning4bindCache' into addWa…
Reminiscent Mar 15, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 35 additions & 8 deletions bindinfo/bind_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,16 @@
package bindinfo

import (
"errors"
"sync"

"github.com/cznic/mathutil"
"github.com/pingcap/tidb/sessionctx/variable"
"github.com/pingcap/tidb/util/hack"
"github.com/pingcap/tidb/util/kvcache"
"github.com/pingcap/tidb/util/logutil"
"github.com/pingcap/tidb/util/memory"
"go.uber.org/zap"
)

// bindCache uses the LRU cache to store the bindRecord.
Expand Down Expand Up @@ -75,26 +78,29 @@ 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 is failed when the memory usage of binding_cache exceeds its capacity.
func (c *bindCache) set(key bindCacheKey, value []*BindRecord) (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")
Copy link
Member

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

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")
Copy link
Member

Choose a reason for hiding this comment

The 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
return
}

// delete remove an item from the cache. It's not thread-safe.
Expand All @@ -105,8 +111,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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
Expand Down Expand Up @@ -150,7 +157,10 @@ 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 {
logutil.BgLogger().Warn("[sql-bind] ", zap.Error(err))
}
}

// RemoveBindRecord removes the BindRecord which has same originSQL with specified BindRecord.
Expand All @@ -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.
Expand All @@ -195,19 +207,34 @@ func (c *bindCache) GetMemCapacity() int64 {
return c.memCapacity
}

// GetMemUsage get the memory Usage for the cache.
// The function is thread-safe.
func (c *bindCache) GetMemUsage() int64 {
c.lock.Lock()
defer c.lock.Unlock()
return c.memTracker.BytesConsumed()
}

// Copy copies a new bindCache from the origin cache.
// The function is thread-safe.
func (c *bindCache) Copy() *bindCache {
c.lock.Lock()
defer c.lock.Unlock()
newCache := newBindCache()
if c.GetMemUsage() > newCache.GetMemCapacity() {
logutil.BgLogger().Warn("[sql-bind] During the cache copy operation, " +
"the memory capacity of the new cache is less than the memory consumption of the old cache, " +
"so there are bindings that are not loaded into the cache")
}
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
}
19 changes: 10 additions & 9 deletions bindinfo/bind_cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import (
)

func TestBindCache(t *testing.T) {
variable.MemQuotaBindCache.Store(100)
variable.MemQuotaBindCache.Store(200)
bindCache := newBindCache()

value := make([][]*BindRecord, 3)
Expand All @@ -39,25 +39,26 @@ func TestBindCache(t *testing.T) {
require.Equal(t, int64(100), calcBindCacheKVMem(key[i], value[i]))
}

ok := bindCache.set(key[0], value[0])
require.True(t, ok)
err := bindCache.set(key[0], value[0])
require.Nil(t, err)
result := bindCache.get(key[0])
require.NotNil(t, result)

ok = bindCache.set(key[1], value[1])
require.True(t, ok)
err = bindCache.set(key[1], value[1])
require.Nil(t, err)
result = bindCache.get(key[1])
require.NotNil(t, result)

ok = bindCache.set(key[2], value[2])
require.True(t, ok)
err = bindCache.set(key[2], value[2])
require.NotNil(t, err)
result = bindCache.get(key[2])
require.NotNil(t, result)

// Both key[0] and key[1] are not in the cache
// key[0] is not in the cache
result = bindCache.get(key[0])
require.Nil(t, result)

// key[1] is still in the cache
result = bindCache.get(key[1])
require.Nil(t, result)
require.NotNil(t, result)
}