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

Memory Leak when using prepare_cached #281

Closed
burkematthew opened this issue Nov 7, 2023 · 6 comments
Closed

Memory Leak when using prepare_cached #281

burkematthew opened this issue Nov 7, 2023 · 6 comments
Labels
A-postgres Area: PostgreSQL support / deadpool-postgres invalid The issue is invalid and has been raised in error.

Comments

@burkematthew
Copy link

burkematthew commented Nov 7, 2023

Hi,

First, thank you for the excellent crate! We've reliably used this for all of our Rust projects.

A few weeks ago, one of our applications saw a complete drain of our freeable memory. After digging in, we found that the problem subsided after creating a background job using tokio::spawn that would take our Database Pools and clear out all the Statement Caches. After digging even further, we found that when we switched from preparing our statements with prepare_cached to prepare, our memory stabilized and no longer showed the same leaking behavior.

I have included a screenshot from DataDog to show the difference in behavior.

Screenshot 2023-11-07 at 4 11 50 PM

On the screenshot, when you see the memory jump up, we had an application restart so that we could recover the used memory. If we had let it continue, the memory would have bottomed out.

I'm looking for a couple of things:

  1. I'd like to have a better understanding of the differences between using prepare_cached and prepare. Both seem to indicate that they're generating Prepared Statements, so I'm not quite sure why you'd use one over the other.
  2. Raise awareness of our observed memory leak and make sure there isn't something under the hood

Dependencies used:

  • deadpool v0.10.0
  • deadpool-postgres v0.11.0
  • tokio-postgres v0.7.7
  • Postgres v15 (although we also experienced this on Postgres v14)

Also note that we experienced this problem before updating to deadpool v0.10. We made the update last week in seeing if this update addressed the issue, to no avail.

If there is any more background or information I can provide to help, please let me know.

Thanks!

@bikeshedder bikeshedder added the A-postgres Area: PostgreSQL support / deadpool-postgres label Nov 9, 2023
@bikeshedder
Copy link
Owner

A prepared statement uses memory on both the client and the database itself. prepare_cached keeps a prepared statement around even after dropping the connection and getting it back from the connection pool. Using the statement cache can offer a significant performance improvement as it saves at least a database round trip and all the processing time needed by PostgreSQL to perform the actual "prepare" operation.

https://www.postgresql.org/docs/current/sql-prepare.html

(...) A prepared statement is a server-side object that can be used to optimize performance. When the PREPARE statement is executed, the specified statement is parsed, analyzed, and rewritten. When an EXECUTE command is subsequently issued, the prepared statement is planned and executed. This division of labor avoids repetitive parse analysis work, while allowing the execution plan to depend on the specific parameter values supplied. (...) Prepared statements only last for the duration of the current database session. (...) a single prepared statement cannot be used by multiple simultaneous database clients; (...)

The built-in statement cache of deadpool-postgres caches prepared statements as long as the connection is alive. As of now there is no LRU or other clean-up logic to remove any statements or unprepare them as long as the connection is around.

prepare_cached should only be used for static queries. When combining prepare_cached with some kind of query builder or even worse one that depends on user input it is bound to consume a lot of memory.

@bikeshedder
Copy link
Owner

I just opened an issue regarding the addition of an automatic cache clean-up:

I'd be very interested in your insights.

@burkematthew
Copy link
Author

@bikeshedder thank you for the information and the proposed enhancement. I'll review and let you know if I have any thoughts\comments!

@burkematthew
Copy link
Author

I want to hone in one specific sentence from your response to make sure I understand correctly.

prepare_cached keeps a prepared statement around even after dropping the connection and getting it back from the connection pool.

When you say "after dropping the connection", do you mean when the connection is closed altogether? Or do you simply mean when it's returned to the database Pool as an idle connection for other sessions to use? My understanding is that, when using a Recycling Mode other than something like Clean, when done with a connection, it's returned to the Pool so that it can be recycled by another process. The cache associated with that connection wouldn't necessarily be cleared at that point, which means the previously prepared statement would already be planned and available to use?

@bikeshedder
Copy link
Owner

The statement cache does not get cleared regardless of the recycling method.

https://docs.rs/deadpool-postgres/latest/deadpool_postgres/enum.RecyclingMethod.html#variant.Clean

This is similar to calling DISCARD ALL. but doesn’t call DEALLOCATE ALL and DISCARD PLAN, so that the statement cache is not rendered ineffective.

When you drop the connection (aka return it to the pool) the statement cache is kept for the next user of the connection.

The cache associated with that connection wouldn't necessarily be cleared at that point, which means the previously prepared statement would already be planned and available to use?

That's correct. Unless you explicitly call the clear method on StatementCache or StatementCaches the statements stay in the cache forever.

@bikeshedder bikeshedder added the invalid The issue is invalid and has been raised in error. label Dec 14, 2023
@bikeshedder
Copy link
Owner

I'm closing this as invalid. There is no memory leak in deadpool-postgres.

Issue #282 tracks the feature of automatically removing unused statements from the cache.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-postgres Area: PostgreSQL support / deadpool-postgres invalid The issue is invalid and has been raised in error.
Projects
None yet
Development

No branches or pull requests

2 participants