Clear query cache during checkin, instead of an execution callback#26909
Conversation
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]
|
This follows on from fa7efca, but unlike that change, does not seem suitable for backport. |
| 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! |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
Got it. Thanks for the explanation!
|
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. |
|
Although I suppose you don't necessarily always want to turn on the query cache... |
|
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:
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.disable_query_cache! unless enabled | ||
|
|
||
| def self.complete(_) | ||
| unless ActiveRecord::Base.connected? && ActiveRecord::Base.connection.transaction_open? |
There was a problem hiding this comment.
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.
|
See follow-up in #26978 |
Clear query cache during checkin, instead of an execution callback
|
Backported in 9228aa8 |
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)