Created attachment 92841 [details] [review] Count total number of rows 1,000 at a time C4::Reports::Guided::nb_rows (called by get_prepped_report in reports/guided_reports.pl) uses DBI::fetchall_arrayref to retrieve all rows at once; counts them; and then discards the rows and returns the count. This has the potential, if the number of rows is very large, to exhaust all available memory. (Other code in guided_reports.pl has the same potential effect, but because the solution to that is much less straightforward it will be addressed in a separate bug report.) This patch uses the second ($max_rows) parameter to DBI::fetchall_arrayref to retrieve a smaller number (1,000) of rows at a time, looping until all results have been retrieved. This will only use as much memory as the maximum amount used by a single call to DBI::fetchall_arrayref.
I've changed the priority and severity on this bug report, along with #23626, to P2/major since the problem they solve can make Koha unusable.
Created attachment 92903 [details] [review] Bug 23624 - Count rows in report without (potentially) consuming all memory C4::Reports::Guided::nb_rows (called by get_prepped_report in reports/guided_reports.pl) uses DBI::fetchall_arrayref to retrieve all rows at once; counts them; and then discards the rows and returns the count. This has the potential, if the number of rows is very large, to exhaust all available memory. (Other code in guided_reports.pl has the same potential effect, but because the solution to that is much less straightforward it will be addressed in a separate bug report.) This patch uses the second ($max_rows) parameter to DBI::fetchall_arrayref to retrieve a smaller number (1,000) of rows at a time, looping until all results have been retrieved. This will only use as much memory as the maximum amount used by a single call to DBI::fetchall_arrayref. Test Plan: 1) Create a report the will generate a huge number of results 2) Run the report, watch your memory usage spike 3) Apply this patch 4) Restart all the things! 5) Run the report again, note your memory usage is much lower Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
QA thoughts: 1000 per fetch feels arbitrary. Should that number be controllable via a config entry or syspref?
It could be, but the number seems more like an implementation detail than a preference or config option. And it seems reasonable -- a million rows, which without this patch would probably kill any Koha instance, is still only 1,000 iterations of the loop. Most importantly, though, I wouldn't want a debate over the number to hold up the patch -- we can always change it later. (I certainly wouldn't object to some other number.)
Created attachment 92968 [details] [review] Bug 23624: Count rows in report without (potentially) consuming all memory C4::Reports::Guided::nb_rows (called by get_prepped_report in reports/guided_reports.pl) uses DBI::fetchall_arrayref to retrieve all rows at once; counts them; and then discards the rows and returns the count. This has the potential, if the number of rows is very large, to exhaust all available memory. (Other code in guided_reports.pl has the same potential effect, but because the solution to that is much less straightforward it will be addressed in a separate bug report.) This patch uses the second ($max_rows) parameter to DBI::fetchall_arrayref to retrieve a smaller number (1,000) of rows at a time, looping until all results have been retrieved. This will only use as much memory as the maximum amount used by a single call to DBI::fetchall_arrayref. Test Plan: 1) Create a report the will generate a huge number of results 2) Run the report, watch your memory usage spike 3) Apply this patch 4) Restart all the things! 5) Run the report again, note your memory usage is much lower Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 92969 [details] [review] Bug 23624: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 92970 [details] [review] Bug 23624: (QA follow-up) Optimize even more This patch makes counting the results have no memory footprint by leveraging on the DB to count the rows. To test: - Without this path, run: $ kshell k$ prove t/db_dependent/Reports/Guided.t => SUCCESS: Tests pass - Apply this patch - Run: k$ prove t/db_dependent/Reports/Guided.t => SUCCESS: Tests still pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Thanks, that's the perfect solution. I had contemplated (and rejected) munging the SQL to get a count but never thought of using a subquery.
Created attachment 92974 [details] [review] Bug 23624: Count rows in report without (potentially) consuming all memory C4::Reports::Guided::nb_rows (called by get_prepped_report in reports/guided_reports.pl) uses DBI::fetchall_arrayref to retrieve all rows at once; counts them; and then discards the rows and returns the count. This has the potential, if the number of rows is very large, to exhaust all available memory. (Other code in guided_reports.pl has the same potential effect, but because the solution to that is much less straightforward it will be addressed in a separate bug report.) This patch uses the second ($max_rows) parameter to DBI::fetchall_arrayref to retrieve a smaller number (1,000) of rows at a time, looping until all results have been retrieved. This will only use as much memory as the maximum amount used by a single call to DBI::fetchall_arrayref. Test Plan: 1) Create a report the will generate a huge number of results 2) Run the report, watch your memory usage spike 3) Apply this patch 4) Restart all the things! 5) Run the report again, note your memory usage is much lower Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 92975 [details] [review] Bug 23624: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 92976 [details] [review] Bug 23624: (QA follow-up) Optimize even more This patch makes counting the results have no memory footprint by leveraging on the DB to count the rows. To test: - Without this path, run: $ kshell k$ prove t/db_dependent/Reports/Guided.t => SUCCESS: Tests pass - Apply this patch - Run: k$ prove t/db_dependent/Reports/Guided.t => SUCCESS: Tests still pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Liz Rea <wizzyrea@gmail.com>
If the report being run has a syntax error in it, Koha now gets an error: Can't use an undefined value as an ARRAY reference at /kohadevbox/koha/C4/Reports/Guided.pm line 432
Created attachment 92995 [details] [review] Bug 23624: (QA follow-up) Don't fetch the count unless the query was successful
Created attachment 93037 [details] [review] Bug 23624: (QA follow-up) Don't fetch the count unless the query was successful Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Do *I* need to sign off on this? I'm woefully ignorant of Koha community dev practices.
(In reply to Paul Hoffman from comment #15) > Do *I* need to sign off on this? I'm woefully ignorant of Koha community > dev practices. One sign-off is enough to move it forward in the process, so yours is not required. However if you *wanted* to test and sign off it would be welcomed.
Created attachment 93153 [details] [review] Bug 23624: Count rows in report without (potentially) consuming all memory C4::Reports::Guided::nb_rows (called by get_prepped_report in reports/guided_reports.pl) uses DBI::fetchall_arrayref to retrieve all rows at once; counts them; and then discards the rows and returns the count. This has the potential, if the number of rows is very large, to exhaust all available memory. (Other code in guided_reports.pl has the same potential effect, but because the solution to that is much less straightforward it will be addressed in a separate bug report.) This patch uses the second ($max_rows) parameter to DBI::fetchall_arrayref to retrieve a smaller number (1,000) of rows at a time, looping until all results have been retrieved. This will only use as much memory as the maximum amount used by a single call to DBI::fetchall_arrayref. Test Plan: 1) Create a report the will generate a huge number of results 2) Run the report, watch your memory usage spike 3) Apply this patch 4) Restart all the things! 5) Run the report again, note your memory usage is much lower Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 93154 [details] [review] Bug 23624: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 93155 [details] [review] Bug 23624: (QA follow-up) Optimize even more This patch makes counting the results have no memory footprint by leveraging on the DB to count the rows. To test: - Without this path, run: $ kshell k$ prove t/db_dependent/Reports/Guided.t => SUCCESS: Tests pass - Apply this patch - Run: k$ prove t/db_dependent/Reports/Guided.t => SUCCESS: Tests still pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 93156 [details] [review] Bug 23624: (QA follow-up) Don't fetch the count unless the query was successful Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 93157 [details] [review] Bug 23624: (QA follow-up) Test error cases Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
I am no fan of the derived table name xxx Could you use something with nb_rows_xxx ? Or use something with random or file::temp etc ?
Nice work! Pushed to master for 19.11.00
Thanks Paul, and congratulations on your first patch accepted into the Koha codebase. I've also taken the liberty of giving full sponsorship attribution in a followup commit message only patch. Thanks: Sponsored-by: Higher Education Libraries of Massachusetts Sponsored-by: Fenway Libraries Online
Pushed to 19.05.x for 19.05.05
backported to 18.11.x for 18.11.11
Moved sponsorship into a git followup commit ensuring it appears in the release notes and on the about page.