Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: configurable caching for postgres with support for tracing #5082

Merged

Conversation

jacek-prisma
Copy link
Contributor

@jacek-prisma jacek-prisma commented Dec 10, 2024

@jacek-prisma jacek-prisma requested a review from aqrln December 10, 2024 18:47
Copy link

codspeed-hq bot commented Dec 10, 2024

CodSpeed Performance Report

Merging #5082 will not alter performance

Comparing jacek-prisma:feat/caching-for-traced-postgres (52c5e61) with main (81600e1)

Summary

✅ 11 untouched benchmarks

@jacek-prisma jacek-prisma added this to the 6.1.0 milestone Dec 11, 2024
@jacek-prisma jacek-prisma changed the title feat: configurable caching with support for tracing feat: configurable caching for postgres with support for tracing Dec 11, 2024
@jacek-prisma jacek-prisma marked this pull request as ready for review December 11, 2024 13:01
@jacek-prisma jacek-prisma requested a review from a team as a code owner December 11, 2024 13:01
Comment on lines +137 to 154
QuaintManager::Postgres {
url,
tls_manager,
is_tracing_enabled: false,
} => {
use crate::connector::PostgreSqlWithDefaultCache;
Ok(Box::new(PostgreSqlWithDefaultCache::new(url.clone(), tls_manager).await?) as Self::Connection)
}

#[cfg(feature = "postgresql-native")]
QuaintManager::Postgres {
url,
tls_manager,
is_tracing_enabled: true,
} => {
use crate::connector::PostgreSqlWithTracingCache;
Ok(Box::new(PostgreSqlWithTracingCache::new(url.clone(), tls_manager).await?) as Self::Connection)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary:

  • when is_tracing_enabled, we use the tracing cache (crate::connector::PostgreSqlWithTracingCache)
  • when !is_tracing_enabled, we use the default cache (crate::connector::PostgreSqlWithDefaultCache)

#[tokio::test]
async fn tracing_lru_cache_reuses_queries_within_capacity() {
run_with_client(|client| async move {
let cache = TracingLruCache::with_capacity(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's expand this test case with a higher capacity, e.g., 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

#[tokio::test]
async fn prepared_statement_lru_cache_reuses_statements_within_capacity() {
run_with_client(|client| async move {
let cache = PreparedStatementLruCache::with_capacity(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's expand this test case with a higher capacity, e.g., 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

#[tokio::test]
async fn prepared_statement_lru_cache_reuses_queries_within_capacity() {
run_with_client(|client| async move {
let cache = PreparedStatementLruCache::with_capacity(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's expand this test case with a higher capacity, e.g., 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

match self.cache.get(sql, types).await {
Some(statement) => Ok(statement),
None => {
let stmt = client.prepare_typed(sql, types).await?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, here's why we need Result in the function signature's return type

Comment on lines +16 to +30
/// Types that can be used as a cache for prepared queries and statements.
#[async_trait]
pub trait QueryCache: From<CacheSettings> + Send + Sync {
/// The type that is returned when a prepared query is requested from the cache.
type Query<'a>: PreparedQuery;

/// Retrieve a prepared query.
async fn get_query<'a>(&self, client: &Client, sql: &'a str, types: &[Type]) -> Result<Self::Query<'a>, Error>;

/// Retrieve a prepared statement.
///
/// This is useful in scenarios that require direct access to a prepared statement,
/// e.g. describing a query.
async fn get_statement(&self, client: &Client, sql: &str, types: &[Type]) -> Result<Statement, Error>;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! This is so much better than having two separate caches.

Copy link
Contributor

@jkomyno jkomyno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! I've left a nit. Please wait for @aqrln's review as well before merging

quaint/src/connector/metrics.rs Show resolved Hide resolved
quaint/src/connector/postgres/native/cache.rs Outdated Show resolved Hide resolved
quaint/src/connector/postgres/native/cache.rs Show resolved Hide resolved
quaint/src/connector/postgres/native/cache.rs Outdated Show resolved Hide resolved
quaint/src/connector/postgres/native/cache.rs Outdated Show resolved Hide resolved
quaint/src/connector/postgres/native/cache.rs Outdated Show resolved Hide resolved
Copy link
Member

@aqrln aqrln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks amazing, and the client tests pass with pnpm --filter @prisma/client test:functional:code --provider postgresql --preview-features tracing locally.

The lack of execute_typed in tokio_postgres is a bummer but I think it only affects createMany.

@jacek-prisma jacek-prisma merged commit 99168c5 into prisma:main Dec 13, 2024
368 checks passed
@jacek-prisma jacek-prisma deleted the feat/caching-for-traced-postgres branch December 13, 2024 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants