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.

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

PgCat Query Mirroring #341

Merged
merged 41 commits into from
Mar 10, 2023
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
393b48c
First cut Query Mirroring
drdrsh Mar 4, 2023
396ffc7
one clone
drdrsh Mar 5, 2023
a6b9df6
fmt
drdrsh Mar 5, 2023
993d8ed
Add connection retries
drdrsh Mar 5, 2023
4c9025c
Better handling of disconnection and recv
drdrsh Mar 5, 2023
226c051
Simpler event model
drdrsh Mar 5, 2023
23e75ed
revert
drdrsh Mar 5, 2023
cd8eb9b
revert
drdrsh Mar 5, 2023
8d4af57
whitespace
drdrsh Mar 5, 2023
7392b3d
comments
drdrsh Mar 5, 2023
53b8422
refactor
drdrsh Mar 5, 2023
a78c0c6
Add tests
drdrsh Mar 5, 2023
d11fd8f
test channel overrun
drdrsh Mar 5, 2023
786ba14
logs
drdrsh Mar 5, 2023
31254eb
logs
drdrsh Mar 5, 2023
7d123aa
more messages to cover recv call
drdrsh Mar 5, 2023
4947e69
add a test to detect failure to close mirror connections
drdrsh Mar 5, 2023
cbe934b
Update src/mirrors.rs
drdrsh Mar 6, 2023
0b3dd37
Update src/pool.rs
drdrsh Mar 6, 2023
0604f9a
Merge branch 'main' of github.com:drdrsh/pgcat into mostafa_mirror3
drdrsh Mar 6, 2023
d22343a
Merge branch 'mostafa_mirror3' of github.com:drdrsh/pgcat into mostaf…
drdrsh Mar 6, 2023
672edc3
comments
drdrsh Mar 6, 2023
035fb8d
test for retries and recovery
drdrsh Mar 7, 2023
d291a1a
make mirror specs less flaky
drdrsh Mar 7, 2023
8b25b67
Simplify
drdrsh Mar 8, 2023
92ff1bd
remove redundent continue
drdrsh Mar 8, 2023
b24c187
rename method
drdrsh Mar 8, 2023
fa509c2
maybe configs would fix flakiness?
drdrsh Mar 8, 2023
781843e
one more to go
drdrsh Mar 8, 2023
d254c0f
drop connections after each test run
drdrsh Mar 9, 2023
cfc347b
some give for mirror tests
drdrsh Mar 9, 2023
9cde6e0
make address_index/address_id safer
drdrsh Mar 9, 2023
2ddd1c7
clean up
drdrsh Mar 9, 2023
795f13d
one address_id
drdrsh Mar 9, 2023
476bd43
remove flaky expectation
drdrsh Mar 9, 2023
87bf33c
build
drdrsh Mar 9, 2023
5e2e205
address comments
drdrsh Mar 10, 2023
bd437f3
revert
drdrsh Mar 10, 2023
2dfb6ff
mirror_idx
drdrsh Mar 10, 2023
69e0a0a
move tests around
drdrsh Mar 10, 2023
e7d4114
restore
drdrsh Mar 10, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,28 +18,28 @@ jobs:
RUSTFLAGS: "-Zprofile -Ccodegen-units=1 -Copt-level=0 -Clink-dead-code -Coverflow-checks=off -Zpanic_abort_tests -Cpanic=abort -Cinstrument-coverage"
RUSTDOCFLAGS: "-Cpanic=abort"
- image: postgres:14
command: ["postgres", "-p", "5432", "-c", "shared_preload_libraries=pg_stat_statements"]
command: ["postgres", "-p", "5432", "-c", "shared_preload_libraries=pg_stat_statements", "-c", "pg_stat_statements.track=all", "-c", "pg_stat_statements.max=100000"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to update the dev/docker-compose.yml as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

environment:
POSTGRES_USER: postgres
POSTGRES_DB: postgres
POSTGRES_PASSWORD: postgres
POSTGRES_INITDB_ARGS: --auth-local=md5 --auth-host=md5 --auth=md5
- image: postgres:14
command: ["postgres", "-p", "7432", "-c", "shared_preload_libraries=pg_stat_statements"]
command: ["postgres", "-p", "7432", "-c", "shared_preload_libraries=pg_stat_statements", "-c", "pg_stat_statements.track=all", "-c", "pg_stat_statements.max=100000"]
environment:
POSTGRES_USER: postgres
POSTGRES_DB: postgres
POSTGRES_PASSWORD: postgres
POSTGRES_INITDB_ARGS: --auth-local=scram-sha-256 --auth-host=scram-sha-256 --auth=scram-sha-256
- image: postgres:14
command: ["postgres", "-p", "8432", "-c", "shared_preload_libraries=pg_stat_statements"]
command: ["postgres", "-p", "8432", "-c", "shared_preload_libraries=pg_stat_statements", "-c", "pg_stat_statements.track=all", "-c", "pg_stat_statements.max=100000"]
environment:
POSTGRES_USER: postgres
POSTGRES_DB: postgres
POSTGRES_PASSWORD: postgres
POSTGRES_INITDB_ARGS: --auth-local=scram-sha-256 --auth-host=scram-sha-256 --auth=scram-sha-256
- image: postgres:14
command: ["postgres", "-p", "9432", "-c", "shared_preload_libraries=pg_stat_statements"]
command: ["postgres", "-p", "9432", "-c", "shared_preload_libraries=pg_stat_statements", "-c", "pg_stat_statements.track=all", "-c", "pg_stat_statements.max=100000"]
environment:
POSTGRES_USER: postgres
POSTGRES_DB: postgres
Expand Down
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
/target
*.deb
.vscode
.profraw
*.profraw
cov/
lcov.info

Expand Down
1 change: 0 additions & 1 deletion src/admin.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use crate::config::Role;
use crate::pool::BanReason;
/// Admin database.
use bytes::{Buf, BufMut, BytesMut};
Expand Down
21 changes: 20 additions & 1 deletion src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,16 @@ pub enum Role {
Primary,
#[serde(alias = "replica", alias = "Replica")]
Replica,
#[serde(alias = "mirror", alias = "Mirror")]
Mirror,
}

impl ToString for Role {
fn to_string(&self) -> String {
match *self {
Role::Primary => "primary".to_string(),
Role::Replica => "replica".to_string(),
Role::Mirror => "mirror".to_string(),
}
}
}
Expand Down Expand Up @@ -90,6 +93,9 @@ pub struct Address {

/// The name of this pool (i.e. database name visible to the client).
pub pool_name: String,

/// List of addresses to receive mirrored traffic.
pub mirrors: Vec<Address>,
}

impl Default for Address {
Expand All @@ -105,6 +111,7 @@ impl Default for Address {
role: Role::Replica,
username: String::from("username"),
pool_name: String::from("pool_name"),
mirrors: Vec::new(),
}
}
}
Expand All @@ -114,11 +121,14 @@ impl Address {
pub fn name(&self) -> String {
match self.role {
Role::Primary => format!("{}_shard_{}_primary", self.pool_name, self.shard),

Role::Replica => format!(
"{}_shard_{}_replica_{}",
self.pool_name, self.shard, self.replica_number
),
Role::Mirror => format!(
"{}_shard_{}_mirror_{}",
self.pool_name, self.shard, self.replica_number
),
}
}
}
Expand Down Expand Up @@ -465,11 +475,19 @@ pub struct ServerConfig {
pub role: Role,
}

