Clear query cache during checkin, instead of an execution callback #26909

Merged
merged 1 commit into from Nov 6, 2016

Projects

None yet

4 participants

@matthewd
Member

It doesn't make sense for the query cache to persist while a connection moves through the pool and is assigned to a new thread.

Fixes #26666

cc @sj26

(based on sj26@ffa26c9)

@matthewd matthewd Clear query cache during checkin, instead of an execution callback
It doesn't make sense for the query cache to persist while a connection
moves through the pool and is assigned to a new thread.

[Samuel Cochran & Matthew Draper]
09b6cc2
@matthewd matthewd self-assigned this Oct 27, 2016
@matthewd
Member

This follows on from fa7efca, but unlike that change, does not seem suitable for backport.

@@ -4,6 +4,7 @@ module QueryCache
class << self
def included(base) #:nodoc:
dirties_query_cache base, :insert, :update, :delete, :rollback_to_savepoint, :rollback_db_transaction
+ base.set_callback :checkin, :after, :disable_query_cache!
@eugeneius
eugeneius Oct 31, 2016 Contributor

We should probably clear the query cache before the connection is checked in, rather than afterwards.

If the callback chain halts before this one can run - if there is another after callback configured, and it raises, for example - the connection would end up in the pool with a dirty cache.

@matthewd
matthewd Nov 1, 2016 Member

It doesn't actually return to the pool until after the callbacks have completed.

I instead chose to make this an 'after' on the theory that 'before' callbacks could reasonably choose to use the connection (i.e., perform queries), and we don't want to be in the middle of that. (Though it'd be a very bad idea for anyone to do so in practice, because the callbacks are run while holding the pool lock, which is supposed to be low contention.)

@eugeneius
eugeneius Nov 1, 2016 Contributor

Got it. Thanks for the explanation!

@sj26
Contributor
sj26 commented Nov 3, 2016 edited

Wonderful, thanks @matthewd. Love how that test has turned out — I wasn't familiar with Concurrent::Events.

Would be nice not to touch the connection until it is required. Could enabling the query cache be an after checkout callback?

If so, the only bits left in the QueryCache concern's executor complete method is connection pool cleanup, nothing to do with the query cache.

@sj26
Contributor
sj26 commented Nov 3, 2016

Although I suppose you don't necessarily always want to turn on the query cache...

@eugeneius
Contributor

If we enable the query cache in an after checkout callback, the cache will be enabled in several situations where it's not currently active. The ones I can think of are:

  • connections manually checked out via checkout or with_connection
  • connections from connection pools other than the one on ActiveRecord::Base
  • connections in contexts other than Rails, e.g. Sidekiq

None of these changes are necessarily bad (I would love for the multiple pools one to happen!) but it's worth carefully considering whether apps could break if they don't expect the caching behaviour.

In contrast, I think it's extremely unlikely that this change could break any application code - so I think we should ship this and decide whether to change how the cache is enabled separately.

- connection.clear_query_cache
- connection.disable_query_cache! unless enabled
-
+ def self.complete(_)
unless ActiveRecord::Base.connected? && ActiveRecord::Base.connection.transaction_open?
@eugeneius
eugeneius Nov 4, 2016 Contributor

Right now the query cache is always disabled at the end of every request. If we instead rely on a callback to disable it when the connection returns to the pool, we'll leak connections with the query cache enabled if a transaction is left open at this point.

I think it's actually fine to say "if your app leaks connections with open transactions, the consequences will be so severe that you won't even notice the query cache problem" and not do anything about this, but I thought it was worth at least pointing out.

@matthewd matthewd merged commit 3359093 into rails:master Nov 6, 2016

2 checks passed

codeclimate Code Climate didn't find any new or fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matthewd
Member
matthewd commented Nov 6, 2016

See follow-up in #26978

@matthewd
Member

Backported in 9228aa8

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