sql: remove redundant session iteration

[ecwall]

Fixes #95743

Improves session/query cancelation with the following

1. Replaces session scanning by session ID with map lookup.
2. Replaces active query scanning by query ID with map lookup
   (session containing query to cancel is still scanned for).
3. Does not serialize entire session to get session username or id.

Informs #77676

77676 was closed but some test cases incorrectly mentioned that addressing
77676 fixed them. This PR correctly fixes these test cases.

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 

[yuzefovich]

Nice! I have some comments and questions. It'd probably be beneficial for someone from @cockroachdb/sql-observability to take a look too.

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

---

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

	}

	hasAdmin, err := b.privilegeChecker.hasAdminRole(ctx, reqUsername)

nit: I think we can avoid this hasAdminRole call (which issues an internal SQL query under the hood) in cases when we just overwrote reqUsername to ctxUsername above. Not sure how frequent and important this is though.

---

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

			return serverError(ctx, err)
		}
		if userName.Undefined() || userName == sessionUser {

Hm, this doesn't seem like an equivalent change.

Previously the logic was:

• if reqUsername is defined
• and reqUsername is different from sessionUsername
• and ctx user is not an admin,
• then we return errRequiresAdmin.

Now the logic is:

• if reqUsername is defined
• and ctx user is not an admin,
• then we return errRequiresAdmin.

Am I missing something? Should the req user be always able to see and to cancel their own query (i.e. reqUsername == sessionUsername), regardless of the privilege?

---

pkg/sql/conn_executor.go line 3150 at r1 (raw file):

// CancelQuery is part of the RegistrySession interface.
func (ex *connExecutor) CancelQuery(queryID clusterunique.ID) bool {
	ex.mu.Lock()

nit: since in CancelQuery and CancelActiveQueries we don't delete the queries from the map and we don't modify queryMeta object, I think we can use read locks. This is orthogonal to your change, so maybe just leave a TODO to use read locks as much as possible.

[ecwall]

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

---

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

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: I think we can avoid this hasAdminRole call (which issues an internal SQL query under the hood) in cases when we just overwrote reqUsername to ctxUsername above. Not sure how frequent and important this is though.

I'll look at cleaning this up further in another PR. I think it can also be skipped
if sessionUsername == reqUsername because the cancel query privileges are only checked if !hasAdmin && sessionUsername != reqUsername

---

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

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Hm, this doesn't seem like an equivalent change.

Previously the logic was:

• if reqUsername is defined
• and reqUsername is different from sessionUsername
• and ctx user is not an admin,
• then we return errRequiresAdmin.

Now the logic is:

• if reqUsername is defined
• and ctx user is not an admin,
• then we return errRequiresAdmin.

Am I missing something? Should the req user be always able to see and to cancel their own query (i.e. reqUsername == sessionUsername), regardless of the privilege?

Yeah you are right, I changed this to reqUsername != ctxUsername && !isAdmin.

---

pkg/sql/conn_executor.go line 3150 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: since in CancelQuery and CancelActiveQueries we don't delete the queries from the map and we don't modify queryMeta object, I think we can use read locks. This is orthogonal to your change, so maybe just leave a TODO to use read locks as much as possible.

I'll check this separately also.

[ecwall]

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

---

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

Previously, ecwall (Evan Wall) wrote…

I'll look at cleaning this up further in another PR. I think it can also be skipped
if sessionUsername == reqUsername because the cancel query privileges are only checked if !hasAdmin && sessionUsername != reqUsername

#95993

---

pkg/sql/conn_executor.go line 3150 at r1 (raw file):

Previously, ecwall (Evan Wall) wrote…

I'll check this separately also.

#95994

[xinhaoz]

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

[yuzefovich]

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

[ecwall]

bors r=xinhaoz,yuzefovich

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)