Skip to content

Commit 358b1c7

Browse files
skipkayhilbyroot
authored andcommitted
Enable query-cacheable queries to be retryable
The query cache implementation calls `to_sql_and_binds` inside its `select_all` override so that it can use the generated sql and binds as a cache key. However, the current implementation does not pass the `allow_retry` parameter into `to_sql_and_binds`. This is an issue when the `arel` parameter is a String, because query cache's `select_all` will override the `allow_retry` parameter with the value returned from `to_sql_and_binds` (and because it isn't passed through, it will always be the default, false). This commit fixes the issue by passing the `allow_retry` parameter through to `to_sql_and_binds`. This will ensure that when `select_all` is called with a String query, `allow_retry: true`, and the query cache enabled that the query will maintain its retryable state. This combination of prerequisites can occur when a query is executed from the Statement Cache since the Statement Cache does its own query compilation.
1 parent 730e815 commit 358b1c7

File tree

2 files changed

+22
-1
lines changed

2 files changed

+22
-1
lines changed

activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,7 @@ def select_all(arel, name = nil, binds = [], preparable: nil, async: false, allo
239239
# If arel is locked this is a SELECT ... FOR UPDATE or somesuch.
240240
# Such queries should not be cached.
241241
if @query_cache&.enabled? && !(arel.respond_to?(:locked) && arel.locked)
242-
sql, binds, preparable, allow_retry = to_sql_and_binds(arel, binds, preparable)
242+
sql, binds, preparable, allow_retry = to_sql_and_binds(arel, binds, preparable, allow_retry)
243243

244244
if async
245245
result = lookup_sql_cache(sql, name, binds) || super(sql, name, binds, preparable: preparable, async: async, allow_retry: allow_retry)

activerecord/test/cases/adapter_test.rb

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -717,6 +717,27 @@ def teardown
717717
end
718718
end
719719

720+
test "query cacheable idempotent SELECT queries allow retries" do
721+
@connection.enable_query_cache!
722+
723+
notifications = capture_notifications("sql.active_record") do
724+
assert_not_nil (a = Author.first)
725+
assert_not_nil Post.where(id: [1, 2]).first
726+
assert_not_nil Post.find(1)
727+
assert_not_nil Post.find_by(title: "Welcome to the weblog")
728+
assert_predicate Post, :exists?
729+
a.books.to_a
730+
end.select { |n| n.payload[:name] != "SCHEMA" }
731+
732+
assert_equal 6, notifications.length
733+
734+
notifications.each do |n|
735+
assert n.payload[:allow_retry], "#{n.payload[:sql]} was not retryable"
736+
end
737+
ensure
738+
@connection.disable_query_cache!
739+
end
740+
720741
test "queries containing SQL fragments do not allow retries" do
721742
notifications = capture_notifications("sql.active_record") do
722743
Post.where("1 = 1").to_a

0 commit comments

Comments
 (0)