Selecting custom caching strategy #4161
Conversation
fraillt
force-pushed
the
choose-caching-strategy
branch
from
97afa88
to
34ef18c
Compare
|
Thanks for opening this PR and working on this issue. That written: I'm not fond of the chosen approach. I personally do not see why rebuilding the statement isn't an option. Yes it might fail for PostgreSQL in a transaction, but on the other hand there is so many other things that might file and render a transaction unusable that I don't feel that this really matters. If you care about such cases you must implement a transaction retry logic anyway. There are several things to consider with the proposed solution, that I personally not a fan of:
I'm sorry if that sounds like I want to dismiss your work, but I just have different ideas in mind on how to approach that problem. I personally would have preferred to discuss the design of this feature first before posting a PR, as that would likely result in much less frustration on both sides. |
|
Since this is not a huge feature, I like to start discussion from some point. Real implementations usually have some subtle nuances and you also see full scope, these things are hard to discuss up-front. One important thing, regarding transactions. YMMV, but for us transactions fail only for network issues or breaking-scheme-change issues. Breaking-scheme-change we solve by properly managing migrations in non-breaking change manner, the rest (network issues) we simply propagate to upstream, as technical problems (which might be retried later or not). Before discussing further points, I think that we need to agree on one fundamental point first:
|
|
I'm open to allow configuring the cache, but I would like to make that option connection specific rather than global. So just That written: I would prefer to have the automatic retry for statements nevertheless as at least I do want to use the statement cache whenever possible as it gives a quite large performance boost. |
|
Let me think out lout :).
To achieve that, migration process could be implemented like this:
In order to implement caching behavior at runtime, we would wrap r2d2 connection pool into our own and every time we get connection from the pool, we would I probably it's a bit more work on end user side (to wrap connection), but actually that would solve our problem :) So I guess we'll come back to you with updated revision ;) |
|
That sounds like a good summary, thanks for writing it up. I think it would be also fine to have an additional function that just empties the current query cache. |
fraillt
force-pushed
the
choose-caching-strategy
branch
2 times, most recently
from
dbe25cc
to
95084a6
Compare
|
Some comments:
Happy reviewing and waiting for your comments :) |
fraillt
force-pushed
the
choose-caching-strategy
branch
2 times, most recently
from
6235344
to
340c893
Compare
There was a problem hiding this comment.
Thanks for the update. This is now much closer to what I would expect in terms of public API. I've left a bunch of mostly minor comments. The only major thing I would like to see changed is how this feature is tested. Instead of having the tests "everywhere" I would like to focus them on only this feature inside of the strategy module.
fraillt
force-pushed
the
choose-caching-strategy
branch
from
340c893
to
3d5462d
Compare
fraillt
force-pushed
the
choose-caching-strategy
branch
from
3d5462d
to
b783552
Compare
|
I think I have addressed all your comments. |
diesel/src/pg/connection/mod.rs
Outdated
| @@ -124,7 +126,8 @@ pub(super) use self::result::PgResult; | |||
| #[allow(missing_debug_implementations)] | |||
| #[cfg(feature = "postgres")] | |||
| pub struct PgConnection { | |||
| statement_cache: StatementCache<Pg, Statement>, | |||
| /// pub(crate) for tests | |||
| pub(crate) statement_cache: StatementCache<Pg, Statement>, | |||
There was a problem hiding this comment.
That's not required anymore, right?
diesel/src/sqlite/connection/stmt.rs
Outdated
| @@ -15,7 +15,7 @@ use std::io::{stderr, Write}; | |||
| use std::os::raw as libc; | |||
| use std::ptr::{self, NonNull}; | |||
|
|
|||
| pub(super) struct Statement { | |||
| pub(crate) struct Statement { | |||
There was a problem hiding this comment.
That shouldn't be required anymore.
diesel/src/sqlite/connection/mod.rs
Outdated
| @@ -121,7 +121,8 @@ pub struct SqliteConnection { | |||
| // statement_cache needs to be before raw_connection | |||
| // otherwise we will get errors about open statements before closing the | |||
| // connection itself | |||
| statement_cache: StatementCache<Sqlite, Statement>, | |||
| // pub(crate) for tests | |||
| pub(crate) statement_cache: StatementCache<Sqlite, Statement>, | |||
There was a problem hiding this comment.
That change shouldn't be required anymore?
| connection.begin_test_transaction().unwrap(); | ||
|
|
||
| crate::sql_query( | ||
| "CREATE TABLE IF NOT EXISTS users(id INTEGER PRIMARY KEY, name TEXT NOT NULL);", |
There was a problem hiding this comment.
Can we use CREATE TEMPORARY TABLE statements here instead? That prevents possible conflicts.
|
|
||
| #[cfg(test)] | ||
| #[cfg(feature = "postgres")] | ||
| mod tests_pg { |
There was a problem hiding this comment.
Would it be possible to add another test that shows that disabling the cache has an effect?
| insertable::Insertable, | ||
| macros::table, | ||
| pg::Pg, | ||
| sql_types::{Integer, VarChar}, |
There was a problem hiding this comment.
We don't have such nested imports in diesel otherwise. I would prefer to keep the style consistent.
|
Thanks for the update. I really like the solution to test this via the instrumentation tooling. |
weiznich
added
the
run-benchmarks
|
| Report | Fri, August 30, 2024 at 08:18:19 UTC |
| Project | diesel |
| Branch | choose-caching-strategy-bda514b4 |
| Testbed | ubuntu-latest-sqlite |
Click to view all benchmark results
| Benchmark | Latency | Latency Results nanoseconds (ns) | (Δ%) | Latency Upper Boundary nanoseconds (ns) | (%) |
|---|---|---|---|
| bench_insert/diesel/1 | ✅ (view plot) | 2,811.20 (-0.39%) | 2,996.20 (93.83%) |
| bench_insert/diesel/10 | ✅ (view plot) | 6,932.90 (-0.43%) | 7,441.43 (93.17%) |
| bench_insert/diesel/100 | ✅ (view plot) | 44,792.00 (-0.54%) | 48,914.27 (91.57%) |
| bench_insert/diesel/25 | ✅ (view plot) | 13,717.00 (+0.24%) | 14,201.07 (96.59%) |
| bench_insert/diesel/50 | ✅ (view plot) | 23,766.00 (-1.49%) | 29,856.48 (79.60%) |
| bench_loading_associations_sequentially/diesel/bench_loading_associations_sequentially | ✅ (view plot) | 408,850.00 (+0.44%) | 435,362.29 (93.91%) |
| bench_medium_complex_query/diesel/1 | ✅ (view plot) | 3,323.80 (+0.40%) | 3,520.41 (94.42%) |
| bench_medium_complex_query/diesel/10 | ✅ (view plot) | 5,516.90 (+1.24%) | 6,522.28 (84.59%) |
| bench_medium_complex_query/diesel/100 | ✅ (view plot) | 28,448.00 (+0.05%) | 28,649.08 (99.30%) |
| bench_medium_complex_query/diesel/1000 | ✅ (view plot) | 249,800.00 (+0.68%) | 275,046.25 (90.82%) |
| bench_medium_complex_query/diesel/10000 | ✅ (view plot) | 2,431,600.00 (+0.77%) | 2,707,893.81 (89.80%) |
| bench_medium_complex_query/diesel_boxed/1 | ✅ (view plot) | 4,654.60 (-0.64%) | 5,164.82 (90.12%) |
| bench_medium_complex_query/diesel_boxed/10 | ✅ (view plot) | 7,005.40 (+1.34%) | 8,380.91 (83.59%) |
| bench_medium_complex_query/diesel_boxed/100 | ✅ (view plot) | 30,075.00 (+0.11%) | 30,559.07 (98.42%) |
| bench_medium_complex_query/diesel_boxed/1000 | ✅ (view plot) | 251,750.00 (+0.39%) | 266,495.60 (94.47%) |
| bench_medium_complex_query/diesel_boxed/10000 | ✅ (view plot) | 2,434,200.00 (+0.75%) | 2,704,535.99 (90.00%) |
| bench_medium_complex_query/diesel_queryable_by_name/1 | ✅ (view plot) | 18,034.00 (-1.48%) | 22,595.53 (79.81%) |
| bench_medium_complex_query/diesel_queryable_by_name/10 | ✅ (view plot) | 20,786.00 (-0.81%) | 23,649.63 (87.89%) |
| bench_medium_complex_query/diesel_queryable_by_name/100 | ✅ (view plot) | 48,428.00 (-0.64%) | 53,732.89 (90.13%) |
| bench_medium_complex_query/diesel_queryable_by_name/1000 | ✅ (view plot) | 316,290.00 (+0.55%) | 342,057.56 (92.47%) |
| bench_medium_complex_query/diesel_queryable_by_name/10000 | ✅ (view plot) | 2,928,600.00 (+0.43%) | 3,117,015.99 (93.96%) |
| bench_trivial_query/diesel/1 | ✅ (view plot) | 880.46 (+0.52%) | 948.30 (92.85%) |
| bench_trivial_query/diesel/10 | ✅ (view plot) | 2,932.90 (-0.23%) | 3,048.63 (96.20%) |
| bench_trivial_query/diesel/100 | ✅ (view plot) | 24,301.00 (-4.48%) | 43,569.23 (55.78%) |
| bench_trivial_query/diesel/1000 | ✅ (view plot) | 230,300.00 (-5.23%) | 445,114.14 (51.74%) |
| bench_trivial_query/diesel/10000 | ✅ (view plot) | 2,237,600.00 (+1.36%) | 2,685,181.07 (83.33%) |
| bench_trivial_query/diesel_boxed/1 | ✅ (view plot) | 1,500.70 (-0.28%) | 1,570.81 (95.54%) |
| bench_trivial_query/diesel_boxed/10 | ✅ (view plot) | 3,515.20 (-0.42%) | 3,763.55 (93.40%) |
| bench_trivial_query/diesel_boxed/100 | ✅ (view plot) | 24,933.00 (-3.29%) | 39,268.02 (63.49%) |
| bench_trivial_query/diesel_boxed/1000 | ✅ (view plot) | 231,890.00 (+1.71%) | 289,829.78 (80.01%) |
| bench_trivial_query/diesel_boxed/10000 | ✅ (view plot) | 2,243,600.00 (+1.43%) | 2,715,757.07 (82.61%) |
| bench_trivial_query/diesel_queryable_by_name/1 | ✅ (view plot) | 5,440.10 (-0.51%) | 5,914.84 (91.97%) |
| bench_trivial_query/diesel_queryable_by_name/10 | ✅ (view plot) | 7,953.50 (-0.52%) | 8,653.78 (91.91%) |
| bench_trivial_query/diesel_queryable_by_name/100 | ✅ (view plot) | 32,246.00 (-0.67%) | 35,929.01 (89.75%) |
| bench_trivial_query/diesel_queryable_by_name/1000 | ✅ (view plot) | 264,610.00 (-0.90%) | 305,325.85 (86.66%) |
| bench_trivial_query/diesel_queryable_by_name/10000 | ✅ (view plot) | 2,523,000.00 (-1.34%) | 3,099,948.71 (81.39%) |
Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help
|
| Report | Fri, August 30, 2024 at 08:18:09 UTC |
| Project | diesel |
| Branch | choose-caching-strategy-bda514b4 |
| Testbed | ubuntu-latest-postgres |
Click to view all benchmark results
| Benchmark | Latency | Latency Results nanoseconds (ns) | (Δ%) | Latency Upper Boundary nanoseconds (ns) | (%) |
|---|---|---|---|
| bench_insert/diesel/1 | ✅ (view plot) | 211,180.00 (-4.46%) | 377,844.68 (55.89%) |
| bench_insert/diesel/10 | ✅ (view plot) | 274,750.00 (+1.27%) | 326,061.71 (84.26%) |
| bench_insert/diesel/100 | ✅ (view plot) | 571,750.00 (-2.76%) | 846,201.88 (67.57%) |
| bench_insert/diesel/25 | ✅ (view plot) | 332,020.00 (+3.17%) | 484,093.30 (68.59%) |
| bench_insert/diesel/50 | ✅ (view plot) | 429,100.00 (+1.41%) | 517,722.54 (82.88%) |
| bench_loading_associations_sequentially/diesel/bench_loading_associations_sequentially | ✅ (view plot) | 6,110,700.00 (-1.18%) | 7,342,312.32 (83.23%) |
| bench_medium_complex_query/diesel/1 | ✅ (view plot) | 88,393.00 (+3.29%) | 130,358.38 (67.81%) |
| bench_medium_complex_query/diesel/10 | ✅ (view plot) | 93,782.00 (+2.16%) | 123,332.78 (76.04%) |
| bench_medium_complex_query/diesel/100 | ✅ (view plot) | 118,140.00 (-2.91%) | 177,946.69 (66.39%) |
| bench_medium_complex_query/diesel/1000 | ✅ (view plot) | 430,520.00 (+0.10%) | 436,999.13 (98.52%) |
| bench_medium_complex_query/diesel/10000 | ✅ (view plot) | 3,322,500.00 (+0.00%) | 3,323,989.45 (99.96%) |
| bench_medium_complex_query/diesel_boxed/1 | ✅ (view plot) | 85,505.00 (-1.01%) | 100,313.07 (85.24%) |
| bench_medium_complex_query/diesel_boxed/10 | ✅ (view plot) | 94,947.00 (+0.61%) | 103,496.47 (91.74%) |
| bench_medium_complex_query/diesel_boxed/100 | ✅ (view plot) | 123,390.00 (-1.28%) | 150,505.74 (81.98%) |
| bench_medium_complex_query/diesel_boxed/1000 | ✅ (view plot) | 433,680.00 (-1.91%) | 576,607.85 (75.21%) |
| bench_medium_complex_query/diesel_boxed/10000 | ✅ (view plot) | 3,393,800.00 (-0.58%) | 3,729,156.72 (91.01%) |
| bench_medium_complex_query/diesel_queryable_by_name/1 | ✅ (view plot) | 237,790.00 (-0.46%) | 256,205.05 (92.81%) |
| bench_medium_complex_query/diesel_queryable_by_name/10 | ✅ (view plot) | 248,830.00 (+1.09%) | 288,970.80 (86.11%) |
| bench_medium_complex_query/diesel_queryable_by_name/100 | ✅ (view plot) | 281,660.00 (-0.53%) | 307,170.76 (91.69%) |
| bench_medium_complex_query/diesel_queryable_by_name/1000 | ✅ (view plot) | 618,060.00 (-0.48%) | 668,405.74 (92.47%) |
| bench_medium_complex_query/diesel_queryable_by_name/10000 | ✅ (view plot) | 3,727,400.00 (-0.50%) | 4,041,638.53 (92.22%) |
| bench_trivial_query/diesel/1 | ✅ (view plot) | 64,073.00 (-0.29%) | 67,257.62 (95.27%) |
| bench_trivial_query/diesel/10 | ✅ (view plot) | 70,478.00 (+1.53%) | 86,303.45 (81.66%) |
| bench_trivial_query/diesel/100 | ✅ (view plot) | 100,050.00 (+0.80%) | 111,913.50 (89.40%) |
| bench_trivial_query/diesel/1000 | ✅ (view plot) | 402,580.00 (-0.72%) | 451,658.65 (89.13%) |
| bench_trivial_query/diesel/10000 | ✅ (view plot) | 3,494,700.00 (+0.63%) | 3,820,890.53 (91.46%) |
| bench_trivial_query/diesel_boxed/1 | ✅ (view plot) | 65,181.00 (-0.04%) | 65,586.47 (99.38%) |
| bench_trivial_query/diesel_boxed/10 | ✅ (view plot) | 69,756.00 (-0.25%) | 72,653.41 (96.01%) |
| bench_trivial_query/diesel_boxed/100 | ✅ (view plot) | 101,630.00 (+1.52%) | 124,344.18 (81.73%) |
| bench_trivial_query/diesel_boxed/1000 | ✅ (view plot) | 393,930.00 (-0.06%) | 397,646.80 (99.07%) |
| bench_trivial_query/diesel_boxed/10000 | ✅ (view plot) | 3,462,000.00 (+0.19%) | 3,561,793.45 (97.20%) |
| bench_trivial_query/diesel_queryable_by_name/1 | ✅ (view plot) | 158,130.00 (+1.49%) | 192,610.87 (82.10%) |
| bench_trivial_query/diesel_queryable_by_name/10 | ✅ (view plot) | 156,310.00 (-0.74%) | 176,076.62 (88.77%) |
| bench_trivial_query/diesel_queryable_by_name/100 | ✅ (view plot) | 190,120.00 (+0.74%) | 210,897.89 (90.15%) |
| bench_trivial_query/diesel_queryable_by_name/1000 | ✅ (view plot) | 497,500.00 (+0.19%) | 511,277.45 (97.31%) |
| bench_trivial_query/diesel_queryable_by_name/10000 | ✅ (view plot) | 3,704,900.00 (+0.61%) | 4,040,027.26 (91.70%) |
Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help
|
| Report | Fri, August 30, 2024 at 08:18:06 UTC |
| Project | diesel |
| Branch | choose-caching-strategy-bda514b4 |
| Testbed | ubuntu-latest-mysql |
Click to view all benchmark results
| Benchmark | Latency | Latency Results nanoseconds (ns) | (Δ%) | Latency Upper Boundary nanoseconds (ns) | (%) |
|---|---|---|---|
| bench_insert/diesel/1 | ✅ (view plot) | 827,590.00 (+1.92%) | 1,059,349.12 (78.12%) |
| bench_insert/diesel/10 | ✅ (view plot) | 947,310.00 (+1.73%) | 1,186,590.86 (79.83%) |
| bench_insert/diesel/100 | ✅ (view plot) | 1,819,200.00 (+0.38%) | 1,921,227.63 (94.69%) |
| bench_insert/diesel/25 | ✅ (view plot) | 1,124,000.00 (+3.02%) | 1,614,030.53 (69.64%) |
| bench_insert/diesel/50 | ✅ (view plot) | 1,366,500.00 (+2.21%) | 1,807,378.53 (75.61%) |
| bench_loading_associations_sequentially/diesel/bench_loading_associations_sequentially | ✅ (view plot) | 16,604,000.00 (+0.79%) | 18,547,738.10 (89.52%) |
| bench_medium_complex_query/diesel/1 | ✅ (view plot) | 112,310.00 (+0.56%) | 121,544.62 (92.40%) |
| bench_medium_complex_query/diesel/10 | ✅ (view plot) | 150,380.00 (+0.96%) | 171,604.73 (87.63%) |
| bench_medium_complex_query/diesel/100 | ✅ (view plot) | 215,960.00 (+0.41%) | 228,992.73 (94.31%) |
| bench_medium_complex_query/diesel/1000 | ✅ (view plot) | 857,240.00 (+0.14%) | 875,187.93 (97.95%) |
| bench_medium_complex_query/diesel/10000 | ✅ (view plot) | 7,543,200.00 (+0.48%) | 8,080,148.34 (93.35%) |
| bench_medium_complex_query/diesel_boxed/1 | ✅ (view plot) | 115,880.00 (+1.38%) | 139,413.38 (83.12%) |
| bench_medium_complex_query/diesel_boxed/10 | ✅ (view plot) | 154,070.00 (+0.98%) | 176,337.34 (87.37%) |
| bench_medium_complex_query/diesel_boxed/100 | ✅ (view plot) | 218,200.00 (+0.44%) | 232,573.24 (93.82%) |
| bench_medium_complex_query/diesel_boxed/1000 | ✅ (view plot) | 861,890.00 (+0.31%) | 901,732.91 (95.58%) |
| bench_medium_complex_query/diesel_boxed/10000 | ✅ (view plot) | 7,528,300.00 (+0.31%) | 7,870,874.53 (95.65%) |
| bench_medium_complex_query/diesel_queryable_by_name/1 | ✅ (view plot) | 191,740.00 (-0.49%) | 207,705.34 (92.31%) |
| bench_medium_complex_query/diesel_queryable_by_name/10 | ✅ (view plot) | 232,070.00 (+1.05%) | 267,965.85 (86.60%) |
| bench_medium_complex_query/diesel_queryable_by_name/100 | ✅ (view plot) | 308,470.00 (+1.99%) | 398,284.11 (77.45%) |
| bench_medium_complex_query/diesel_queryable_by_name/1000 | ✅ (view plot) | 980,850.00 (+0.10%) | 995,148.76 (98.56%) |
| bench_medium_complex_query/diesel_queryable_by_name/10000 | ✅ (view plot) | 8,163,800.00 (+1.13%) | 9,528,140.31 (85.68%) |
| bench_trivial_query/diesel/1 | ✅ (view plot) | 66,284.00 (-0.03%) | 66,596.55 (99.53%) |
| bench_trivial_query/diesel/10 | ✅ (view plot) | 74,996.00 (+0.78%) | 83,619.94 (89.69%) |
| bench_trivial_query/diesel/100 | ✅ (view plot) | 132,590.00 (-0.85%) | 151,765.31 (87.37%) |
| bench_trivial_query/diesel/1000 | ✅ (view plot) | 684,270.00 (-0.65%) | 759,788.62 (90.06%) |
| bench_trivial_query/diesel/10000 | ✅ (view plot) | 6,139,800.00 (-0.36%) | 6,513,169.44 (94.27%) |
| bench_trivial_query/diesel_boxed/1 | ✅ (view plot) | 67,274.00 (-0.96%) | 78,238.56 (85.99%) |
| bench_trivial_query/diesel_boxed/10 | ✅ (view plot) | 75,812.00 (+0.76%) | 84,309.34 (89.92%) |
| bench_trivial_query/diesel_boxed/100 | ✅ (view plot) | 135,900.00 (+0.10%) | 137,910.76 (98.54%) |
| bench_trivial_query/diesel_boxed/1000 | ✅ (view plot) | 686,120.00 (-0.56%) | 751,079.52 (91.35%) |
| bench_trivial_query/diesel_boxed/10000 | ✅ (view plot) | 6,144,200.00 (-0.37%) | 6,530,240.35 (94.09%) |
| bench_trivial_query/diesel_queryable_by_name/1 | ✅ (view plot) | 128,230.00 (-0.93%) | 148,503.45 (86.35%) |
| bench_trivial_query/diesel_queryable_by_name/10 | ✅ (view plot) | 138,690.00 (+2.01%) | 179,352.11 (77.33%) |
| bench_trivial_query/diesel_queryable_by_name/100 | ✅ (view plot) | 197,160.00 (-1.35%) | 242,606.33 (81.27%) |
| bench_trivial_query/diesel_queryable_by_name/1000 | ✅ (view plot) | 761,240.00 (+0.40%) | 806,817.31 (94.35%) |
| bench_trivial_query/diesel_queryable_by_name/10000 | ✅ (view plot) | 6,424,200.00 (+0.88%) | 7,257,549.78 (88.52%) |
Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help
|
Hi, @weiznich |
Hello,
Following #4058 discussion (about statement caches), I want to propose a possible solution.
Since rebuilding statement cache on error is not an option (at least for PostgreSQL). The only convenient option that I see, is to give control to end user, to decide how he wants to use statement cache (of course user can implement it's own connection type, but that is not convenient option, as most likely user also needs to implement r2d2 traits as well).
There's the core idea:
StatementCacheStrategytrait. To implementations are already provided:WithCacheStrategy(default),WithoutCacheStrategy.set_cache_strategyglobally per connection type (because traits are generic over DB and Statement).i-implement-a-third-party-backend-and-opt-into-breaking-changesfeature flags).Notable changes:
ConnectionStatementCachethat is implemented by all connections which will use statement cache. This trait has two purposes: 1) exposes functionality for end user to set custom caching strategy, 2)StatementCacheuse this, to actually get current caching strategy. Another option was to implement this directly onConnection, but that would require adding newtype Statementassociated type parameter, and I didn't wanted to deal withMultiConnectionmacro.lenfromStatementCache. It was used by PostgreSQL implementation to generate statement cache names, but this might easily break if user provides it's own implementation for statement cache. Instead,StatementCacheimplementation provide acache_counter: Option<u64>that is incremented every time if statement will be cached, or None otherwise.IntrospectCachingStrategyfor tests only (behindcfg(test)), that wraps actualStatementCacheStrategyand records outcome. This is used in tests to check if tests related to statement caching behave correctly. (previouslylenfield fromStatementCachewas utilized, but this is more precise).statement_cacheis public (instead of being behindi-implement-a-third-party-backend-and-opt-into-breaking-changes).