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

cacheutil: fix computation of entry size #96892

Merged
merged 1 commit into from
Feb 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 8 additions & 6 deletions pkg/sql/cacheutil/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ package cacheutil

import (
"context"
"unsafe"

"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/util/log"
Expand Down Expand Up @@ -79,12 +78,16 @@ func (c *Cache) LoadValueOutsideOfCacheSingleFlight(
// MaybeWriteBackToCache tries to put the key, value into the
// cache, and returns true if it succeeded. If the underlying system
// tables have been modified since they were read, the cache is not
// updated.
// updated. The entrySize should be the size in bytes of the key and value.
// Note that reading from system tables may give us data from a newer table
// version than the one we pass in here, that is okay since the cache will
// be invalidated upon the next read.
func (c *Cache) MaybeWriteBackToCache(
ctx context.Context, tableVersions []descpb.DescriptorVersion, key interface{}, entry interface{},
ctx context.Context,
tableVersions []descpb.DescriptorVersion,
key interface{},
value interface{},
entrySize int64,
) bool {
c.Lock()
defer c.Unlock()
Expand All @@ -98,14 +101,13 @@ func (c *Cache) MaybeWriteBackToCache(
}
}
// Table version remains the same: update map, unlock, return.
const sizeOfEntry = int(unsafe.Sizeof(entry))
if err := c.boundAccount.Grow(ctx, int64(sizeOfEntry)); err != nil {
if err := c.boundAccount.Grow(ctx, entrySize); err != nil {
// If there is no memory available to cache the entry, we can still
// proceed with authentication so that users are not locked out of
// the database.
log.Ops.Warningf(ctx, "no memory available to cache info: %v", err)
} else {
c.cache[key] = entry
c.cache[key] = value
}
return true
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/sql/cacheutil/cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,13 @@ func TestCache(t *testing.T) {
}()
}

wrote := cache.MaybeWriteBackToCache(ctx, []descpb.DescriptorVersion{2, 2}, "test", "val")
wrote := cache.MaybeWriteBackToCache(ctx, []descpb.DescriptorVersion{2, 2}, "test", "val", int64(len("test")+len("val")))
require.Equal(t, wrote, true)

wrote = cache.MaybeWriteBackToCache(ctx, []descpb.DescriptorVersion{0, 2}, "test", "val")
wrote = cache.MaybeWriteBackToCache(ctx, []descpb.DescriptorVersion{0, 2}, "test", "val", int64(len("test")+len("val")))
require.Equal(t, wrote, false)

wrote = cache.MaybeWriteBackToCache(ctx, []descpb.DescriptorVersion{2, 0}, "test", "val")
wrote = cache.MaybeWriteBackToCache(ctx, []descpb.DescriptorVersion{2, 0}, "test", "val", int64(len("test")+len("val")))
require.Equal(t, wrote, false)

val, ok := cache.GetValueLocked("test")
Expand Down
22 changes: 18 additions & 4 deletions pkg/sql/syntheticprivilegecache/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ package syntheticprivilegecache
import (
"context"
"fmt"
"unsafe"

"github.com/cockroachdb/cockroach/pkg/clusterversion"
"github.com/cockroachdb/cockroach/pkg/kv"
Expand Down Expand Up @@ -96,9 +97,9 @@ func (c *Cache) Get(
return nil, err
}
privDesc := val.(*catpb.PrivilegeDescriptor)
// Only write back to the cache if the table version is
// committed.
c.c.MaybeWriteBackToCache(ctx, []descpb.DescriptorVersion{desc.GetVersion()}, spo.GetPath(), *privDesc)
entrySize := int64(len(spo.GetPath())) + computePrivDescSize(privDesc)
// Only write back to the cache if the table version is committed.
c.c.MaybeWriteBackToCache(ctx, []descpb.DescriptorVersion{desc.GetVersion()}, spo.GetPath(), *privDesc, entrySize)
return privDesc, nil
}

Expand Down Expand Up @@ -278,7 +279,8 @@ func (c *Cache) start(ctx context.Context) error {
if accum, ok := vtablePathToPrivilegeAccumulator[vtablePriv.GetPath()]; ok {
privDesc = accum.finish()
}
c.c.MaybeWriteBackToCache(ctx, tableVersions, vtablePriv.GetPath(), *privDesc)
entrySize := int64(len(vtablePriv.GetPath())) + computePrivDescSize(privDesc)
c.c.MaybeWriteBackToCache(ctx, tableVersions, vtablePriv.GetPath(), *privDesc, entrySize)
})
}
return nil
Expand All @@ -292,3 +294,15 @@ func (c *Cache) waitForWarmed(ctx context.Context) error {
return ctx.Err()
}
}

// computePrivDescSize computes the size in bytes required by the data in this
// descriptor.
func computePrivDescSize(privDesc *catpb.PrivilegeDescriptor) int64 {
privDescSize := int(unsafe.Sizeof(*privDesc))
privDescSize += len(privDesc.OwnerProto)
for _, u := range privDesc.Users {
privDescSize += int(unsafe.Sizeof(u))
privDescSize += len(u.UserProto)
}
return int64(privDescSize)
}