adapter: refactor max connection handling #30624
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* pull connection handling out of the sql crate and into pgwire_common * refactor SystemVars to have a set of callbacks to notify interested folks in changes to variables * refactor max connection handling so we don't panic if the limit drops to less than current number of connections
5 tasks
jkosh44
approved these changes
Nov 25, 2024
src/adapter/src/coord.rs
Outdated
Comment on lines
3943
to
3944
| // If superuser_reserved > max_connections, prefer max_connections. | ||
| let superuser_reserved = std::cmp::min(limit, superuser_reserved); |
There was a problem hiding this comment.
If superuser_reserved > max_connections is true, then that means only super users could connect to the database right? i.e., all connections are reserved for superusers and normal users will be locked out. That feels like a bad enough situation that we should add an error log here.
There was a problem hiding this comment.
Great call, added a soft assert here!
* add some extra asserts * update comments * add test case to make sure we don't panic when dropping below number of open connections
|
Added a test that previously repro'd the panic! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR refactors our max connection handling in a few ways:
sqlcrate and moves it into thepgwire_commoncrate. AFAICT this logic lived in thesqlcrate so it could be updated viaSystemVars.SystemVarsto contain a generic set of callbacks so anyone can register to be notified about changes to a system variable, not just connection handling.It also fixes a bug that has occurred in production. Previously if the max connection limit dropped below the number of current connections (e.g. reducing the LaunchDarkly flag) we could panic.
Motivation
Fixes a known panic and refactors the code.
Tips for reviewer
I would start by reviewing the changes in
sql/src/session/vars.rsto see how SystemVars changed, then look at the changes inpgwire-common/src/conn.rs.Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.