Invalidate all query caches for current thread #35089
Conversation
eileencodes
force-pushed
the
fix-query-cache-for-database-switching
branch
from
5a27799
to
be4b1a4
Compare
| @@ -87,6 +87,14 @@ def uncached | |||
| # the same SQL query and repeatedly return the same result each time, silently | |||
| # undermining the randomness you were expecting. | |||
| def clear_query_cache | |||
| ActiveRecord::Base.connection_handlers.each_value do |handler| | |||
| handler.connection_pool_list.each do |pool| | |||
| pool.connections.each(&:clear_query_cache!) | |||
There was a problem hiding this comment.
I don't think we can iterate over pool.connections here without holding the pool's lock.
Regardless, we don't need to clear the query cache on all connections; we only need read-your-writes consistency for the current thread. If it's acceptable for a concurrent request in another process to see stale data, it should be acceptable for a concurrent request in another thread, too.
| pool.connections.each(&:clear_query_cache!) | |
| pool.connection.clear_query_cache! if pool.active_connection? |
There was a problem hiding this comment.
👍
Beyond it being acceptable, having one thread's wipe out every cache in the system would be not-great for hit rates / performance.
Along that line, I think I'd actually go further, and limit this to only the current-thread-owned connection in the peer pools -- the corresponding same-named pool in other handlers -- not all pools.
There was a problem hiding this comment.
Yea this clearly blows up in the tests but also @eugeneius version doesn't pass either.
I'm starting to get really frustrated with the query cache. It seems like it just wasn't built for multiple connections and we may need to rethink it sooner rather than later.
There was a problem hiding this comment.
Ok I found a fix I think and I'm not proud of it but the tests pass locally. I'm not sure what consequences to test for at this point if the tests pass on CI.
eileencodes
force-pushed
the
fix-query-cache-for-database-switching
branch
from
30c6f9b
to
cdf6aa2
Compare
eileencodes
changed the title
|
@tenderlove and I looked at this today and came up with a different solution. We've fixed the issue by adding a new method that clears the query cache for all the connections but only for the current thread. The reason the thread test was failing before with @eugeneius's suggested change was because when we call |
This change ensures that all query cahces are cleared across all connections per handler for the current thread so if you write on one connection the read will have the query cache cleared.
eileencodes
force-pushed
the
fix-query-cache-for-database-switching
branch
from
cdf6aa2
to
183c0eb
Compare
This change ensures that all query caches are cleared across connection
handlers and pools so that if you write on one connection the read
connection will have the update that occurred.
cc/ @kamipo as followup to your concerns in #35073
cc/ @tenderlove