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

Invalidate all query caches for current thread #35089

Merged

Conversation

Projects
None yet
3 participants
@eileencodes
Copy link
Member

commented Jan 29, 2019

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

@eileencodes eileencodes added this to the 6.0.0 milestone Jan 29, 2019

@eileencodes eileencodes force-pushed the eileencodes:fix-query-cache-for-database-switching branch from 5a27799 to be4b1a4 Jan 29, 2019

@@ -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!)

This comment has been minimized.

Copy link
@eugeneius

eugeneius Jan 29, 2019

Member

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.

Suggested change
pool.connections.each(&:clear_query_cache!)
pool.connection.clear_query_cache! if pool.active_connection?

This comment has been minimized.

Copy link
@matthewd

matthewd Jan 30, 2019

Member

👍

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.

This comment has been minimized.

Copy link
@eileencodes

eileencodes Jan 30, 2019

Author Member

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.

This comment has been minimized.

Copy link
@eileencodes

eileencodes Jan 30, 2019

Author Member

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 eileencodes force-pushed the eileencodes:fix-query-cache-for-database-switching branch from 30c6f9b to cdf6aa2 Feb 1, 2019

@eileencodes eileencodes changed the title Invalidate all query caches when anything writes Invalidate all query caches for current thread Feb 1, 2019

@eileencodes

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2019

@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 clear_active_connections! that calls disable_query_cache which clears all the caches for all the connections. The previous change broke that, but we made a new change that's better and more accurate.

@eileencodes eileencodes self-assigned this Feb 1, 2019

Invalidate query cache for all connections in the current thread
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 eileencodes force-pushed the eileencodes:fix-query-cache-for-database-switching branch from cdf6aa2 to 183c0eb Feb 1, 2019

@eileencodes eileencodes merged commit 7d85d38 into rails:master Feb 4, 2019

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@eileencodes eileencodes deleted the eileencodes:fix-query-cache-for-database-switching branch Feb 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.