sql: use RLock in connExecutor.CancelQuery and connExecutor.CancelActiveQueries #96241

ecwall · 2023-01-30T22:01:48Z

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

blathers-crl · 2023-01-30T22:01:52Z

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 · 2023-01-30T22:01:59Z

This change is

ecwall · 2023-01-31T00:05:30Z

pkg/sql/exec_util.go

@@ -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() {


This method just calls a similarly named function field and seems like an unnecessary layer.

rafiss

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

-- commits line 7 at r1:
just curious: then what is responsible for removing the query from the ActiveQueries list?

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

// BaseSessionUser is part of the RegistrySession interface.
func (ex *connExecutor) BaseSessionUser() username.SQLUsername {

nit: the word Base is redundant in the function name. the session user is immutable, and always is the user who originally logged in. (you could also replace ex.sessionDataStack.Base().SessionUser() below with ex.sessionDataStack.SessionUser())

ecwall

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

-- commits line 7 at r1:

Previously, rafiss (Rafi Shamim) wrote…

just curious: then what is responsible for removing the query from the ActiveQueries list?

It happens here in a defer after it is added to ActiveQueries:

cockroach/pkg/sql/conn_executor_exec.go

Line 351 in 194f6a1

ex.removeActiveQuery(queryID, ast)

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

Previously, rafiss (Rafi Shamim) wrote…

nit: the word Base is redundant in the function name. the session user is immutable, and always is the user who originally logged in. (you could also replace ex.sessionDataStack.Base().SessionUser() below with ex.sessionDataStack.SessionUser())

I renamed BaseSessionUser() to SessionUser(), but Stack does not have a SessionUser() method.

Are you saying that

func (s *SessionData) SessionUser() username.SQLUsername {
	if s.SessionUserProto == "" {
		return s.User()
	}
	return s.SessionUserProto.Decode()
}

should actually be a method on Stack?

func (s *Stack) SessionUser() username.SQLUsername {
	session := s.base
	if session.SessionUserProto == "" {
		return session.User()
	}
	return session.SessionUserProto.Decode()
}

rafiss

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

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

Previously, ecwall (Evan Wall) wrote…

I renamed BaseSessionUser() to SessionUser(), but Stack does not have a SessionUser() method.

Are you saying that

func (s *SessionData) SessionUser() username.SQLUsername {
	if s.SessionUserProto == "" {
		return s.User()
	}
	return s.SessionUserProto.Decode()
}

should actually be a method on Stack?

func (s *Stack) SessionUser() username.SQLUsername {
	session := s.base
	if session.SessionUserProto == "" {
		return session.User()
	}
	return session.SessionUserProto.Decode()
}

no, i mistyped. i meant to say that you can use ex.sessionData().SessionUser(). analogous to the user() function you deleted above.

yuzefovich

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

-- commits line 7 at r1:

Previously, ecwall (Evan Wall) wrote…

It happens here in a defer after it is added to ActiveQueries:

cockroach/pkg/sql/conn_executor_exec.go

Line 351 in 194f6a1

ex.removeActiveQuery(queryID, ast)

When I was reviewing #95745, I was also confused by this. Consider adding a quick comment to CancelQuery and CancelActiveQueries that we can use read-lock there because the actual removal of the query meta from the map occurs in removeActiveQuery.

rafiss

lgtm with one nit, and i also agree with yahor's sugegstion to comment this

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

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

Previously, rafiss (Rafi Shamim) wrote…

no, i mistyped. i meant to say that you can use ex.sessionData().SessionUser(). analogous to the user() function you deleted above.

nit: i think it would be more consistent with other usages of it were changed

ecwall

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

-- commits line 7 at r1:

Previously, yuzefovich (Yahor Yuzefovich) wrote…

When I was reviewing #95745, I was also confused by this. Consider adding a quick comment to CancelQuery and CancelActiveQueries that we can use read-lock there because the actual removal of the query meta from the map occurs in removeActiveQuery.

Added

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

Previously, rafiss (Rafi Shamim) wrote…

nit: i think it would be more consistent with other usages of it were changed

That is using the SessionData from the top of the stack instead of the bottom like before.

rafiss

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

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

Previously, ecwall (Evan Wall) wrote…

That is using the SessionData from the top of the stack instead of the bottom like before.

as i mentioned above, SessionUser is immutable. see

cockroach/pkg/sql/sessiondata/session_data.go

Lines 148 to 151 in d6a32ed

    
           // SessionUser retrieves the session_user. 
        
           // The SessionUser is the username that originally logged into the session. 
        
           // If a user applies SET ROLE, the SessionUser remains the same whilst the 
        
           // User() changes.

rafiss

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

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

Previously, rafiss (Rafi Shamim) wrote…

as i mentioned above, SessionUser is immutable. see

cockroach/pkg/sql/sessiondata/session_data.go

Lines 148 to 151 in d6a32ed

// SessionUser retrieves the session_user.

// The SessionUser is the username that originally logged into the session.

// If a user applies SET ROLE, the SessionUser remains the same whilst the

// User() changes.

using Base does work, but it's just confusing since it's inconsistent with all the other usages of SessionUser

…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

ecwall

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

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

Previously, rafiss (Rafi Shamim) wrote…

using Base does work, but it's just confusing since it's inconsistent with all the other usages of SessionUser

Talked about it in slack. Only the base stack element is thread safe so adding a comment to explain why we are using it instead of the more common ex.sessionData().

ecwall · 2023-02-01T23:15:50Z

bors r=rafiss,yuzefovich

craig · 2023-02-01T23:30:44Z

This PR was included in a batch that was canceled, it will be automatically retried

craig · 2023-02-02T01:51:54Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-02-02T02:29:29Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-02-02T05:22:43Z

Build succeeded:

Bazel Essential CI (Cockroach)

ecwall requested a review from yuzefovich January 30, 2023 22:01

ecwall requested a review from a team as a code owner January 30, 2023 22:01

ecwall requested a review from rafiss January 30, 2023 22:01

ecwall commented Jan 31, 2023

View reviewed changes

rafiss reviewed Feb 1, 2023

View reviewed changes

ecwall commented Feb 1, 2023

View reviewed changes

rafiss reviewed Feb 1, 2023

View reviewed changes

ecwall requested a review from a team February 1, 2023 18:48

ecwall requested a review from a team as a code owner February 1, 2023 18:48

yuzefovich approved these changes Feb 1, 2023

View reviewed changes

rafiss approved these changes Feb 1, 2023

View reviewed changes

ecwall commented Feb 1, 2023

View reviewed changes

rafiss reviewed Feb 1, 2023

View reviewed changes

ecwall commented Feb 1, 2023

View reviewed changes

craig bot merged commit 22244a7 into cockroachdb:master Feb 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: use RLock in connExecutor.CancelQuery and connExecutor.CancelActiveQueries #96241

sql: use RLock in connExecutor.CancelQuery and connExecutor.CancelActiveQueries #96241

ecwall commented Jan 30, 2023

blathers-crl bot commented Jan 30, 2023

cockroach-teamcity commented Jan 30, 2023

ecwall Jan 31, 2023

rafiss left a comment

ecwall left a comment

rafiss left a comment

yuzefovich left a comment

rafiss left a comment

ecwall left a comment

rafiss left a comment

rafiss left a comment

ecwall left a comment

ecwall commented Feb 1, 2023

craig bot commented Feb 1, 2023

craig bot commented Feb 2, 2023

craig bot commented Feb 2, 2023

craig bot commented Feb 2, 2023

	// SessionUser retrieves the session_user.
	// The SessionUser is the username that originally logged into the session.
	// If a user applies SET ROLE, the SessionUser remains the same whilst the
	// User() changes.

sql: use RLock in connExecutor.CancelQuery and connExecutor.CancelActiveQueries #96241

sql: use RLock in connExecutor.CancelQuery and connExecutor.CancelActiveQueries #96241

Conversation

ecwall commented Jan 30, 2023

blathers-crl bot commented Jan 30, 2023

cockroach-teamcity commented Jan 30, 2023

ecwall Jan 31, 2023

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

ecwall left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

ecwall left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

ecwall left a comment

Choose a reason for hiding this comment

ecwall commented Feb 1, 2023

craig bot commented Feb 1, 2023

craig bot commented Feb 2, 2023

craig bot commented Feb 2, 2023

craig bot commented Feb 2, 2023