sql: add accounting for entries in Txn Fingerprint ID cache #121956
Conversation
This reverts commit 780ddf4.
This commit fixes a bug that could previously result in the memory accounting leak that was exposed by 88ebd70. Namely, the problem is that we previously unconditionally grew the memory account in `Add` even though if the ID is already present in the cache, it wouldn't be inserted again. As a result, we'd only shrink the account once but might have grown it any number of times for a particular ID. Now we check whether the ID is present in the cache and only grow the account if not. Epic: None Release note: None
|
This change is |
yuzefovich
changed the title
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/sql/txn_fingerprint_id_cache.go line 89 at r2 (raw file):
// The value is already in the cache - do nothing. return nil }
BTW, there's also a Len() method we could use to update memory accounting without having to do anything in OnEvictedEntry. I'm fine with either method, though.
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/sql/txn_fingerprint_id_cache.go line 89 at r2 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
BTW, there's also a
Len()method we could use to update memory accounting without having to do anything inOnEvictedEntry. I'm fine with either method, though.
Hm, I think we'd still need to set OnEvictedEntry in case the capacity of the cache is reduced. In other words, we'd use BoundAccount.ResizeTo in both Add and OnEvictedEntry which will be effectively the same as the current approach.
Right, good point. |
This commit fixes a bug that could previously result in the memory accounting leak that was exposed by 88ebd70. Namely, the problem is that we previously unconditionally grew the memory account in
Addeven though if the ID is already present in the cache, it wouldn't be inserted again. As a result, we'd only shrink the account once but might have grown it any number of times for a particular ID. Now we check whether the ID is present in the cache and only grow the account if not.Epic: None
Release note: None