Skip to content

Conversation

zainkabani
Copy link
Contributor

@zainkabani zainkabani commented May 31, 2023

We noticed some connection thrashing issues with using PgCat and Lifo, defaulting to using Fifo (default of bb8) and making the option configurable

The idle timeout was incorrectly commented as 10 minutes before but actually set to 1 minute. A previous PR of mine fixed the comment but it seems that a value of 10 minutes is a better default for this timeout (same as pgbouncer)

@zainkabani zainkabani changed the title Change idle timeout default to 10 minutes Make queue strategy configurable and default to Ffifo Jun 1, 2023
@zainkabani zainkabani marked this pull request as ready for review June 1, 2023 04:53
@zainkabani zainkabani changed the title Make queue strategy configurable and default to Ffifo Make queue strategy configurable and default to Fifo Jun 1, 2023

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

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

Copy link
Contributor

@levkk levkk left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@levkk levkk merged commit aca9738 into postgresml:main Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants