Description
Paul Hoffman
2019-09-16 19:51:19 UTC
I've changed the priority and severity on this bug report, along with #23624, to P2/major since the problem they solve can make Koha unusable. Created attachment 92904 [details] [review] Bug 23626 - Add a system preference to limit the number of rows of data used in a chart when viewing report results The number of rows generated by a report is potentially unlimited; this has the potential to exhaust all available memory, bringing a Koha server down to its knees. The attached patch introduces a system preference to limit the number of rows used when charting report results; at this time, and with bug #23624 fixed, that is the only aspect of reporting that loads all results into memory at once. Created attachment 92905 [details] [review] Bug 23626 - Add a system preference to limit the number of rows of data used in a chart when viewing report results The number of rows generated by a report is potentially unlimited; this has the potential to exhaust all available memory, bringing a Koha server down to its knees. The attached patch introduces a system preference to limit the number of rows used when charting report results; at this time, and with bug #23624 fixed, that is the only aspect of reporting that loads all results into memory at once. Created attachment 92908 [details] [review] Bug 23626 - Add a system preference to limit the number of rows of data used in a chart when viewing report results The number of rows generated by a report is potentially unlimited; this has the potential to exhaust all available memory, bringing a Koha server down to its knees. The attached patch introduces a system preference to limit the number of rows used when charting report results; at this time, and with bug #23624 fixed, that is the only aspect of reporting that loads all results into memory at once. Created attachment 92909 [details] [review] Bug 23626: Add database update and system preference to web ui Created attachment 92910 [details] [review] Bug 23626: Don't enable by default I see the message about data limiting, but the charge doesn't seem to change. I don't see anything in the patch that actually changes the chart generating logic. Also, just so you know, your patches were formatted for git. I went ahead and made them into git patches for you! > Also, just so you know, your patches were formatted for git. I went ahead
> and made them into git patches for you!
*not* that is, your patches were not formatted for applying with git.
Created attachment 92919 [details] [review] Bug 23626 - Add a system preference to limit the number of rows of data used in a chart when viewing report results The number of rows generated by a report is potentially unlimited; this has the potential to exhaust all available memory, bringing a Koha server down to its knees. The attached patch introduces a system preference to limit the number of rows used when charting report results; at this time, and with bug #23624 fixed, that is the only aspect of reporting that loads all results into memory at once. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 92920 [details] [review] Bug 23626: Add database update and system preference to web ui Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 92921 [details] [review] Bug 23626: Don't enable by default Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Hi, I would like an example query to be used on charts, to understand the problem better. My understanding is that charts should be used in statistical reports, i.e. those that count things, calculate averages, etc. Those kind of reports aren't usually returning thousands of rows. I'm sure I'm missing something, so please share with me some real-life report to understand the problem better Here's an example from our Koha instance: SELECT monthname(timestamp) AS month, year(timestamp) AS year, count(itemnumber) AS count FROM items WHERE timestamp BETWEEN <<Between (yyyy-mm-dd)|date>> AND <<and (yyyy-mm-dd)|date>> GROUP BY year(timestamp), month(timestamp) I have screenshots showing the steps in charting this -- I can attach them if they're useful. However, I don't want this to distract from the problem that this bug report and patch are trying to mitigate (not solve!) which is that no matter what the report is and what the user might end up doing with it -- charting, downloading as *.ods or *.csv, or just viewing page by page -- the full report results (n rows of data with n unbounded) are loaded into memory on the Koha server, converted to JSON, and then returned to the user embedded in the generated HTML. For example, I ran a query that returned a single row; it yielded HTML containing the following (some whitespace, along with the bulk of the function body, removed for clarity): -------------------------------------------------------------------------------- $('#draw-chart').click(function() { var x_elements = $('select[name="x"]').val(); var y_elements = []; var groups = []; var lines = []; var options = {}; headers = [{"cell":"month"},{"cell":"year"},{"cell":"count"}]; var results; if ($('input[name="chart-include-all"]').prop('checked')) { results = [{"cells":[{"cell":"September"},{"cell":2019},{"cell":39}]}] } else { results = [{"cells":[{"cell":"September"},{"cell":"2019"},{"cell":"39"}]}] } [...] } -------------------------------------------------------------------------------- The first line that sets the _results_ variable is where the full report results appear; if the report had yielded 10,000 rows, there would be 10,000 elements in the array. And I haven't even touched on the possibility of a query like this that fails to properly JOIN tables: SELECT foo.bar, baz.qux FROM foo, baz If foo and baz each contain 10,000 rows, that's 100 million rows returned by the DB, loaded into memory, converted to JSON, and -- well, it won't get that far, because nobody is running a Koha instance with enough RAM and swap to survive this. But I would bet that a lot of Koha instances have (staff) users with the requisite permissions to write reports but not the expertise in SQL and RDBMSes that would steer them away from writing a query like this. Stepping outside the bounds of this bug report for a moment, I'll just mention some options for truly *fixing* the underlying problem that may be worth considering; these include (a) Removing the option to chart the full report results; (b) Implementing full-data charting using callbacks (AJAX or whatever) to fetch data from Koha only as it's needed; and (c) Ripping out the charting feature altogether on the principle of "do one thing well", which entails taking the position that charting is best done in a tool (Libreoffice Calc, Excel, whatever) dedicated to that task. This last one is my favorite, but I'm sure not everyone agrees! :-) Paul, I understand the problem now. I'm not sure what's the best solution, even though I have a tendency to think we could send the data stream into a temporary file if the resultset size exceeds some fixed value, then send the file. That said, I think this issue and a proper solution deserves being discussed on our regular dev meetings on IRC. The next one is due to take place on sept 25th. [1] I'd suggest you add this problem and approach to the agenda so the dev team as a whole can discuss it. Also, adding it to the agenda in advance gives everyone time to think about it. As of the patchset itself, the only thing I wouldn't accept is that there's no fallback to a default value when reading the syspref. i.e. it could be undef and break things. [1] https://wiki.koha-community.org/wiki/Next_IRC_meetings Thanks, Tomás. You're right; the patch should be changed so that it defaults to zero: my $max_rows = C4::Context->preference('ReportsMaxRowsInChart') || 0; Should I submit a modified patch and mark the previous one (attachment 92919 [details] [review]) obsolete? The only problem is -- I'm embarrased to admit this -- I don't know how to make git format-patch generate the right format. Created attachment 92992 [details] [review] Bug 23626: Move message to relavent chart control As it stands, this limit message makes it sounds like the chart data is limited where it clearly is not. Instead, we should show the limit on the 'use all data' checkbox. As it stands, you could actual have more results data in the chart than the system preference allows by only using the displayed results! I think this is a benefit and not a problem, since we are not refetching those data to generate the chart. Instead, we need just move the 'warning' to make it clear when the syspref limit will be used and relevant. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 92994 [details] [review] Bug 23626: Move message to relevant chart control As it stands, this limit message makes it sounds like the chart data is limited where it clearly is not. Instead, we should show the limit on the 'use all data' checkbox. As it stands, you could actual have more results data in the chart than the system preference allows by only using the displayed results! I think this is a benefit and not a problem, since we are not refetching those data to generate the chart. Instead, we need just move the 'warning' to make it clear when the syspref limit will be used and relevant. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Video Demonstration: https://monosnap.com/file/eCK8NlXEG5hN0Z0KwHkxsuR0Hf7nq6 Created attachment 93126 [details] [review] Bug 23626: [alternate] Use svc/reports to fetch chart data We have a method for getting json report data via svc, and this has a built in limiter in the sysprefs. We can utilise this for our charts to avoid passing all data in memory To test: 1 - Write a report that can be charted, for example: SELECT RIGHT(barcode,2), COUNT(*) FROM items WHERE RIGHT(barcode,1) != <<Number>> GROUP BY RIGHT(barcode,2) 2 - Set the 'Cache expiry' to 1 for this report 3 - Run it and click 'Create chart' on results page Chart type: Pie x column RIGHT(barcode,2) Include all rows: UNCHECKED 4 - Note you get a nice pie chart 5 - Click 'Create chart', same except check 'Include all rows' 6 - Note you get a different pie 7 - Apply patch 8 - Repeat above - note that 'all rows' is now 'Fetch additional rows' and notes the sys pref control 9 - Increase SvcMaxReportRows and note chart changes Presenting an alternate patch here. We should only fetch the additional chart data when needed, using the report/svc we can get the json on demand. This also allows us to utilize the existing syspref SvcMaxReportRows Please test and let me know what you think. (In reply to Nick Clemens from comment #20) > Presenting an alternate patch here. We should only fetch the additional > chart data when needed, using the report/svc we can get the json on demand. > This also allows us to utilize the existing syspref SvcMaxReportRows > > Please test and let me know what you think. With the caching on the svc you can often see confusing results when testing JSON - I think using it in the GUI could give the same problem. Is it really such an issue that we should not allow people to get "all" if they explicitly check "all"? I think at least you'd want the GUI behave differently than a more restricted web service that might be used by third parties. (In reply to Katrin Fischer from comment #21) > With the caching on the svc you can often see confusing results when testing > JSON - I think using it in the GUI could give the same problem. I think we could add some info to GUI to make it more clear how the caching operates and why you would set it. > Is it really such an issue that we should not allow people to get "all" if > they explicitly check "all"? I think at least you'd want the GUI behave > differently than a more restricted web service that might be used by third > parties. It is a big issue because loading a full set of results can crash the system. Reports with a large amounts of rows/data must be loaded into memory to pass to the template in either scenario. (DON'T) Try running: SELECT * FROM items JOIN biblio_metadata USING (itemnumber) JOIN items i ON 1=1; It generates so much data it will lock the system (mine required a hard reboot). This is a silly report, but honestly large reports that are legitimate in production systems are triggering this issue. Charts are neat, but I think we can limit the amount of data they get and rely on exporting to tools meant for data analysis if large scale are needed. Created attachment 93137 [details] [review] Bug 23626: [alternate] Use svc/reports to fetch chart data We have a method for getting json report data via svc, and this has a built in limiter in the sysprefs. We can utilise this for our charts to avoid passing all data in memory To test: 1 - Write a report that can be charted, for example: SELECT RIGHT(barcode,2), COUNT(*) FROM items WHERE RIGHT(barcode,1) != <<Number>> GROUP BY RIGHT(barcode,2) 2 - Set the 'Cache expiry' to 1 for this report 3 - Run it and click 'Create chart' on results page Chart type: Pie x column RIGHT(barcode,2) Include all rows: UNCHECKED 4 - Note you get a nice pie chart 5 - Click 'Create chart', same except check 'Include all rows' 6 - Note you get a different pie 7 - Apply patch 8 - Repeat above - note that 'all rows' is now 'Fetch additional rows' and notes the sys pref control 9 - Increase SvcMaxReportRows and note chart changes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> I feel if we need this limit, we should not tie it to the pref and have another (yes...) and also display a tooltip or something. Otherwise this will be very hard to figure out for users. And the preset limit of 20 or something for the pref is very low. I'm worried that a rewrite of the report charting capability is going to take some time, and meanwhile our Koha server is liable to run out of memory at any time when someone runs a report that generates a lot of results. How would you all feel about sticking to the original bug fix plan for now and reworking charting later? (In reply to Paul Hoffman from comment #25) > I'm worried that a rewrite of the report charting capability is going to > take some time, and meanwhile our Koha server is liable to run out of memory > at any time when someone runs a report that generates a lot of results. > > How would you all feel about sticking to the original bug fix plan for now > and reworking charting later? Hi Paul, the maintenance releases just happened, so it will take some time to get a fix into Koha proper (until next month's bugfix release). Using reports is permission based, maybe inform your staff of the problem? Also implementing a temporary fix might be an option if you are running your own installation. Btw - I like the idea of a separate preference as I don't want to mix things with the web service preference/caching that could cause too much confusion. (In reply to Katrin Fischer from comment #26) > (In reply to Paul Hoffman from comment #25) > > I'm worried that a rewrite of the report charting capability is going to > > take some time, and meanwhile our Koha server is liable to run out of memory > > at any time when someone runs a report that generates a lot of results. > > > > How would you all feel about sticking to the original bug fix plan for now > > and reworking charting later? > > Hi Paul, the maintenance releases just happened, so it will take some time > to get a fix into Koha proper (until next month's bugfix release). Using > reports is permission based, maybe inform your staff of the problem? Also > implementing a temporary fix might be an option if you are running your own > installation. > > Btw - I like the idea of a separate preference as I don't want to mix things > with the web service preference/caching that could cause too much confusion. Obsoleted my patches, this should be ready for QA QA Comment: We are ignoring execute_query's parameters: $offset, $limit In Guided.pm you can find code around stripping limits etc. too. And now we are looping thru all results in order to apply the limit. Does not look good to me. Note that the interface should not offer more results than we allow. And should we offer the offset parameter on Reporting ? Minor detail: ReportsMaxRowsInChart: I think that it should go under Tools, not under Webservices. Detail on submitting patches: Processing additional checks * Commit title does not start with 'Bug XXXXX: ' - 2cd0b04b9d We discussed this bug during the Development Meeting and concluded that a simpler fix for now would be to add a system preference to enable/disable charting functionality entirely. In this way we would be able to skip the 'all results' call entirely whilst we work on another implementation to allow fetching of results in a more memory friendly way. (In reply to Paul Hoffman from comment #0) > The attached patch introduces a system preference to limit the number of > rows used when charting report results; at this time, and with bug #23624 > fixed, that is the only aspect of reporting that loads all results into > memory at once. Well, I was totally wrong. Exporting the results (tab-delimited, CSV, or *.ods) also loads all results into memory at once. I am working on patches that broaden the sys pref and use it to limit the rows when exporting. Created attachment 93204 [details] [review] Broaden the scope of the system preference to include exported report results This patch brings the previous set of patches together, renames the system preference, and applies the limit (if set) to exported results. It will require some additional tweaking to insert a prominent note in the UI when the limit has been set. (In reply to Martin Renvoize from comment #30) > We discussed this bug during the Development Meeting and concluded that a > simpler fix for now would be to add a system preference to enable/disable > charting functionality entirely. > > In this way we would be able to skip the 'all results' call entirely whilst > we work on another implementation to allow fetching of results in a more > memory friendly way. Since the problem now turns out to affect exporting as well as charting, I don't think we should add a system preference to enable/disable charting. Or, if we do, it should be a lower priority than this bug. Created attachment 93406 [details] [review] Bug 23626: [alternate] Only fetch full chart data if requested This patchset prevents the full return of report data unless explicitly requested by the user for charting purposes Additionally the user is warned if requesting more than 1000 rows of data To test: 1 - Create a report that returns over 1000 rows of data 2 - Run the report 3 - Note you have two buttons now 'Chart data' and 'Fetch all data for chart' 4 - Click chart data 5 - Note the note that you are only charting visible data 6 - Create the chart and confirm it works 7 - Close the chart 8 - Click 'Fetch all data' 9 - Note the confirm window 10 - Click 'cancel', note there is no change 11 - Repeat and click ok 12 - Fetch all data button is gone 13 - Page to next data, note fetch all does not return 14 - Click 'Chart data' 15 - Note you now have checkbox option to use all data in report 16 - Click it 17 - Create chart 18 - Confirm it works as expected Hi Paul, We talked at the hackfest, people prefer retaining the ability to fetch full data when charting. I have provided an alternate patch that prevents loading the full data on initial report load and adds a button to load the data if requested. There is a warning when fetching over 1000 rows. The button could also be hidden if you never want to use additional rows. Can you test this and confirm it works as needed? The issues with exporting should probably be filed on a new separate bug to assess. Thanks, Nick The sandbox you've requested is not ready. Some problems occurred applying patches from bug 23626: <h1>Something went wrong !</h1>Applying: Limit memory consumption when exporting, too fatal: sha1 information is lacking or useless (installer/data/mysql/atomicupdate/bug_23626.perl). error: could not build fake ancestor Patch failed at 0001 Limit memory consumption when exporting, too (In reply to Séverine Queune from comment #36) > The sandbox you've requested is not ready. > Some problems occurred applying patches from bug 23626: > <h1>Something went wrong !</h1>Applying: Limit memory consumption when > exporting, too > fatal: sha1 information is lacking or useless > (installer/data/mysql/atomicupdate/bug_23626.perl). > error: could not build fake ancestor > Patch failed at 0001 Limit memory consumption when exporting, too I obsoleted the original to allow this to apply on a sandbox Created attachment 93589 [details] [review] Bug 23262: Use correct parameter in URL Created attachment 93720 [details] [review] Bug 23626: [alternate] Only fetch full chart data if requested This patchset prevents the full return of report data unless explicitly requested by the user for charting purposes Additionally the user is warned if requesting more than 1000 rows of data To test: 1 - Create a report that returns over 1000 rows of data 2 - Run the report 3 - Note you have two buttons now 'Chart data' and 'Fetch all data for chart' 4 - Click chart data 5 - Note the note that you are only charting visible data 6 - Create the chart and confirm it works 7 - Close the chart 8 - Click 'Fetch all data' 9 - Note the confirm window 10 - Click 'cancel', note there is no change 11 - Repeat and click ok 12 - Fetch all data button is gone 13 - Page to next data, note fetch all does not return 14 - Click 'Chart data' 15 - Note you now have checkbox option to use all data in report 16 - Click it 17 - Create chart 18 - Confirm it works as expected Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 93723 [details] [review] Bug 23626: [alternate] Only fetch full chart data if requested This patchset prevents the full return of report data unless explicitly requested by the user for charting purposes Additionally the user is warned if requesting more than 1000 rows of data To test: 1 - Create a report that returns over 1000 rows of data 2 - Run the report 3 - Note you have two buttons now 'Chart data' and 'Fetch all data for chart' 4 - Click chart data 5 - Note the note that you are only charting visible data 6 - Create the chart and confirm it works 7 - Close the chart 8 - Click 'Fetch all data' 9 - Note the confirm window 10 - Click 'cancel', note there is no change 11 - Repeat and click ok 12 - Fetch all data button is gone 13 - Page to next data, note fetch all does not return 14 - Click 'Chart data' 15 - Note you now have checkbox option to use all data in report 16 - Click it 17 - Create chart 18 - Confirm it works as expected Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 93729 [details] [review] Bug 23626: [alternate] Only fetch full chart data if requested This patchset prevents the full return of report data unless explicitly requested by the user for charting purposes Additionally the user is warned if requesting more than 1000 rows of data To test: 1 - Create a report that returns over 1000 rows of data 2 - Run the report 3 - Note you have two buttons now 'Chart data' and 'Fetch all data for chart' 4 - Click chart data 5 - Note the note that you are only charting visible data 6 - Create the chart and confirm it works 7 - Close the chart 8 - Click 'Fetch all data' 9 - Note the confirm window 10 - Click 'cancel', note there is no change 11 - Repeat and click ok 12 - Fetch all data button is gone 13 - Page to next data, note fetch all does not return 14 - Click 'Chart data' 15 - Note you now have checkbox option to use all data in report 16 - Click it 17 - Create chart 18 - Confirm it works as expected Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> (In reply to Nick Clemens from comment #35) > The issues with exporting should probably be filed on a new separate bug to > assess. I've reopened bug 23685, which covers the issue with exporting. Created attachment 93914 [details] [review] Bug 23626: [alternate] Only fetch full chart data if requested This patchset prevents the full return of report data unless explicitly requested by the user for charting purposes Additionally the user is warned if requesting more than 1000 rows of data To test: 1 - Create a report that returns over 1000 rows of data 2 - Run the report 3 - Note you have two buttons now 'Chart data' and 'Fetch all data for chart' 4 - Click chart data 5 - Note the note that you are only charting visible data 6 - Create the chart and confirm it works 7 - Close the chart 8 - Click 'Fetch all data' 9 - Note the confirm window 10 - Click 'cancel', note there is no change 11 - Repeat and click ok 12 - Fetch all data button is gone 13 - Page to next data, note fetch all does not return 14 - Click 'Chart data' 15 - Note you now have checkbox option to use all data in report 16 - Click it 17 - Create chart 18 - Confirm it works as expected Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 93915 [details] [review] Bug 23626: [alternate] Default to including all rows If a person is fetching all data, it seems most likely that that person wants to see all the data in the chart. We should default to that behavior. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 93916 [details] [review] Bug 23626: Add missing TT failure Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Nice work! Pushed to master for 19.11.00 Created attachment 93981 [details] [review] Bug 23626: (follow-up) Fix JS when not viewing results Followup pushed.. I just forgot to note it here :) Pushed to 19.05.x for 19.05.05 Moved sponsorship detail to a git follow-up for the release notes (and about page) |