sql: refactor checkCancelPrivilege to avoid unessessary hasAdminRole call

[ecwall]

Fixes #95993

Reorders the logic in checkCancelPrivilege to only call hasAdminRole when necessary.

Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[rafiss]

out of curiosity, if we use this diff instead:

diff --git a/pkg/sql/syntheticprivilegecache/accumulator.go b/pkg/sql/syntheticprivilegecache/accumulator.go
index 53fdd9533b..680dcb3bb7 100644
--- a/pkg/sql/syntheticprivilegecache/accumulator.go
+++ b/pkg/sql/syntheticprivilegecache/accumulator.go
@@ -29,7 +29,7 @@ type accumulator struct {
 // newAccumulator initializes a Accumulator.
 func newAccumulator(objectType privilege.ObjectType, path string) *accumulator {
     return &accumulator{
-        desc:       &catpb.PrivilegeDescriptor{},
+        desc:       catpb.NewBasePrivilegeDescriptor(username.NodeUserName()),
         objectType: objectType,
         path:       path,
     }

would that let us remove this call to hasAdminRole entirely. this diff should cause the hasGlobalPrivilege function called on line 309 to always return true if the user is an admin

[yuzefovich]

LGTM, but I'll defer to Rafi on this one.

Reviewed 1 of 2 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ecwall)

[rafiss]

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

---

pkg/server/status.go line 295 at r2 (raw file):

	// Check if the user has permission to see the session.
	// If reqUsername == ctxUsername && isCtxAdmin then reqUsername is admin.
	if sessionUsername != reqUsername && !(reqUsername == ctxUsername && isCtxAdmin) {

nit: i feel like this would be easier to read with short-circuits. it also allows you to comment each short-circuit condition more clearly.

// A user can always cancel their own sessions.
if sessionUsername == reqUsername {
  return nil
}

// If reqUsername == ctxUsername && isCtxAdmin then reqUsername is admin.
if reqUsername == ctxUsername && isCtxAdmin {
  return nil
}

// rest of the code

[ecwall]

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

---

pkg/server/status.go line 300 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

out of curiosity, if we use this diff instead:

diff --git a/pkg/sql/syntheticprivilegecache/accumulator.go b/pkg/sql/syntheticprivilegecache/accumulator.go
index 53fdd9533b..680dcb3bb7 100644
--- a/pkg/sql/syntheticprivilegecache/accumulator.go
+++ b/pkg/sql/syntheticprivilegecache/accumulator.go
@@ -29,7 +29,7 @@ type accumulator struct {
 // newAccumulator initializes a Accumulator.
 func newAccumulator(objectType privilege.ObjectType, path string) *accumulator {
     return &accumulator{
-        desc:       &catpb.PrivilegeDescriptor{},
+        desc:       catpb.NewBasePrivilegeDescriptor(username.NodeUserName()),
         objectType: objectType,
         path:       path,
     }

would that let us remove this call to hasAdminRole entirely. this diff should cause the hasGlobalPrivilege function called on line 309 to always return true if the user is an admin

I tried it, but it caused some tests to fail https://teamcity.cockroachdb.com/viewLog.html?buildId=8544383&buildTypeId=Cockroach_BazelEssentialCi.

[ecwall]

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

---

pkg/server/status.go line 295 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: i feel like this would be easier to read with short-circuits. it also allows you to comment each short-circuit condition more clearly.

// A user can always cancel their own sessions.
if sessionUsername == reqUsername {
  return nil
}

// If reqUsername == ctxUsername && isCtxAdmin then reqUsername is admin.
if reqUsername == ctxUsername && isCtxAdmin {
  return nil
}

// rest of the code

Updated.

[ecwall]

bors r=rafiss

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)