davydovanton / hanami-pagination

MIT License
14 stars 7 forks source link

N+1 for counts #13

Open davydovanton opened 6 years ago

davydovanton commented 6 years ago

https://github.com/davydovanton/hanami-pagination/commit/e86eb0af19dac92b46020cc26ab6c470546b0f55#commitcomment-29103256

iJackUA commented 6 years ago

@bhanuone @jozzi05 those fix does not remove all counts because other methods like

      def total_pages
        pager.total_pages
      end

next_page, etc. also touches ROM's pager.total indirectly https://github.com/rom-rb/rom-sql/blob/master/lib/rom/sql/plugin/pagination.rb#L71

that's quite a big issue and seems like can not be fixed by simple "caching" on hanami-pagination side

jozzi05 commented 6 years ago

@iJackUA I created PR 2 months ago https://github.com/davydovanton/hanami-pagination/pull/14 ;)

iJackUA commented 6 years ago

@jozzi05 does it really solve issue? for example when you do

        @total = pager.total
        @total_pages = pager.total_pages

rom-sql pager has such a code

          def total
            dataset.unlimited.count
          end

          def total_pages
            (total / per_page.to_f).ceil
          end

I wonder is this call

          def total
            dataset.unlimited.count
          end

being cached by rom-sql pager or underlying sequel somehow? or it still tiggers 2 queris for select count(*)...?

jozzi05 commented 6 years ago

In https://github.com/davydovanton/hanami-pagination/pull/14 description:

** Only two count queries are left. The first one when total is touched and the second one when total_pages is touched.
jozzi05 commented 6 years ago

At least one count query has to be fired(total), and since the total_pages method has some logic inside rom-sql I left it also as a delegate method.