-
Notifications
You must be signed in to change notification settings - Fork 193
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.
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
Add support for multi-database / multi-user pools #96
Conversation
return Ok(Client { | ||
read: BufReader::new(read), | ||
write: write, | ||
addr, | ||
buffer: BytesMut::with_capacity(8196), | ||
cancel_mode: true, | ||
transaction_mode: transaction_mode, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cancel mode don't care about transaction_mode so it is easier to hard code a value instead of working our way backwards from the secret to the corresponding pool
src/client.rs
Outdated
target_pool = match get_pool(database.clone(), user.clone()) { | ||
Some(pool) => pool, | ||
None => { | ||
error_response( | ||
&mut write, | ||
&format!("No pool configured for database: {:?}, user: {:?}", database, user), | ||
).await?; | ||
return Err(Error::ClientError) | ||
} | ||
}; | ||
transaction_mode = target_pool.settings.pool_mode == "transaction"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This concept of target_pool
simplifies the implementation. Once a client connects it is permanently assigned a target_pool
. This target_pool
connects to one cluster (with whatever shards/instances configured for that cluster) using a specific user. Almost everything after more or less behaves identically to how Pgcat operated before
let correct_user = config.general.admin_username.as_str(); | ||
let correct_password = config.general.admin_password.as_str(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is new. Instead of any authenticated user being allowed to access admin db, a specific username/password is required for that. Honestly, I went with that for simplicity
@@ -410,7 +448,7 @@ where | |||
|
|||
// The query router determines where the query is going to go, | |||
// e.g. primary, replica, which shard. | |||
let mut query_router = QueryRouter::new(); | |||
let mut query_router = QueryRouter::new(self.target_pool.clone()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Query routing configs are per-pool now and not global so the router needs access to the pool configs
src/pool.rs
Outdated
pub static POOLS: Lazy<ArcSwap<PoolMap>> = | ||
Lazy::new(|| ArcSwap::from_pointee(HashMap::default())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
POOL
-> POOLS
😛
src/admin.rs
Outdated
let value = stats.get(column.0).unwrap_or(&0).to_string(); | ||
row.push(value); | ||
for (_, pool) in get_all_pools() { | ||
let pool_config = pool.settings.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can borrow the pool config here, e.g. let pool_config = &pool.settings;
, but it doesn't really matter.
src/client.rs
Outdated
} | ||
|
||
debug!("Password authentication successful"); | ||
|
||
auth_ok(&mut write).await?; | ||
write_all(&mut write, get_pool().server_info()).await?; | ||
backend_key_data(&mut write, process_id, secret_key).await?; | ||
if !admin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can send this for admin users too, it won't hurt I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For admin, there isn't really any server, and the pool we use to get server_info
is a dummy one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but if you don't send server info, the psql will look weird. We prewarm the pool on boot, so we actually have server info, so we have something to send to the client.
src/config.rs
Outdated
|
||
for (pool_name, pool_config) in &self.pools { | ||
info!("--- Settings for pool {} ---", pool_name); | ||
info!("Total Pool size from all users: {}", pool_config.users.iter().map(|(_, user_cfg)| user_cfg.pool_size ).sum::<u32>().to_string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
info!("Total Pool size from all users: {}", pool_config.users.iter().map(|(_, user_cfg)| user_cfg.pool_size ).sum::<u32>().to_string()); | |
info!("Pool size from all users: {}", pool_config.users.iter().map(|(_, user_cfg)| user_cfg.pool_size ).sum::<u32>().to_string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
No description provided.