Skip to content

Commit

Permalink
sql: use RLock in connExecutor.CancelQuery and connExecutor.CancelAct…
Browse files Browse the repository at this point in the history
…iveQueries

Fixes #95994

`connExecutor.CancelQuery` and `connExecutor.CancelActiveQueries` do not
modify `mu.ActiveQueries` or the `*queryMetas` inside so they can safely
use `RLock` instead of `Lock`.

Release note: None
  • Loading branch information
ecwall committed Feb 1, 2023
1 parent d6a32ed commit 38299ef
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 24 deletions.
4 changes: 2 additions & 2 deletions pkg/server/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -3056,7 +3056,7 @@ func (s *statusServer) CancelSession(
}, nil
}

if err := s.checkCancelPrivilege(ctx, reqUsername, session.BaseSessionUser()); err != nil {
if err := s.checkCancelPrivilege(ctx, reqUsername, session.SessionUser()); err != nil {
// NB: not using serverError() here since the priv checker
// already returns a proper gRPC error status.
return nil, err
Expand Down Expand Up @@ -3109,7 +3109,7 @@ func (s *statusServer) CancelQuery(
}, nil
}

if err := s.checkCancelPrivilege(ctx, reqUsername, session.BaseSessionUser()); err != nil {
if err := s.checkCancelPrivilege(ctx, reqUsername, session.SessionUser()); err != nil {
// NB: not using serverError() here since the priv checker
// already returns a proper gRPC error status.
return nil, err
Expand Down
27 changes: 14 additions & 13 deletions pkg/sql/conn_executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -3144,22 +3144,26 @@ func (ex *connExecutor) hasQuery(queryID clusterunique.ID) bool {

// CancelQuery is part of the RegistrySession interface.
func (ex *connExecutor) CancelQuery(queryID clusterunique.ID) bool {
ex.mu.Lock()
defer ex.mu.Unlock()
// RLock can be used because map deletion happens in
// connExecutor.removeActiveQuery.
ex.mu.RLock()
defer ex.mu.RUnlock()
if queryMeta, exists := ex.mu.ActiveQueries[queryID]; exists {
queryMeta.cancel()
queryMeta.cancelQuery()
return true
}
return false
}

// CancelActiveQueries is part of the RegistrySession interface.
func (ex *connExecutor) CancelActiveQueries() bool {
ex.mu.Lock()
defer ex.mu.Unlock()
// RLock can be used because map deletion happens in
// connExecutor.removeActiveQuery.
ex.mu.RLock()
defer ex.mu.RUnlock()
canceled := false
for _, queryMeta := range ex.mu.ActiveQueries {
queryMeta.cancel()
queryMeta.cancelQuery()
canceled = true
}
return canceled
Expand All @@ -3174,13 +3178,10 @@ func (ex *connExecutor) CancelSession() {
ex.onCancelSession()
}

// user is part of the RegistrySession interface.
func (ex *connExecutor) user() username.SQLUsername {
return ex.sessionData().User()
}

// BaseSessionUser is part of the RegistrySession interface.
func (ex *connExecutor) BaseSessionUser() username.SQLUsername {
// SessionUser is part of the RegistrySession interface.
func (ex *connExecutor) SessionUser() username.SQLUsername {
// SessionUser is the same for all elements in the stack so use Base()
// to avoid needing a lock and race conditions.
return ex.sessionDataStack.Base().SessionUser()
}

Expand Down
11 changes: 2 additions & 9 deletions pkg/sql/exec_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -2034,12 +2034,6 @@ type queryMeta struct {
database string
}

// cancel cancels the query associated with this queryMeta, by closing the
// associated stmt context.
func (q *queryMeta) cancel() {
q.cancelQuery()
}

// SessionDefaults mirrors fields in Session, for restoring default
// configuration values in SET ... TO DEFAULT (or RESET ...) statements.
type SessionDefaults map[string]string
Expand Down Expand Up @@ -2138,9 +2132,8 @@ func (r *SessionRegistry) deregister(
}

type RegistrySession interface {
user() username.SQLUsername
// BaseSessionUser returns the base session's username.
BaseSessionUser() username.SQLUsername
// SessionUser returns the session user's username.
SessionUser() username.SQLUsername
hasQuery(queryID clusterunique.ID) bool
// CancelQuery cancels the query specified by queryID if it exists.
CancelQuery(queryID clusterunique.ID) bool
Expand Down

0 comments on commit 38299ef

Please sign in to comment.