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.

Sign up for GitHub

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

Jump to bottom

sql: reduce contention between session (de)registration and query cancelation #95535

Closed
ecwall opened this issue Jan 19, 2023 · 2 comments · Fixed by #95553
Closed

sql: reduce contention between session (de)registration and query cancelation #95535

ecwall opened this issue Jan 19, 2023 · 2 comments · Fixed by #95553
Assignees
@ecwall
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@ecwall
Copy link
Contributor

ecwall commented Jan 19, 2023 •
edited
Loading

The SessionRegistry contains a mutex is used for session registration, de-registration, and query cancelation:

cockroach/pkg/sql/exec_util.go

Line 2065 in 9735954

syncutil.Mutex

Registration and de-registration are fast and should not hold the lock very long, but query cancelation can be long running.

We should be able to use the mutex to lock looking up the session, but release it after the session has been looked up for the duration of the query cancelations.

Jira issue: CRDB-23574
@ecwall ecwall added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Jan 19, 2023
@ecwall ecwall self-assigned this Jan 19, 2023
@rafiss
Copy link
Collaborator

rafiss commented Jan 19, 2023

Where is the session registry lock held? So far I only found one usage:

cockroach/pkg/server/status.go

Line 219 in 9735954

b.sessionRegistry.Lock()

@ecwall ecwall changed the title Reduce contention between session (de)registration and query cancelation sql: reduce contention between session (de)registration and query cancelation Jan 19, 2023
@ecwall
Copy link
Contributor Author

ecwall commented Jan 19, 2023

Where is the session registry lock held? So far I only found one usage:

cockroach/pkg/server/status.go

Line 219 in 9735954

b.sessionRegistry.Lock()

There are more usages in the SessionRegistry methods below the struct.

@ecwall ecwall mentioned this issue Jan 19, 2023
Merged
craig bot pushed a commit that referenced this issue Jan 20, 2023
@ecwall @knz
Merge #95553 #95573
875420d 
95553: sql: reduce contention on `SessionRegistry` mutex r=yuzefovich,rafiss a=ecwall

Fixes #95535

This reduces `SessionRegistry` contention in 2 ways:
1) Change `SessionRegistry` `syncutil.Mutex` to `syncutil.RWMutex` and use `RLock()` in place of `Lock()` when not modifying the registry.
2) Only lock reading/writing `SessionRegistry` maps with mutex instead of including the session operations in the lock.

Release note (bug fix): Reduce register session, deregister session, and session cancel query contention.

95573: sql,multitenant: simplify the cap pb package name r=arulajmani a=knz

Fixes  #95572

proto packages don't need to have a relationship with the filesystem path.

Release note: None

Co-authored-by: Evan Wall <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
@craig craig bot closed this as completed in 56d3687 Jan 20, 2023
blathers-crl bot pushed a commit that referenced this issue Jan 20, 2023
@ecwall
sql: reduce contention on SessionRegistry mutex
7db94c0 
Fixes #95535

This reduces `SessionRegistry` contention in 2 ways:
1) Change `SessionRegistry` `syncutil.Mutex` to `syncutil.RWMutex` and use
`RLock()` in place of `Lock()` when not modifying the registry.
2) Only lock reading/writing `SessionRegistry` maps with mutex instead of
including the session operations in the lock.

Release note (bug fix): Reduce register session, deregister session, and
session cancel query contention.
@blathers-crl blathers-crl bot mentioned this issue Jan 20, 2023
Merged
@ecwall ecwall mentioned this issue Jan 21, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ecwall ecwall

Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql: reduce contention on SessionRegistry mutex
2 participants
@rafiss @ecwall