perf(db_handler): use LRU eviction for backend prepared statements#980
Conversation
When the backend prepared statement set hits the 200-statement limit, eviction picks 40 entries at random and closes them. That can drop hot statements as readily as cold ones, so the next Bind pays for a fresh Parse round-trip. Replaces the MapSet with a tiny LRU storage that drops the least recently used statements first. Recency is bumped on Parse and on Bind for statements already known to the backend. No default behavior change — still gated by the named_prepared_statements feature flag.
There was a problem hiding this comment.
Thanks for the PR, @MorganaFuture, it looks great conceptually and the implementation is sound! I wanted to implement something like this but didn't get around to do it.
I'd like to suggest an architectural change, though: I'd like to make the db handler prepared statement storage backend pluggable. Why: we haven't really adopted this yet in production, and it would be nice to have both implementations at the same time so we can compare them in different scenarios. We could select it based on a feature flag, and eventually settle in a default. This also sets us up for a future in case we want to try a third storage.
WDYT?
Splits BackendStorage into a behaviour with LRU and Random impls, selectable per-tenant via the "backend_prepared_statements_storage" feature flag. Default stays Random.
Great suggestion. BackendStorage is now a behaviour with two impls LRU and Random. Random is default. |
v0idpwn
left a comment
v0idpwn
left a comment
There was a problem hiding this comment.
Looks great, just one small note!
|
Regarding #978, we will do the bump separately as it requires appups to ship these version upgrades with zero downtime in production. These appups can be a bit nasty to write and test (and potentially not possible), so we will take our time to explore the version bumping in the next couple days. |
Random doesn't track recency, so "oldest" misnamed the callback. "evict" describes the operation regardless of strategy.
The defensive 2-tuple clause in handle_auth_pkts moved from line 818 to 822 after the backend prepared-statement storage changes, but the .dialyzer_ignore.exs entry still pinned 818. That left the real pattern_match warning unignored and the 818 skip unnecessary, failing the Dialyze CI job (Skipped: 26, Unnecessary Skips: 1).
|
@v0idpwn So let's merge? |
|
Just wanted to thank you for this one @MorganaFuture, sorry for how long it took to merge! 💚 |
2 participants
What kind of change does this PR introduce?
Performance improvement.
What is the current behavior?
When the per-backend prepared statement set hits the 200-statement limit, eviction picks 40 entries at random (
Enum.take_random) and sendsClosefor them. Hot statements get evicted with the same probability as cold ones,so under churn we can drop a frequently used statement and pay for a fresh
Parseround-trip on the nextBind. Relates to #69 / #239.What is the new behavior?
Replaces the
MapSet-backed set with a small LRU storage (Supavisor.Protocol.PreparedStatements.BackendStorage). Recency is bumped onParseand onBindfor statements already known to the backend. Eviction now drops the least recently used statements first, so hot ones stay around.No default behavior change still gated by the
named_prepared_statementsfeature flag. Telemetry (prepared_statements_evicted) is unchanged. Covered by unit tests (basic API + edge cases) and aStreamDataproperty test that checks the LRU ordering invariant across arbitraryput/touch/deletesequences.