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

utilccl: improve CheckEnterpriseEnabled performance #62489

Closed
erikgrinaker opened this issue Mar 23, 2021 · 3 comments · Fixed by #62498
Closed

utilccl: improve CheckEnterpriseEnabled performance #62489

erikgrinaker opened this issue Mar 23, 2021 · 3 comments · Fixed by #62498
Assignees
Labels
C-performance Perf of queries or internals. Solution not expected to change functional behavior.

Comments

@erikgrinaker
Copy link
Contributor

erikgrinaker commented Mar 23, 2021

In #59571, utilccl.CheckEnterpriseEnabled was called much more often in the read path, via kvfollowerreadsccl.canSendToFollower. However, CheckEnterpriseEnabled has a couple of performance issues:

  1. We're instantiating a pgerror.Newf() in utilccl.check() when enterprise features are unavailable -- this is often necessary because we include cluster-specific info in the error message, but is expensive. However, in this specific case the caller just needs to know if there was an error, not the actual error message, so we should provide a variant using a generic, static error message.

  2. We're decoding the license (if any) on each call, see:

// FIXME(tschottdorf): see whether it makes sense to cache the decoded
// license.
if str := enterpriseLicense.Get(&st.SV); str != "" {
var err error
if lic, err = decode(str); err != nil {
return err
}
}

@erikgrinaker erikgrinaker added C-performance Perf of queries or internals. Solution not expected to change functional behavior. GA-blocker labels Mar 23, 2021
@erikgrinaker erikgrinaker self-assigned this Mar 23, 2021
@blathers-crl
Copy link

blathers-crl bot commented Mar 23, 2021

Hi @erikgrinaker, please add branch-* labels to identify which branch(es) this release-blocker affects.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@nvanbenschoten
Copy link
Member

Did we mean to label this as a GA-blocker? The performance of CheckEnterpriseEnabled did not regress in this release, did it? If not, then a fix for this will not qualify for a backport.

@erikgrinaker
Copy link
Contributor Author

erikgrinaker commented Mar 29, 2021

Did we mean to label this as a GA-blocker?The performance of CheckEnterpriseEnabled did not regress in this release, did it?

I initially thought much of this code was new, but then realized it was present in 20.2 and earlier -- forgot to update the labels, did so now. However, I'd imagine follower reads might get pretty expensive because of this.

If not, then a fix for this will not qualify for a backport.

Got it, thanks for the heads-up.

@craig craig bot closed this as completed in 36f99c3 Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-performance Perf of queries or internals. Solution not expected to change functional behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants