Refactor QueryCache to be owned by the pool #50938
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rafaelfranca
approved these changes
Feb 10, 2024
casperisfine
force-pushed
the
refactor-query-cache-to-pool
branch
from
bbf34f3
to
f771d9b
Compare
casperisfine
commented
Feb 12, 2024
|
|
||
| private | ||
| def query_cache | ||
| @thread_query_caches.compute_if_absent(connection_cache_key(ActiveSupport::IsolatedExecutionState.context)) do |
There was a problem hiding this comment.
Note: we need to prune dead thread/fibers from this map, otherwise code that do queries from short lived threads or fibers would leak memory.
That's likely the reaper's job.
There was a problem hiding this comment.
(or we should move that in ActiveSupport::IsolatedExecutionState.
casperisfine
force-pushed
the
refactor-query-cache-to-pool
branch
from
9a8d06d
to
f864c6c
Compare
Ref: rails#50793 If we want to stop caching the checked out connections, then we must persist the cache in the pool, and assign it to the connection when it's checked out. The pool become responsible for managing the cache lifecycle. This also open the door to sharing the cache between multiple connections, which is valuable for read replicas, etc. This change only really make sense if we go through with no longer caching checked out connections. Otherwise it's just extra complexity.
Otherwise they could linger around and leak memory if a user checkout connections from short lived fibers or threads. The undocumented `connection_cache_key` hook point is eliminated because it was essentially add to allow the connection pool to be fiber based rather than thread based, which is now supported out of the box.
casperisfine
force-pushed
the
refactor-query-cache-to-pool
branch
from
f864c6c
to
94fc536
Compare
casperisfine
pushed a commit
to Shopify/rails
that referenced
this pull request
Feb 21, 2024
Followup: rails#50938 The behavior changed to always clear the query cache as soon as it's disabled, on the assumption that once queries have been performed without it, all bets are off. However that isn't quite true, we can apply the same clearing logic than when it's enabled. If you perform read only queries without the cache, there is no reason to clear it.
casperisfine
pushed a commit
to Shopify/rails
that referenced
this pull request
Feb 21, 2024
Followup: rails#50938 The behavior changed to always clear the query cache as soon as it's disabled, on the assumption that once queries have been performed without it, all bets are off. However that isn't quite true, we can apply the same clearing logic than when it's enabled. If you perform read only queries without the cache, there is no reason to clear it.
jenshenny
pushed a commit
to Shopify/rails
that referenced
this pull request
Feb 23, 2024
Followup: rails#50938 The behavior changed to always clear the query cache as soon as it's disabled, on the assumption that once queries have been performed without it, all bets are off. However that isn't quite true, we can apply the same clearing logic than when it's enabled. If you perform read only queries without the cache, there is no reason to clear it.
gabriel-amaral
pushed a commit
to gabriel-amaral/rails
that referenced
this pull request
Feb 27, 2024
Followup: rails#50938 The behavior changed to always clear the query cache as soon as it's disabled, on the assumption that once queries have been performed without it, all bets are off. However that isn't quite true, we can apply the same clearing logic than when it's enabled. If you perform read only queries without the cache, there is no reason to clear it.
jathayde
pushed a commit
to jathayde/rails
that referenced
this pull request
Feb 28, 2024
Followup: rails#50938 The behavior changed to always clear the query cache as soon as it's disabled, on the assumption that once queries have been performed without it, all bets are off. However that isn't quite true, we can apply the same clearing logic than when it's enabled. If you perform read only queries without the cache, there is no reason to clear it.
Ridhwana
pushed a commit
to Ridhwana/rails
that referenced
this pull request
Mar 7, 2024
Followup: rails#50938 The behavior changed to always clear the query cache as soon as it's disabled, on the assumption that once queries have been performed without it, all bets are off. However that isn't quite true, we can apply the same clearing logic than when it's enabled. If you perform read only queries without the cache, there is no reason to clear it.
viralpraxis
pushed a commit
to viralpraxis/rails
that referenced
this pull request
Mar 24, 2024
Followup: rails#50938 The behavior changed to always clear the query cache as soon as it's disabled, on the assumption that once queries have been performed without it, all bets are off. However that isn't quite true, we can apply the same clearing logic than when it's enabled. If you perform read only queries without the cache, there is no reason to clear it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ref: #50793
If we want to stop caching the checked out connections, then we must persist the cache in the pool, and assign it to the connection when it's checked out.
The pool become responsible for managing the cache lifecycle.
This also open the door to sharing the cache between multiple connections, which is valuable for read replicas, etc.
This change only really make sense if we go through with no longer caching checked out connections. Otherwise it's just extra complexity.