#[derive(Clone, PartialEq, Serialize, Deserialize, Debug, Hash, Eq)]
pub struct MirrorServerConfig {
pub host: String,
pub port: u16,
pub mirroring_target_index: usize,
}

/// Shard configuration.
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Hash, Eq)]
pub struct Shard {
pub database: String,
pub servers: Vec<ServerConfig>,
pub mirrors: Option<Vec<MirrorServerConfig>>,
}

impl Shard {
Expand Down Expand Up @@ -518,6 +536,7 @@ impl Default for Shard {
port: 5432,
role: Role::Primary,
}],
mirrors: None,
database: String::from("postgres"),
}
}
Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ pub mod config;
pub mod constants;
pub mod errors;
pub mod messages;
pub mod mirrors;
pub mod multi_logger;
pub mod pool;
pub mod scram;
Expand Down
1 change: 1 addition & 0 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ mod config;
mod constants;
mod errors;
mod messages;
mod mirrors;
mod multi_logger;
mod pool;
mod prometheus;
Expand Down
153 changes: 153 additions & 0 deletions src/mirrors.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
/// A mirrored PostgreSQL client.
/// Packets arrive to us through a channel from the main client and we send them to the server.
use bb8::Pool;
use bytes::{Bytes, BytesMut};

use crate::config::{Address, Role, User};
use crate::pool::{ClientServerMap, ServerPool};
use crate::stats::get_reporter;
use log::{error, info, trace, warn};
use tokio::sync::mpsc::{channel, Receiver, Sender};

