Bug 8329 - GetLostItems in C4::Items.pm has a SELECT *
Summary: GetLostItems in C4::Items.pm has a SELECT *
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low major (vote)
Assignee: Galen Charlton
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-28 09:27 UTC by Mark Tompsett
Modified: 2013-12-05 19:59 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Reduced memory requirements of a massive select statement (1.02 KB, patch)
2012-06-29 14:52 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 8329 - GetLostItems in C4::Items.pm has a SELECT * Provided a smaller list based on reports/itemlost.tt (1.04 KB, patch)
2012-07-24 10:36 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tompsett 2012-06-28 09:27:39 UTC
my $query   = "
        SELECT *
        FROM   items
            LEFT JOIN biblio ON (items.biblionumber = biblio.biblionumber)
            LEFT JOIN biblioitems ON (items.biblionumber = biblioitems.biblionumber)
            LEFT JOIN authorised_values ON (items.itemlost = authorised_values.authorised_value)
        WHERE
                authorised_values.category = 'LOST'
                AND itemlost IS NOT NULL
                AND itemlost <> 0
    ";

We're using 3.6.3, and I comfirmed this is in 3.6.6. This problem would only be obvious if there are many lost items. It is a hellish query as we have 39,000+ "lost" items. We're in the midst of doing data entry/cleanup on a new system.

    my $query   = "
        SELECT title, author, lib, itemlost, authorised_value, barcode, datelastseen, price, replacementprice, homebranch,
               itype, itemtype, holdingbranch, location, itemnotes, items.biblionumber as biblionumber
        FROM   items
            LEFT JOIN biblio ON (items.biblionumber = biblio.biblionumber)
            LEFT JOIN biblioitems ON (items.biblionumber = biblioitems.biblionumber)
            LEFT JOIN authorised_values ON (items.itemlost = authorised_values.authorised_value)
        WHERE
                authorised_values.category = 'LOST'
                AND itemlost IS NOT NULL
                AND itemlost <> 0
";

This is functional, though it can probably be trimmed. The SELECT * includes XML and MARC fields which are sizable and eat system memory!
Comment 1 Mark Tompsett 2012-06-29 14:52:04 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2012-07-24 10:36:40 UTC
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>
Comment 3 Paul Poulain 2012-08-03 15:47:19 UTC
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
Comment 4 Mason James 2012-08-10 05:12:19 UTC
(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
Comment 5 Chris Cormack 2012-08-10 19:47:06 UTC
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/
Comment 6 Paul Poulain 2012-08-29 13:35:48 UTC
Patch pushed. I've only 79 lost items in my setup, the perf improvement is not noticeable.
Comment 7 Paul Poulain 2012-08-29 13:36:56 UTC
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