The TL;DR for the change is that we allow QueryRouter to set the active shard to None. This signals to the Pool::get method that we have no shard selected. The get method follows a no_shard_specified_behavior config to know how to route the query.
Original PR description
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.
* User server parameters struct instead of server info bytesmut
* Refactor to use hashmap for all params and add server parameters to client
* Sync parameters on client server checkout
* minor refactor
* update client side parameters when changed
* Move the SET statement logic from the C packet to the S packet.
* trigger build
* revert validation changes
* remove comment
* Try fix
* Reset cleanup state after sync
* fix server version test
* Track application name through client life for stats
* Add tests
* minor refactoring
* fmt
* fix
* fmt
* Make infer role configurable and fix double parse bug
* Fix tests
* Enable infer_role_from query in toml for tests
* Fix test
* Add max length config, add logging for which application is failing to parse, and change config name
* fmt
* Update src/config.rs
---------
Co-authored-by: Lev Kokotov <levkk@users.noreply.github.com>
* Move connection checkin log messages to their own target
Under heavy load they can happen thousands of times per second, and
should generally be considered a nuisance at best. This marks the state
discard as an info rather than a warning, and moves all the messages
into their own log-target, so they can be filtered separately from the
more relevant warnings.
Signed-off-by: D.S. Ljungmark <spider@skuggor.se>
* Remove left-over env_logger dependencies
When moving to tracing-subscriber for logging, the env_logger
dependencies were left around, this cuts them out as dead code.
Signed-off-by: D.S. Ljungmark <spider@skuggor.se>
* Restore ability to filter log messages at runtime
This restores the RUST_LOG filters from env_logger but now with the
tracing subscriber setup. The filters are chained so commandline options
mark the default in case either option is set, which should be the path
of least confusion for users. ( RUST_LOG setting level to debug, and
commandline to warning is an odd user case, and I don't know what a user
who does that is expecting. )
It also bumps the version number as a fix to see which versions have
which behaviour.
Signed-off-by: D.S. Ljungmark <spider@skuggor.se>
---------
Signed-off-by: D.S. Ljungmark <spider@skuggor.se>
add --no-color option to disable colors
this commit adds a new option to disable colors in the terminal and also
moves the logger configuration to a different crate.
Signed-off-by: Sebastian Webber <sebastian@swebber.me>
This commit adds the clap library and configures the necessary args to
parse from the command line, expanding the current option of a single
file and adding support for environment variables.
Signed-off-by: Sebastian Webber <sebastian@swebber.me>
This commit adds a new function to handle notify and use it
in the SHOW HELP command, which displays the available options
in the admin console.
Also, adding Fabrízio as a co-author for all the help with the
protocol and the help to structure this PR.
Signed-off-by: Sebastian Webber <sebastian@swebber.me>
Co-authored-by: Fabrízio de Royes Mello <fabriziomello@gmail.com>
* Change idle timeout default to 10 minutes
* Revert lifo for now while we investigate connection thrashing issues
* Make queue strategy configurable
* test revert idle time out
* Add pgcat start to python test
What is wrong
Stats reported by SHOW POOLS seem to be leaking. We see lingering cl_idle , cl_waiting, and similarly for sv_idle , sv_active. We confirmed that these are reporting issues not actual lingering clients.
This behavior is readily reproducible by running
while true; do
psql "postgres://sharding_user:sharding_user@localhost:6432/sharded_db" -c "SELECT 1" > /dev/null 2>&1 &
done
Why it happens
I wasn't able to get to figure our the reason for the bug but my best guess is that we have race conditions when updating pool-level stats. So even though individual update operations are atomic, we perform a check then update sequence which is not protected by a guard.
https://github.com/postgresml/pgcat/blob/main/src/stats/pool.rs#L174-L179
I am also suspecting that using Relaxed ordering might allow this behavior (I changed all operations to use Ordering::SeqCst but still got lingering clients)
How to fix
Since SHOW POOLS/SHOW SERVER/SHOW CLIENTS all show the current state of the proxy (as opposed to SHOW STATS which show aggregate values), this PR refactors SHOW POOLS to have it construct the results directly from SHOW SERVER and SHOW CLIENT datasets. This reduces the complexity of stat updates and eliminates the need for having locks when updating pool stats as we only care about updating individual client/server states.
This will change the semantics of maxwait, so instead of it holding the maxwait time ever encountered by a client (connected or disconnected), it will only consider connected clients which should be okay given PgCat tends to hold on to client connections more than Pgbouncer.
* keep track of current stats and zero them after updating averages
* Try tests
* typo
* remove commented test stuff
* Avoid dividing by zero
* Fix test
* refactor, get rid of iterator. do it manually
* trigger build
* Fix
