Collection Helper should not make redundant COUNT queries.

[allisonphillips]

I have been looking into why pagination_total: false is not preventing the execution of multiple subquery_for_count queries. I found the issue appeared rooted in the collection_size method not validating whether a collection is already loaded. General context on my changes:

• This method is called several times when building an index page
• Because the current method definition uses count, this triggers several redundant count subqueries (subquery_for_count) that cause latency and even timeouts when loading index pages
• I also modified the guards at the beginning of the method because Array#length should be more performant than Array#count within this context

Resolves #6492, #2638

Testing/QA

Because this is a refactor and not new behavior I did not add new specs. When looking into this, I did come across some bugs in the paginated collection specs that should probably be fixed at some point though; for example, the examples pertaining to pagination_total will always result in a false pass because they're testing for the absence of queries that will never occur, and it's going uncaught because the author did not test the opposite assertion when the option is false.

I've QA'd this as an override in a staging environment that uses a sanitized production DB snapshot by loading index pages for tables that I know to have millions of records. The staging environment has fewer computing resources, but after pushing this change it is rendering large index pages faster than the production environment — it's even rendering pagination_total: false pages that are timing out in production (due to the COUNT subqueries).

In a development environment, I can also see that this prevents the redundant COUNT subqueries that drive many of us to attempt using the pagination_total option; here is my sanitized server log:

Before

Rendering redacted/app/views/active_admin/resource/index.html.arb
  ...
   (41.4ms)  SELECT COUNT(*) FROM (SELECT  1 AS one FROM `redacted_models ` LIMIT 20 OFFSET 0) subquery_for_count
   (2.6ms)  SELECT COUNT(*) FROM (SELECT  1 AS one FROM `redacted_models ` LIMIT 20 OFFSET 0) subquery_for_count
   (4.9ms)  SELECT COUNT(*) FROM (SELECT  1 AS one FROM `redacted_models ` ORDER BY `redacted_models `.`id` desc LIMIT 1 OFFSET 20) subquery_for_count
   (0.4ms)  SELECT COUNT(*) FROM (SELECT  1 AS one FROM `redacted_models ` LIMIT 20 OFFSET 0) subquery_for_count
  RedactedModel Load (0.4ms)  SELECT  `redacted_models`.* FROM `redacted_models ` ORDER BY `redacted_models `.`id` desc LIMIT 20 OFFSET 0

  Rendered redacted/app/views/active_admin/resource/index.html.arb (233.7ms)

After

Rendering redacted/app/views/active_admin/resource/index.html.arb
  ...
  RedactedModel Load (1.6ms)  SELECT  `redacted_models`.* FROM `redacted_models` ORDER BY `redacted_models`.`id` desc LIMIT 20 OFFSET 0
   (0.6ms)  SELECT COUNT(*) FROM (SELECT  1 AS one FROM `redacted_models` ORDER BY `redacted_models`.`id` desc LIMIT 1 OFFSET 20) subquery_for_count
  Rendered redacted/app/views/active_admin/resource/index.html.arb (2815.6ms)

[javierjulio]

Thanks for tackling this! Would you consider adding some specs for the cases you've mentioned? It would help to have some more coverage around this as we'd appreciate any help and input on improving that.

[Nkadze]

Guys any plans to merge this PR?

[deivid-rodriguez]

Can you help us add a spec?

[Nkadze]

Just checked this fork on my project and I still get subquery_for_count when used with scoped_collection including joins and merges

[deivid-rodriguez]

So what you're saying is that this does not actually work?

[Nkadze]

Right now, in my case
if you use

controller do
    def scoped_collection
      super.joins(:parent).includes(:parent).merge(Parent.some_simple_scope).order('parent.column DESC')
    end
  end

something like this, then you use filter on one of the columns, it still generates heavy subquery_for_counts

[DaniPB]

Right now, in my case if you use

controller do
    def scoped_collection
      super.joins(:parent).includes(:parent).merge(Parent.some_simple_scope).order('parent.column DESC')
    end
  end

something like this, then you use the filter on one of the columns, it still generates heavy subquery_for_counts

@Nkadze if you want to solve this, you can set the scope_count option as false

index pagination_total: false, scope_count: false do

I've applied this snippet as a monkey patch, and it works fine. Are you planning to merge this PR?

[hwo411]

There is also a problem when using pagination_total: false with

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

Line 106 in 3f2c8d2

offset = collection.offset(collection.current_page * collection.limit_value).limit(1).count

this part when custom select and order is used.

We monkey-patched it with collection.model.from to perform count on subquery. Not sure if it's good for general use-case, so not providing and MR, but JFYI, if someone is more experienced with the codebase who can fix it reads it.