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

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Feb 9, 2023

fixes #89444

The previous way would always result in a fixed size for each entry, since it would just compute the size of a pointer. Now, the size of the entry must be specified when writing to the cache.

Release note: None

@rafiss rafiss requested review from ZhouXing19 and a team February 9, 2023 18:25
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@ZhouXing19 ZhouXing19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but just to make sure I understood it

  1. The bound account need to increase the size by the sum of the key's and value's size
  2. For a descriptor, its size should include those for the users.
    Is it correct?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)


pkg/sql/cacheutil/cache.go line 85 at r1 (raw file):

// 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(

Maybe add a comment about how the entrySize should be calculated?


pkg/sql/syntheticprivilegecache/cache.go line 298 at r1 (raw file):

}

func computePrivDescSize(privDesc *catpb.PrivilegeDescriptor) int64 {

Could we add a comment for this as well?

The previous way would always result in a fixed size for each entry,
since it would just compute the size of a pointer. Now, the size of the
entry must be specified when writing to the cache.

Release note: None
Copy link
Collaborator Author

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The bound account need to increase the size by the sum of the key's and value's size
  2. For a descriptor, its size should include those for the users.

Is it correct?

that's right. check out this example: https://go.dev/play/p/liBjF3Jetk5?v=goprev
it shows that unsafe.Sizeof only shows the length of the string header, and the actual bytes of the string need to be counted separately.

bors r=ZhouXing19

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ZhouXing19)


pkg/sql/cacheutil/cache.go line 85 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

Maybe add a comment about how the entrySize should be calculated?

done


pkg/sql/syntheticprivilegecache/cache.go line 298 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

Could we add a comment for this as well?

done

@craig
Copy link
Contributor

craig bot commented Feb 10, 2023

Build succeeded:

@craig craig bot merged commit 5f310bc into cockroachdb:master Feb 10, 2023
@rafiss rafiss deleted the fix-cache-entry-size branch February 13, 2023 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cacheutil: caching size estimate is not valid
3 participants