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

Configurable routing for queries without routing comment #567

Closed
wants to merge 5 commits into from

Conversation

drdrsh
Copy link
Collaborator

@drdrsh drdrsh commented Aug 29, 2023

Ruby-pg library makes a startup query to SET client_encoding to ... if Encoding.default_internal value is set (Code). This query is troublesome because we cannot possibly attach a routing comment to it. PgCat, by default, will route that query to the default shard.

Everything is fine until shard 0 has issues, Clients will all be attempting to send this query to shard0 which increases the connection latency significantly for all clients, even those not interested in shard0

This PR introduces no_shard_specified_behavior that defines the behavior in case we have routing-by-comment enabled but we get a query without a comment. The allowed behaviors are

random: Picks a shard at random
random_healthy: Picks a shard at random favoring shards with the least number of recent connection/checkout errors
shard_<number>: e.g. shard_0, shard_4, etc. picks a specific shard, everytime

In order to achieve this, this PR introduces an error_count on the Address Object that tracks the number of errors since the last checkout and uses that metric to sort shards by error count before making a routing decision.
I didn't want to use address stats to avoid introducing a routing dependency on internal stats (We might do that in the future but I prefer to avoid this for the time being.

I also made changes to the test environment to replace Ruby's TOML reader library, It appears to be abandoned and does not support mixed arrays (which we use in the config toml), and it also does not play nicely with single-quoted regular expressions. I opted for using yj which is a CLI tool that can convert from toml to JSON and back. So I refactor the tests to use that library.

@drdrsh drdrsh mentioned this pull request Aug 29, 2023
match self.pool_settings.no_shard_specified_behavior {
Some(behavior) => {
match behavior {
NoShardSpecifiedHandling::Random => {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can match the option and the enum both in one go to reduce a level of indentation

match option {
  Some(NoShardSpecifiedHandling::Random) => {}
Some(NoShardSpecifiedHandling::RandomHealthy) => {}
...

}

NoShardSpecifiedHandling::Shard(shard_id) => {
if shard_id >= self.pool_settings.shards {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we validate this in the config?

NoShardSpecifiedHandling::Shard(shard_id) => {
if shard_id >= self.pool_settings.shards {
warn!("Shard id {} is out of range, defaulting to 0", shard_id);
self.set_shard(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this represent a significant functionality change? If there's 5 shards and someone queries shard 6 the current version triggers an error and this will now reroute it to shard 0 with no indication of that to the client. Am I reading this code right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. I can make that change.
Does the current implementation trigger an error or does it panic?

Copy link
Contributor

Choose a reason for hiding this comment

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

Current version panics. It would be far better if it returned an error to the client, but also that logic is likely better somewhere else and in another PR.

@drdrsh drdrsh closed this Sep 17, 2023
@drdrsh drdrsh deleted the mostafa_routing-unscoped branch August 29, 2024 14:02
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.

3 participants