Fix pagination_total index option to prevent SELECT count(*) queries #3848
Conversation
|
Do you want me to modify the line lengths? They're mostly from code I just moved around, and I didn't want to be too invasive. My comments are longer than 80 characters since I didn't realize the length restriction. I can change the line breaks if you want. |
ae7145d to
cc476fb
Compare
| # To do so we must not call `collection.num_pages` | ||
| offset = (collection.current_page - 1) * collection.limit_value | ||
| I18n.t "active_admin.pagination.multiple_without_total", model: entries_name, | ||
| from: offset + 1, to: offset + collection_size |
There was a problem hiding this comment.
Align the elements of a hash literal if they span more than one line.
There was a problem hiding this comment.
I know it's not your code style, but please change this and do the same on line 138
|
Okay, I changed my comments to be within 80 characters, and changed single quotes to double. |
|
note: related to #2638
Sorry but I can't merge that, that's a hack and not a solution. In my eyes this is not acceptable. There are some unexpected behaviors if you trie all variants of BTW: In my test app I still see count queries: SELECT COUNT(count_column) FROM (SELECT 1 AS count_column FROM `companies` WHERE (user_id IS NOT NULL) LIMIT 10000 OFFSET 0) subquery_for_countHave you read #2638? I'm not very familiar with #2638 but in my understanding it's a kaminari bug. |
|
Apologies, I didn't see that other issue. I tried searching for
I'm sorry, I don't understand this. Can you explain a bit? And if you disable pagination then won't that blow up activeadmin if you have millions of records (the impetus for turning
Yeah, I get these, too, but they run instantaneously for me. The The query to look out for is
Thank you for the blunt feedback. I'll see if I can find another approach. |
|
Okay, I've looked into this some more, including #2638. Contrary to what #2638 says, I'd argue that this is not a bug in Kaminari, but rather a feature request. It is a bug in activeadmin, because the documentation advertises this as a feature that removes the While Kaminari does not have an "infinite pages" feature, it does let you specify the I looked at #2638 and @azach links to a clever approach called Let me explain the issue and the approach: Here's a screenshot of the pagination component:
The activeadmin code that generates this is: def build_pagination
options = {}
options[:param_name] = @param_name if @param_name
text_node paginate collection, options
endThe pagination component can be configured via the There are two parts to the pagination component that are relevant here:
So the clever approach is still to send in options[:total_pages] = collection.current_page + Kaminari.config.window + 1
options[:right] = 0To be clear there is still one small downside: Kaminari seems to generate the "Last" link no matter what. The screenshot from my proposed fix looks like this:
If you click "Last", then the pagination on that page will look the same except now So, at this point, I see two possibilities:
I hope this new approach is sufficiently "not hacky" so that you'll accept it, but I'll understand if not and close the PR. Please let me know. |
cc476fb to
453a67d
Compare
|
Okay, since I had already worked out the code locally for the screenshots, I just pushed up my changes to the PR. Feel free to review and merge if you approve of the approach! :) |
|
First: 👍 for one of the best documented PR's I still think it's to hacky. But I have a idea: options[:total_pages] = collection.current_page + Kaminari.config.window + 1to this: next_page = collection.page(collection.current_page + 1).per(@per_page).limit(1).any?
options[:total_pages] = collection.current_page + (next_page ? 1 : 0)That's just produce a query like this: SELECT COUNT(count_column) FROM (SELECT 1 AS count_column FROM `companies` LIMIT 1 OFFSET 30) subquery_for_count |
|
Oh, that's very interesting. I like it! Sorry I haven't had time to respond. I'll try out this approach over the weekend and update the PR. |
|
There is a more easy version: offset = collection.page(collection.current_page + 1).per(@per_page).limit(1).count
options[:total_pages] = collection.current_page + offset |
|
I looked into these approaches this weekend and unfortunately they didn't work. I put a and I believe it's the usage of Kaminari's However, I don't know enough about activeadmin: are we guaranteed that the If I can use If I can't use them, then I'm out of ideas. I'll put in one more plug for the PR's current approach which is to set the total pages as "current page + 1". The only downside is that when you're on the last page, it will still have a "Next" button. In practice, I don't think it's a problem, since we're talking about collections with tens of thousands of pages here, and if a user opts in to |
|
|
|
Ah, great! I'll update the PR to use |
|
Need we more test for that? |
|
@losvedir any updates? |
453a67d to
6ab8c65
Compare
| offset = (collection.current_page - 1) * collection.limit_value | ||
| total = collection.total_count | ||
| I18n.t "active_admin.pagination.multiple", | ||
| model: entries_name, |
There was a problem hiding this comment.
Align the parameters of a method call if they span more than one line.
|
@timoschilling - Sorry, for the delay. I made the |
6ab8c65 to
d3b5a93
Compare
| @@ -5,3 +5,11 @@ | |||
| Then /^I should see pagination with (\d+) pages$/ do |count| | |||
| expect(page).to have_css '.pagination span.page', count: count | |||
| end | |||
|
|
|||
| Then /^I should see the pagination "Next" link/ do | |||
| expect(page).to have_css 'a', text: 'Next ›' | |||
There was a problem hiding this comment.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
7973eaa to
c6dd98c
Compare
| else | ||
| offset = (collection.current_page - 1) * collection.limit_value | ||
| total = collection.total_count | ||
| I18n.t 'active_admin.pagination.multiple', |
There was a problem hiding this comment.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
|
@timoschilling - Okay, ready for review! I've never used cucumber before; please let me know if I should be doing that a different way. |
c6dd98c to
61ea1ea
Compare
|
There is a syntax error on Ruby 1.9 https://travis-ci.org/activeadmin/activeadmin/jobs/57748755#L303 |
|
Ah, bummer. I don't have Ruby 1.9 on this computer and it's slow to build for me. Will have to take a look tonight. |
The pagination_total option is supposed to prevent expensive SELECT
count(*) queries from being performed when viewing a resource's index
page.
However, while this option was hiding the display of the total number of
pages, it did not actually prevent the query from being performed.
There were two reasons:
In `#build_pagination`, the line:
`text_node paginate collection, options`
would trigger Kaminari to call `#total_pages` on the collection, which would
trigger the query:
```
def paginate(scope, options = {}, &block)
options[:total_pages] ||= options[:num_pages] || scope.total_pages
```
By passing in the `:total_pages` option, we short circuit this assignment
and avoid the query.
That fix was not sufficient. In `#page_entries_info`, the call
`collection.num_pages` also triggered a SELECT count(*). I re-arranged
the conditionals to avoid that call if `@display_total` is false, while
trying to keep the behavior the same.
61ea1ea to
10b9c8a
Compare
|
Ah, ruby 1.9.3 didn't like the unicode literal in my string! Fixed. |
|
I think this As it stands there are 3 queries actually run against the table:
I see two separate potential optimizations here:
|
Addresses #3847.
The
pagination_totaloption is supposed to prevent expensiveSELECT count(*)queries from being performed when viewing a resource's index page.However, while this option was hiding the display of the total number of pages, it did not actually prevent the query from being performed.
There were two reasons:
First, in
#build_pagination, the line:text_node paginate collection, optionswould trigger Kaminari to call#total_pageson the collection, which would trigger the query. Excerpt of kaminari code:By passing in the
:total_pagesoption, we short circuit this assignment and avoid the query.The downside is we do so by hardcoding in an essentially random total pages count of 100. I chose the number 100, because I wanted to ensure ellipses would appear in Kaminari's "Page 1, 2, 3, ..., Next, Last" links, no matter how many links are configured to be displayed. However, if the resource only had 2 pages worth of items, then clicking the "3" would result in an empty page. That said, without querying the resource, there's no way to know how many pages there should be.
Second, in
#page_entries_info, the callcollection.num_pagesalso triggered aSELECT count(*). I re-arranged the conditionals to avoid that call if@display_totalis false, while trying to keep the behavior the same.