pub struct MirroredClient {
address: Address,
user: User,
database: String,
bytes_rx: Receiver<Bytes>,
disconnect_rx: Receiver<()>,
}

impl MirroredClient {
async fn create_pool(&self) -> Pool<ServerPool> {
let manager = ServerPool::new(
self.address.clone(),
self.user.clone(),
self.database.as_str(),
ClientServerMap::default(),
get_reporter(),
);

Pool::builder()
.max_size(1)
.connection_timeout(std::time::Duration::from_millis(10_000))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to use the config values here, so we don't get long timeouts unexpectedly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

.idle_timeout(Some(std::time::Duration::from_millis(10_000)))
.test_on_check_out(false)
.build(manager)
.await
.unwrap()
}

pub fn start(mut self) {
tokio::spawn(async move {
let pool = self.create_pool().await;
let address = self.address.clone();
loop {
let mut server = match pool.get().await {
Ok(server) => server,
Err(err) => {
error!(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mark_bad ?

Copy link
Collaborator Author

@drdrsh drdrsh Mar 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mark_bad works if we have a server connection. In this case we failed to checkout a connection from the pool so we have no server to mark_bad.

In the non-mirrored version, we ban the server but in the mirrored mode, it doesn't make sense to ban (we don't have banlists and banning in a mirrored setup is not very useful)

"Failed to get connection from pool, Discarding message {:?}, {:?}",
err,
address.clone()
);
continue;
}
};

tokio::select! {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It may be possible to refactor this outer loop into it's own function returning an Option so we can use the ? operator in that function. The goal being to remove the calls to unwrap. I realize we're breaking / continuing using if option.is_none(), but if someone refactors later and removes the break / continue this could start failing in production.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I simplified the logic a ton, I think your concern should be addressed now.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. 🙇

// Exit channel events
_ = self.disconnect_rx.recv() => {
info!("Got mirror exit signal, exiting {:?}", address.clone());
break;
}

// Incoming data from server (we read to clear the socket buffer and discard the data)
recv_result = server.recv() => {
match recv_result {
Ok(message) => trace!("Received from mirror: {} {:?}", String::from_utf8_lossy(&message[..]), address.clone()),
Err(err) => error!("Failed to receive from mirror {:?} {:?}", err, address.clone())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you mark_bad here and make bb8 create you a new server?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left that up to server.recv. It calls mark_bad in a handful of places. Similarly for server.send. Double marking bad should be fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should mark_bad to be safe or just leave it up to the server logic to handle it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. An extra mark_bad does not hurt. It documents the behavior here

}
}

// Messages to send to the server
message = self.bytes_rx.recv() => {
match message {
Some(bytes) => {
match server.send(&BytesMut::from(&bytes[..])).await {
Ok(_) => trace!("Sent to mirror: {} {:?}", String::from_utf8_lossy(&bytes[..]), address.clone()),
Err(err) => error!("Failed to send to mirror, Discarding message {:?}, {:?}", err, address.clone())
}
}
None => {
info!("Mirror channel closed, exiting {:?}", address.clone());
break;
},
}
}
}
}
});
}
}
pub struct MirroringManager {
pub byte_senders: Vec<Sender<Bytes>>,
pub disconnect_senders: Vec<Sender<()>>,
}
impl MirroringManager {
pub fn from_addresses(
user: User,
database: String,
addresses: Vec<Address>,
) -> MirroringManager {
let mut byte_senders: Vec<Sender<Bytes>> = vec![];
let mut exit_senders: Vec<Sender<()>> = vec![];

addresses.iter().for_each(|mirror| {
let (bytes_tx, bytes_rx) = channel::<Bytes>(500);
let (exit_tx, exit_rx) = channel::<()>(1);
let mut addr = mirror.clone();
addr.role = Role::Mirror;
let client = MirroredClient {
user: user.clone(),
database: database.to_owned(),
address: addr,
bytes_rx,
disconnect_rx: exit_rx,
};
exit_senders.push(exit_tx.clone());
byte_senders.push(bytes_tx.clone());
client.start();
});

Self {
byte_senders: byte_senders,
disconnect_senders: exit_senders,
}
}

pub fn send(self: &mut Self, bytes: &BytesMut) {
let cpy = bytes.clone().freeze();
self.byte_senders
.iter_mut()
.for_each(|sender| match sender.try_send(cpy.clone()) {
Ok(_) => {}
Err(err) => {
warn!("Failed to send bytes to a mirror channel {}", err);
}
});
}

pub fn disconnect(self: &mut Self) {
self.disconnect_senders
.iter_mut()
.for_each(|sender| match sender.try_send(()) {
Ok(_) => {}
Err(err) => {
warn!(
"Failed to send disconnect signal to a mirror channel {}",
err
);
}
});
}
}
27 changes: 26 additions & 1 deletion src/pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ impl ConnectionPool {
let config = get_config();

let mut new_pools = HashMap::new();
let mut address_id = 0;
let mut address_id: usize = 0;

for (pool_name, pool_config) in &config.pools {
let new_pool_hash_value = pool_config.hash_value();
Expand Down Expand Up @@ -244,7 +244,31 @@ impl ConnectionPool {
let mut servers = Vec::new();
let mut replica_number = 0;

// Load Mirror settings
for (address_index, server) in shard.servers.iter().enumerate() {
let mut mirror_addresses: Vec<Address> = vec![];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need a type annotation typically, Rust should infer it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if let Some(mirror_settings_vec) = &shard.mirrors {
for mirror_settings in mirror_settings_vec {
if mirror_settings.mirroring_target_index != address_index {
continue;
}
mirror_addresses.push(Address {
id: address_id,
database: shard.database.clone(),
host: mirror_settings.host.clone(),
port: mirror_settings.port,
role: server.role,
address_index: 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We send stats from the mirrors , so we should make sure these unique identifiers are unique.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the stats use the address_id, not the address_index. The address_id is unique.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the address_index callsites
https://cs.github.com/levkk/pgcat?q=.address_index

They are both irrelevant to mirrors

Copy link
Collaborator Author

@drdrsh drdrsh Mar 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed the same pattern we do for the server addresses. For the server addresses, we set the address_index to equal the index of the server in the configs array. We do the same for mirrors, we set the mirror address_index to be the index of the mirror in the mirror config array

https://github.com/levkk/pgcat/blob/main/src/pool.rs#L247-L254

replica_number,
shard: shard_idx.parse::<usize>().unwrap(),
username: user.username.clone(),
pool_name: pool_name.clone(),
mirrors: vec![],
});
address_id += 1;
}
}

let address = Address {
id: address_id,
database: shard.database.clone(),
Expand All @@ -256,6 +280,7 @@ impl ConnectionPool {
shard: shard_idx.parse::<usize>().unwrap(),
username: user.username.clone(),
pool_name: pool_name.clone(),
mirrors: mirror_addresses,
};

address_id += 1;
Expand Down
1 change: 1 addition & 0 deletions src/query_router.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ impl QueryRouter {
Command::ShowServerRole => match self.active_role {
Some(Role::Primary) => Role::Primary.to_string(),
Some(Role::Replica) => Role::Replica.to_string(),
Some(Role::Mirror) => Role::Mirror.to_string(),
None => {
if self.query_parser_enabled() {
String::from("auto")
Expand Down
Loading