Skip to content

Commit 18e17e4

Browse files
deivid-rodriguezrafaelsalesmgrunberg
authored
Optimize count query for pagination_total: false option (#6911)
* Remove duplicated delegation `total_pages` is listed twice. * Remove ORDER BY from count subquery Queries like `SELECT COUNT(*) FROM (SELECT DISTINCT resources.* FROM resources ORDER BY resources.created_at DESC LIMIT 1 OFFSET 30) subquery_for_count` are too inefficient * add specs about ensure count query does not include ORDER BY clause * exclude also select because based on https://github.com/activeadmin/activeadmin/pull/7489\#issuecomment-1554197081 --------- Co-authored-by: Rafael Sales <rafaelcds@gmail.com> Co-authored-by: David Rodríguez <deivid-rodriguez> Co-authored-by: Matias Grunberg <matias@yellowspot.dev>
1 parent b87754a commit 18e17e4

4 files changed

Lines changed: 42 additions & 10 deletions

File tree

‎app/controllers/active_admin/resource_controller/decorators.rb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ def self.wrap(decorator)
6161
def self.wrap!(parent, name)
6262
::Class.new parent do
6363
delegate :reorder, :page, :current_page, :total_pages, :limit_value,
64-
:total_count, :total_pages, :offset, :to_key, :group_values,
64+
:total_count, :offset, :to_key, :group_values,
6565
:except, :find_each, :ransack, to: :object
6666

6767
define_singleton_method(:name) { name }

‎lib/active_admin/views/components/paginated_collection.rb‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,10 @@ def build_pagination
102102
# you pass in the :total_pages option. We issue a query to determine
103103
# if there is another page or not, but the limit/offset make this
104104
# query fast.
105-
offset = @collection.offset(@collection.current_page * @collection.limit_value).limit(1).count
105+
offset_scope = @collection.offset(@collection.current_page * @collection.limit_value)
106+
# Support array collections. Kaminari::PaginatableArray does not respond to except
107+
offset_scope = offset_scope.except(:select, :order) if offset_scope.respond_to?(:except)
108+
offset = offset_scope.limit(1).count
106109
options[:total_pages] = @collection.current_page + offset
107110
options[:right] = 0
108111
end

‎spec/support/matchers/perform_database_query_matcher.rb‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,12 @@
22

33
RSpec::Matchers.define :perform_database_query do |query|
44
match do |block|
5+
query_regexp = query.is_a?(Regexp) ? query : Regexp.new(Regexp.escape(query))
6+
57
@match = nil
68

79
callback = lambda do |_name, _started, _finished, _unique_id, payload|
8-
@match = Regexp.new(Regexp.escape(query)).match?(payload[:sql])
10+
@match = query_regexp.match?(payload[:sql])
911
end
1012

1113
ActiveSupport::Notifications.subscribed(callback, "sql.active_record", &block)

‎spec/unit/views/components/paginated_collection_spec.rb‎

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -229,16 +229,43 @@ def paginated_collection(*args)
229229
end
230230
end
231231

232-
it "makes no expensive COUNT queries when pagination_total is false" do
233-
undecorated_collection = Post.all.page(1).per(30)
232+
describe "when pagination_total is false" do
233+
it "makes no expensive COUNT queries" do
234+
undecorated_collection = Post.all.page(1).per(30)
235+
236+
expect { paginated_collection(undecorated_collection, pagination_total: false) }
237+
.not_to perform_database_query("SELECT COUNT(*) FROM \"posts\"")
238+
239+
decorated_collection = controller_with_decorator("index", PostDecorator).apply_collection_decorator(undecorated_collection.reset)
240+
241+
expect { paginated_collection(decorated_collection, pagination_total: false) }
242+
.not_to perform_database_query("SELECT COUNT(*) FROM \"posts\"")
243+
end
244+
245+
it "makes a performant COUNT query to figure out if we are on the last page" do
246+
# "SELECT COUNT(*) FROM (SELECT 1". Let's make sure the subquery has LIMIT and OFFSET. It shouldn't have ORDER BY
247+
count_query = %r{SELECT COUNT\(\*\) FROM \(SELECT 1 .*FROM "posts" (?=.*OFFSET \?)(?=.*LIMIT \?)(?!.*ORDER BY)}
234248

235-
expect { paginated_collection(undecorated_collection, pagination_total: false) }
236-
.not_to perform_database_query("SELECT COUNT(*) FROM \"posts\"")
249+
undecorated_collection = Post.all.page(1).per(30)
237250

238-
decorated_collection = controller_with_decorator("index", PostDecorator).apply_collection_decorator(undecorated_collection)
251+
expect { paginated_collection(undecorated_collection, pagination_total: false) }
252+
.to perform_database_query(count_query)
239253

240-
expect { paginated_collection(decorated_collection, pagination_total: false) }
241-
.not_to perform_database_query("SELECT COUNT(*) FROM \"posts\"")
254+
undecorated_sorted_collection = undecorated_collection.reset.order(id: :desc)
255+
256+
expect { paginated_collection(undecorated_sorted_collection, pagination_total: false) }
257+
.to perform_database_query(count_query)
258+
259+
decorated_collection = controller_with_decorator("index", PostDecorator).apply_collection_decorator(undecorated_collection.reset)
260+
261+
expect { paginated_collection(decorated_collection, pagination_total: false) }
262+
.to perform_database_query(count_query)
263+
264+
decorated_sorted_collection = controller_with_decorator("index", PostDecorator).apply_collection_decorator(undecorated_sorted_collection.reset)
265+
266+
expect { paginated_collection(decorated_sorted_collection, pagination_total: false) }
267+
.to perform_database_query(count_query)
268+
end
242269
end
243270

244271
it "makes no COUNT queries to figure out the last element of each page" do

0 commit comments

Comments
 (0)