Skip to content

Commit

Permalink
sql: fix accounting for entries in Txn Fingerprint ID cache
Browse files Browse the repository at this point in the history
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
  • Loading branch information
yuzefovich committed Apr 8, 2024
1 parent bf54a27 commit f70a1c9
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 5 deletions.
36 changes: 36 additions & 0 deletions pkg/sql/testdata/txn_fingerprint_id_cache
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,30 @@ show
----
[8 7 6 5]

# Each cache entry takes up 56 bytes.
accounting
----
224 bytes

# Attempt to add ids that are already present in the cache. Neither the cache
# nor the accounting should change.

enqueue id=05
----
size: 4

enqueue id=07
----
size: 4

show
----
[8 7 6 5]

accounting
----
224 bytes

enqueue id=09
----
size: 5
Expand All @@ -37,6 +61,10 @@ show
----
[10 9 8 7 6 5]

accounting
----
336 bytes

# Decrease the TxnFingerprintIDCacheCapacity cluster setting to below current size.
override capacity=3
----
Expand All @@ -52,6 +80,10 @@ show
----
[11 10 9]

accounting
----
168 bytes

# Check that retrieving IDs also properly truncates the cache when the capacity has
# been changed.
# Increase capacity back up to 5, insert some values, then decrease capacity to 2 and
Expand All @@ -75,3 +107,7 @@ TxnFingerprintIDCacheCapacity: 2
show
----
[13 12]

accounting
----
112 bytes
4 changes: 4 additions & 0 deletions pkg/sql/txn_fingerprint_id_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ func (b *TxnFingerprintIDCache) Add(
) error {
b.mu.Lock()
defer b.mu.Unlock()
if _, ok := b.mu.cache.StealthyGet(id); ok {
// The value is already in the cache - do nothing.
return nil
}
if err := b.mu.acc.Grow(ctx, cacheEntrySize+txnFingerprintIDSize); err != nil {
return err
}
Expand Down
16 changes: 11 additions & 5 deletions pkg/sql/txn_fingerprint_id_cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/server/serverpb"
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
"github.com/cockroachdb/cockroach/pkg/sql/appstatspb"
"github.com/cockroachdb/cockroach/pkg/sql/execinfra"
"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
"github.com/cockroachdb/cockroach/pkg/testutils/datapathutils"
"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
Expand All @@ -34,18 +35,21 @@ import (

func TestTxnFingerprintIDCacheDataDriven(t *testing.T) {
defer leaktest.AfterTest(t)()

ctx := context.Background()
st := cluster.MakeTestingClusterSettings()
memMonitor := execinfra.NewTestMemMonitor(ctx, st)
memAccount := memMonitor.MakeBoundAccount()
var txnFingerprintIDCache *TxnFingerprintIDCache

datadriven.Walk(t, datapathutils.TestDataPath(t, "txn_fingerprint_id_cache"), func(t *testing.T, path string) {
datadriven.RunTest(t, path, func(t *testing.T, d *datadriven.TestData) string {
ctx := context.Background()
switch d.Cmd {
case "init":
var capacity int
d.ScanArgs(t, "capacity", &capacity)

st := cluster.MakeTestingClusterSettings()
txnFingerprintIDCache = NewTxnFingerprintIDCache(ctx, st, nil /* acc */)
txnFingerprintIDCache = NewTxnFingerprintIDCache(ctx, st, &memAccount)

TxnFingerprintIDCacheCapacity.Override(ctx, &st.SV, int64(capacity))

Expand Down Expand Up @@ -74,10 +78,12 @@ func TestTxnFingerprintIDCacheDataDriven(t *testing.T) {
case "show":
return printTxnFingerprintIDCache(txnFingerprintIDCache)

case "accounting":
return fmt.Sprintf("%d bytes", memAccount.Used())

default:
return ""
}
return ""

})
})
}
Expand Down

0 comments on commit f70a1c9

Please sign in to comment.