Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert PR #4068 and use pagination_total optimization with decorators #5389

Closed
wants to merge 3 commits into from

Conversation

rafaelsales
Copy link

@rafaelsales rafaelsales commented Apr 3, 2018

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 implement offset. I believe the PR #4068 made the wrong fix for this

  • Fix: revert Skip optimization if decorated collection does not support it. #4068 and make the collection decorator delegate offset to AR relation
  • 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 very inefficient

@codecov
Copy link

codecov bot commented Apr 3, 2018

Codecov Report

Merging #5389 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5389      +/-   ##
==========================================
+ Coverage   98.33%   98.33%   +<.01%     
==========================================
  Files         294      294              
  Lines       11033    11035       +2     
==========================================
+ Hits        10849    10851       +2     
  Misses        184      184
Impacted Files Coverage Δ
lib/active_admin/resource_controller/decorators.rb 89.79% <ø> (ø) ⬆️
...ive_admin/views/components/paginated_collection.rb 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dabd8b3...a5316b8. Read the comment docs.

varyonic
varyonic previously approved these changes Apr 3, 2018
@varyonic varyonic dismissed their stale review April 3, 2018 12:41

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
@rafaelsales
Copy link
Author

@varyonic fixed!

Copy link
Contributor

@varyonic varyonic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As Sean noted on my original PR a test would be nice, but I agree this is a better fix.

# The #paginate method in kaminari will query the resource with a
# count(*) to determine how many pages there should be unless
# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the check for responding to offset no longer required?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was never required (https://github.com/activeadmin/activeadmin/pull/4068/files#diff-a388362633460ba06e0a1f38bbb8ce1aR103) - it was just a different way to optimize.

@zorab47
Copy link
Contributor

zorab47 commented Jun 1, 2018

Any update on my question for the offset check @rafaelsales?

Is there a way to time out queries in a multi-database way to only allocate a certain amount of time for count queries?

@rafaelsales
Copy link
Author

rafaelsales commented May 6, 2019

@zorab47

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 😭

@deivid-rodriguez
Copy link
Member

@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!

@deivid-rodriguez
Copy link
Member

Rebased as #6911.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants