Revert PR #4068 and use pagination_total optimization with decorators#5389
Revert PR #4068 and use pagination_total optimization with decorators#5389rafaelsales wants to merge 3 commits intoactiveadmin:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5389 +/- ##
==========================================
+ Coverage 98.33% 98.33% +<.01%
==========================================
Files 294 294
Lines 11033 11035 +2
==========================================
+ Hits 10849 10851 +2
Misses 184 184
Continue to review full report at Codecov.
|
varyonic
dismissed
their stale review
Oops, paginated_collection_spec is not passing.
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
|
@varyonic fixed! |
varyonic
left a comment
varyonic
left a comment
There was a problem hiding this comment.
As Sean noted on my original PR a test would be nice, but I agree this is a better fix.
| # you pass in the :total_pages option. We issue a query to determine | ||
| # if there is another page or not, but the limit/offset make this | ||
| # query fast. | ||
| offset = collection.offset(collection.current_page * @per_page.to_i).limit(1).count |
There was a problem hiding this comment.
Is the check for responding to offset no longer required?
There was a problem hiding this comment.
It was never required (https://github.com/activeadmin/activeadmin/pull/4068/files#diff-a388362633460ba06e0a1f38bbb8ce1aR103) - it was just a different way to optimize.
|
Any update on my question for the Is there a way to time out queries in a multi-database way to only allocate a certain amount of time for count queries? |
Maybe... but this is certainly out of scope :) I really wanted to get rid of the monkey patch from my apps 😭 |
|
@rafaelsales Sorry for missing this PR. We have (at least partially) fixed this in #5811. I should've included you in the author's list because the solution originates from this PR, sorry about that. This PR includes some extra changes that are probably further optimizations. Would you like to rebase this PR so we can also add those improvements? Thanks! |
|
Rebased as #6911. |
4 participants
The PR #3848 introduced an optimization that prevents COUNT queries when using
index pagination_total: false.I noticed that when using the macro
decorate_with, the optimization didn't work and the count query across the entire database was made. That is because the AA default collection decorator didn't implementoffset. I believe the PR #4068 made the wrong fix for thisoffsetto AR relationSELECT COUNT(*) FROM (SELECT DISTINCT resources.* FROM resources ORDER BY resources.created_at DESC LIMIT 1 OFFSET 30) subquery_for_countare very inefficient