Summary: | GetLostItems in C4::Items.pm has a SELECT * | ||
---|---|---|---|
Product: | Koha | Reporter: | Mark Tompsett <mtompset> |
Component: | Reports | Assignee: | Galen Charlton <gmcharlt> |
Status: | CLOSED FIXED | QA Contact: | |
Severity: | major | ||
Priority: | P5 - low | CC: | chris, jonathan.druart, mtj, paul.poulain |
Version: | 3.8 | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | --- | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: | Circulation function: | ||
Attachments: |
Reduced memory requirements of a massive select statement
Bug 8329 - GetLostItems in C4::Items.pm has a SELECT * Provided a smaller list based on reports/itemlost.tt |
Description
Mark Tompsett
2012-06-28 09:27:39 UTC
Created attachment 10565 [details] [review] Reduced memory requirements of a massive select statement SELECT * joining items, bibioitems, and biblio is bad. The MARC and XML fields will eat memory very quickly. Our server has ridiculously low memory and lots of items marked as lost. We were getting Error 500 pages as output as a result. Since the GetLostItems function is only used by the Items Lost report, I looked at the template in reports/itemslost.tt to get an idea of what might better be suited in the SELECT statement. It was horribly slow, but finally worked. I need to get more than 512MB for our server. Created attachment 11102 [details] [review] Bug 8329 - GetLostItems in C4::Items.pm has a SELECT * Provided a smaller list based on reports/itemlost.tt Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> QA comment: this patch looks good, but a small question: why not SELECT biblio,author,...,items.* (all items fields) ? would that solve your performance problem ? if yest, I think it's better, because it will be more evolutive in case one want to introduce a new column in the report. But I won't fail QA for that, if you say enumerating fields is needed, I'll push your patch (In reply to comment #3) > QA comment: this patch looks good, but a small question: why not > SELECT biblio,author,...,items.* (all items fields) ? would that solve your > performance problem ? if yest, I think it's better, because it will be more > evolutive in case one want to introduce a new column in the report. > > But I won't fail QA for that, if you say enumerating fields is needed, I'll > push your patch ok Paul, all QA tests pass, patch looks good - passing QA :) mason@xen1:~/git/head$ koha-qa.pl * d4f15be Bug 8329 - GetLostItems in C4::Items.pm has a SELECT * Provided a smaller list based on reports/itemlost.tt C4/Items.pm - perlcritic-progressive tests... OK - perl -c syntax tests... OK - xt/tt_valid.t tests... OK - xt/author/valid-template.t tests... OK Pushed to 3.8.x will be in 3.8.4. Re the select *, I have definitely come around to Andy Lesters' school of thought. Select * is a bug waiting to happen. http://petdance.com/2012/07/select-is-a-bug-waiting-to-happen/ Patch pushed. I've only 79 lost items in my setup, the perf improvement is not noticeable. additionnal QA comment: 15:34 ~/koha.dev/koha-community (new/bug_8329 $%)$ grep -R GetLostItems * C4/Items.pm: GetLostItems C4/Items.pm:=head2 GetLostItems C4/Items.pm: $items = GetLostItems( $where, $orderby ); C4/Items.pm: my $items = GetLostItems( $where, "homebranch" ); C4/Items.pm:sub GetLostItems { reports/itemslost.pl: my $items = GetLostItems( \%where, $orderbyfilter ); t/db_dependent/lib/KohaTest/Items.pm: GetLostItems => this sub is used only in reports/itemslost.pl, nothing more to test |