Skip to content

Enable query cache on all connection pools#28869

Merged
matthewd merged 1 commit into
rails:masterfrom
eugeneius:query_cache_all_pools
Nov 17, 2017
Merged

Enable query cache on all connection pools#28869
matthewd merged 1 commit into
rails:masterfrom
eugeneius:query_cache_all_pools

Conversation

@eugeneius

Copy link
Copy Markdown
Member

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

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.
@eugeneius

Copy link
Copy Markdown
Member Author

Fixes #17921.

@matthewd

Copy link
Copy Markdown
Member

This is great, thank you! Sorry it fell off my list for so long.

@matthewd matthewd merged commit cd3c0fa into rails:master Nov 17, 2017
@eugeneius

Copy link
Copy Markdown
Member Author

No problem! Thanks for getting to it 😊

@rafaelfranca

Copy link
Copy Markdown
Member

Don't we need to backport this?

@matthewd

Copy link
Copy Markdown
Member

@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

@bogdan bogdan Mar 29, 2018

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Member Author

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? 😄

@mainameiz

Copy link
Copy Markdown

Hello! I have a question about using query cache with slaves databases which are read-only.
I'm using active_record_slave to read some records from slave database. It uses special abstract model which connects to slave database using establish_connection and all reads go through this model by default.

My case (consider this happens inside http request/response cycle):

  1. fetch some record - it will be read from slave and this query will be cached
  2. delete that record - this query will go through AR::Base.connection_pool to master database and this will clear the cache for this connection pool but not for connection pool of the model for slave database.
  3. fetch the same record from step №1 using the same query - this will return the record from cache, because all reads go to slave by default.

AR.uncached cannot be used because it works only for AR::Base.connection_pool.

As for me it will be nice to tell some connection pools not to use query cache.
I will be glad to hear your thoughts.

@eugeneius

Copy link
Copy Markdown
Member Author

As of #35089, released in Rails 6.0, writes invalidate the query cache for all connections.

@mainameiz

Copy link
Copy Markdown

@eugeneius thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants