Skip to content

Make queue strategy configurable and default to Fifo #463

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

Merged
merged 5 commits into from
Jun 9, 2023
Merged

Make queue strategy configurable and default to Fifo #463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
075b8a8
Change idle timeout default to 10 minutes
zainkabani May 31, 2023
01e9145
Revert lifo for now while we investigate connection thrashing issues
zainkabani May 31, 2023
ddaab1a
Make queue strategy configurable
zainkabani Jun 1, 2023
3d15710
test revert idle time out
zainkabani Jun 1, 2023
9b6b6c5
Add pgcat start to python test
zainkabani Jun 1, 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
13 changes: 11 additions & 2 deletions src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,9 @@ pub struct General {
#[serde(default = "General::default_server_lifetime")]
pub server_lifetime: u64,

#[serde(default = "General::default_server_round_robin")] // False
pub server_round_robin: bool,

#[serde(default = "General::default_worker_threads")]
pub worker_threads: usize,

Expand Down Expand Up @@ -352,7 +355,7 @@ impl General {
}

pub fn default_idle_timeout() -> u64 {
60000 // 1 minute
600000 // 10 minutes
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not change defaults unnecessarily. People are getting used to them, and it's much easier to change the config when a change is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a valid point.

I'm mainly changing this to match the pgbouncer default, but if we want to keep it as 1 minute then I can revert

}

pub fn default_shutdown_timeout() -> u64 {
Expand Down Expand Up @@ -390,6 +393,10 @@ impl General {
pub fn default_prometheus_exporter_port() -> i16 {
9930
}

pub fn default_server_round_robin() -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

This brings us back to original PgCat behavior before the bb8 queuing strategy change, is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct, using a queue instead of stack as the default. Once we figure out if this is causing connection thrashing we can switch the default and match pgbouncer

true
}
}

impl Default for General {
Expand Down Expand Up @@ -424,7 +431,8 @@ impl Default for General {
auth_query: None,
auth_query_user: None,
auth_query_password: None,
server_lifetime: 1000 * 3600 * 24, // 24 hours,
server_lifetime: Self::default_server_lifetime(),
server_round_robin: false,
validate_config: true,
}
}
Expand Down Expand Up @@ -983,6 +991,7 @@ impl Config {
"Default max server lifetime: {}ms",
self.general.server_lifetime
);
info!("Sever round robin: {}", self.general.server_round_robin);
match self.general.tls_certificate.clone() {
Some(tls_certificate) => {
info!("TLS certificate: {}", tls_certificate);
Expand Down
7 changes: 6 additions & 1 deletion src/pool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,11 @@ impl ConnectionPool {
.min()
.unwrap();

let queue_strategy = match config.general.server_round_robin {
true => QueueStrategy::Fifo,
false => QueueStrategy::Lifo,
};

debug!(
"[pool: {}][user: {}] Pool reaper rate: {}ms",
pool_name, user.username, reaper_rate
Expand All @@ -401,7 +406,7 @@ impl ConnectionPool {
.idle_timeout(Some(std::time::Duration::from_millis(idle_timeout)))
.max_lifetime(Some(std::time::Duration::from_millis(server_lifetime)))
.reaper_rate(std::time::Duration::from_millis(reaper_rate))
.queue_strategy(QueueStrategy::Lifo)
.queue_strategy(queue_strategy)
.test_on_check_out(false);

let pool = if config.general.validate_config {
Expand Down
1 change: 1 addition & 0 deletions tests/python/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ def cleanup_conn(conn: psycopg2.extensions.connection, cur: psycopg2.extensions.


def test_normal_db_access():
pgcat_start()
conn, cur = connect_db(autocommit=False)
cur.execute("SELECT 1")
res = cur.fetchall()
Expand Down