Add global query timeout to SELECT queries#4272
Conversation
|
Warning Gemini is experiencing higher than usual traffic and was unable to create the review. Please try again in a few hours by commenting |
|
WDYT about setting this within the connection instead? EDIT: this caps the inserts and we don't want that, but we can apply that to the read-only replica pool and then it's fine |
I was writing the same comment and only now noticed it. I think this PR is overkill. |
|
A few things about the PR description vs the actual diff: The description is out of date. It describes an application-level timeout approach (extension traits
Described changes not in the diff:
Undescribed changes in the diff:
|
MartinquaXD
left a comment
There was a problem hiding this comment.
Nice! That's a lot simpler and less error prone than the original implementation of this idea.
Please do update the PR description to reflect the new approach though as @squadgazzz already suggested.
Co-authored-by: Martin Magnus <martin.beckmann@protonmail.com>
Description
Following the issues we faced with RDS, and the fact that SQLx does not provide global timeouts, this PR uses PostgreSQL's native
statement_timeoutto limit the duration of read queries on the orderbook's read replica.Instead of wrapping every query on the client side, the timeout is set once per connection via the pool's
after_connecthook. This means every query issued through that pool is automatically subject to the configured timeout — no per-call changes needed.The timeout is only applied to the orderbook read replica pool (created with
Postgres::try_new_with_timeout). Write pools and other services are unaffected.Changes
crates/orderbook/src/database/mod.rs— Addedstatement_timeouttoConfigand a newPostgres::try_new_with_timeoutconstructor that setsstatement_timeouton every connection viaafter_connect. Includes a postgres integration test usingpg_sleep.crates/orderbook/src/run.rs— The read replica pool now usestry_new_with_timeout.crates/configs/src/database.rs— Addedstatement_timeoutfield toDatabasePoolConfig(defaults to30s, deserialized viahumantime_serde).crates/shared/src/arguments.rs— Added--statement-timeoutCLI flag (used by refunder).crates/database/src/trades.rs— Changedtrades()from returning aBoxStreamtofetch_all, since streaming doesn't play well with connection-level timeouts.crates/database/src/*.rsfiles and removal of an unrelated stale config section in the playground autopilot config.How to test
Run the integration test against a local postgres (
docker compose up -dfirst):