-
Notifications
You must be signed in to change notification settings - Fork 21.7k
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
Enable query cache on all connection pools #28869
Conversation
Since the query cache no longer eagerly checks out a connection, we can enable it on all connection pools at the start of every request, and it will only take effect for requests that actually use those pools.
f1e2a6f
to
11e32c1
Compare
Fixes #17921. |
This is great, thank you! Sorry it fell off my list for so long. |
No problem! Thanks for getting to it 😊 |
Don't we need to backport this? |
@rafaelfranca I don't think so; it's a new feature. We've never previously applied query caching to secondary connections. |
|
||
[caching_pool, caching_was_enabled] | ||
[pool, caching_was_enabled] | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can reduce the number of generated arrays by passing only the pools where caching was enabled into the complete
hook:
def self.run
ActiveRecord::Base.connection_handler.connection_pool_list.select do |pool|
unless pool.query_cache_enabled
p.enable_query_cache!
end
end
end
def self.complete(pools)
pools.each {|pool| pool.disable_query_cache! }
...
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right! That approach makes the code a bit clearer too.
As you already wrote the patch... do you want to open a pull request? 😄
Hello! I have a question about using query cache with slaves databases which are read-only. My case (consider this happens inside http request/response cycle):
As for me it will be nice to tell some connection pools not to use query cache. |
As of #35089, released in Rails 6.0, writes invalidate the query cache for all connections. |
@eugeneius thanks a lot! |
Followup to #26978.
Since the query cache no longer eagerly checks out a connection, we can enable it on all connection pools at the start of every request, and it will only take effect for requests that actually use those pools.
r? @matthewd