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

Optimize the code inside AR::QueryCache middleware #32414

Merged
merged 1 commit into from Apr 4, 2018

Conversation

Projects
None yet
4 participants
@bogdan
Contributor

bogdan commented Apr 2, 2018

Summary

Reduce number of generated arrays from n+1 to 1 where n is the number of pools.
It can be important for applications that use a techniques like database sharding and can have a hundred of databases used by the same app.

In my project we use 6: 1 primary and 2 shards + a slave for each of them.

Follow up on #28869 (comment)

r? @matthewd

cc @eugeneius

activerecord/lib/active_record/query_cache.rb Outdated
end
ActiveRecord::Base.connection_handler.connection_pool_list.select do |pool|
unless pool.query_cache_enabled
pool.enable_query_cache!

This comment has been minimized.

@eugeneius

eugeneius Apr 2, 2018

Member

ConnectionPool#enable_query_cache! returns nil when there's no active connection:

connection.enable_query_cache! if active_connection?

The query cache will be enabled but the pool won't be returned, so the complete hook won't disable it... which will cause a regression of #26666 🐛

This comment has been minimized.

@eugeneius

eugeneius Apr 2, 2018

Member

It's probably clearer to find the connection pools and enable the query cache on them as separate steps:

ActiveRecord::Base.connection_handler.connection_pool_list
  .reject(&:query_cache_enabled).each(&:enable_query_cache!)

Could you update this test to assert that the query cache is disabled afterwards too?

activerecord/lib/active_record/query_cache.rb Outdated
pool.disable_query_cache! unless caching_was_enabled
end
def self.complete(pools)
pools.each {|pool| pool.disable_query_cache! }

This comment has been minimized.

@eugeneius

eugeneius Apr 4, 2018

Member

RuboCop wants a space after the opening brace here.

@rafaelfranca rafaelfranca merged commit e120343 into rails:master Apr 4, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
codeclimate All good!
Details

rafaelfranca added a commit that referenced this pull request Apr 4, 2018

Merge pull request #32414 from bogdan/query-cache-optimization
Optimize the code inside AR::QueryCache middleware
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment