Coalesce concurrent statement preparation - #498
Merged
Conversation
|
For whatever it's worth: this LGTM, and with much better test coverage than I achieved in #422. |
Collaborator
Author
|
This is my first AI assisted code change of Deadpool. While writing the tests for it the Agent even found a real resource leak in tokio-postgres: I'm just updating the CHANGELOG and will merge this shortly. |
Rework the statement cache so that many tasks racing to prepare the same statement no longer each send a redundant, expensive `PREPARE` to the database. Each cache entry is now a `tokio::sync::OnceCell`: the first caller runs the preparation while the others wait for it, and if that caller fails or is cancelled, one of the waiters takes over. A failed preparation leaves the cell uninitialized, so the next caller simply retries. The cells are held behind an `Arc`, which matters for eviction. A preparation in flight keeps working on its own cell, so a `clear()` or `remove()` landing mid-flight detaches that cell from the map instead of being silently undone once the preparation completes. Resurrecting an entry that way would defeat the very purpose of `StatementCaches::remove()`, which exists to drop statements that went stale. The map lives in a `StatementCacheInner<V>`, with `StatementCache` as a thin `StatementCacheInner<Statement>` wrapper. The value type is generic purely as a test seam, not as a claim to reusability: `Statement` has no public constructor, so without it every test would need a live database. With it the interesting behaviour is covered by unit tests — coalescing to a single preparation under 128 concurrent tasks, eviction during preparation, size accounting against uninitialized cells, borrowed/owned key matching, and clear. Based on the approach in #422 by @tgeoghegan. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bikeshedder
force-pushed
the
testable-statement-cache
branch
from
August 4, 2026 20:10
55f113d to
6d38f06
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rework the statement cache so that many tasks racing to prepare the same statement no longer each send a redundant, expensive
PREPAREto the database. Each cache entry is now atokio::sync::OnceCell: the first caller runs the preparation while the others wait for it, and if that caller fails or is cancelled, one of the waiters takes over. A failed preparation leaves the cell uninitialized, so the next caller simply retries.The cells are held behind an
Arc, which matters for eviction. A preparation in flight keeps working on its own cell, so aclear()orremove()landing mid-flight detaches that cell from the map instead of being silently undone once the preparation completes. Resurrecting an entry that way would defeat the very purpose ofStatementCaches::remove(), which exists to drop statements that went stale.The map lives in a
StatementCacheInner<V>, withStatementCacheas a thinStatementCacheInner<Statement>wrapper. The value type is generic purely as a test seam, not as a claim to reusability:Statementhas no public constructor, so without it every test would need a live database. With it the interesting behaviour is covered by unit tests — coalescing to a single preparation under 128 concurrent tasks, eviction during preparation, size accounting against uninitialized cells, borrowed/owned key matching, and clear.Based on the approach in #422 by @tgeoghegan.