From srdjan@catalyst.net.nz Mon Oct 18 15:23:44 2010 Return-Path: X-Original-To: robin@catalyst.net.nz Delivered-To: robin@catalyst.net.nz Received: from localhost (localhost [127.0.0.1]) by mail.catalyst.net.nz (Postfix) with ESMTP id 354BE31E18; Mon, 18 Oct 2010 15:23:44 +1300 (NZDT) X-Virus-Scanned: Debian amavisd-new at catalyst.net.nz Received: from mail.catalyst.net.nz ([127.0.0.1]) by localhost (bertrand.catalyst.net.nz [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id n6+eyr+h-UWc; Mon, 18 Oct 2010 15:23:41 +1300 (NZDT) Received: from stuffdev.wgtn.cat-it.co.nz (unknown [IPv6:2404:130:0:1000:205:1cff:fe1d:c4e2]) by mail.catalyst.net.nz (Postfix) with ESMTPS id D8D7A31C73; Mon, 18 Oct 2010 15:23:41 +1300 (NZDT) Received: from srdjan by stuffdev.wgtn.cat-it.co.nz with local (Exim 4.69) (envelope-from ) id 1P7fP5-0001KR-3M; Mon, 18 Oct 2010 15:24:47 +1300 From: Srdjan Jankovic To: robin@catalyst.net.nz Cc: Srdjan Jankovic Subject: [PATCH] bug 5304: GetItemsInfo() - optimise query Date: Mon, 18 Oct 2010 15:24:39 +1300 Message-Id: <1287368679-5076-1-git-send-email-srdjan@catalyst.net.nz> X-Mailer: git-send-email 1.6.5 X-Evolution-Source: imap://robin@mail.catalyst.net.nz/ Content-Transfer-Encoding: 8bit Mime-Version: 1.0 GetItemsInfo(): combine queries so we get as mch as possible in one go. CheckReserves() - extract MostImportantReserve((), so we don't need to execute items query if we already have details Create index (biblionumber, biblioitemnumber) on reserveconstraints. --- C4/Items.pm | 210 +++++++++++++++----------------- C4/Reserves.pm | 55 ++++++--- installer/data/mysql/kohastructure.sql | 3 +- installer/data/mysql/updatedatabase.pl | 7 + 4 files changed, 145 insertions(+), 130 deletions(-) diff --git a/C4/Items.pm b/C4/Items.pm index 794bff7..5366222 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -2,6 +2,8 @@ package C4::Items; # Copyright 2007 LibLime, Inc. # +# Parts copyright Catalyst IT 2010 +# # This file is part of Koha. # # Koha is free software; you can redistribute it and/or modify it under the @@ -1168,130 +1170,119 @@ If this is set, it is set to C. sub GetItemsInfo { my ( $biblionumber, $type ) = @_; my $dbh = C4::Context->dbh; + + # get notforloan complete status if applicable + my $sthnflstatus = $dbh->prepare( + "SELECT authorised_value + FROM marc_subfield_structure + WHERE kohafield='items.notforloan' + " + ); + $sthnflstatus->execute; + my ($authorised_value_nfl) = $sthnflstatus->fetchrow; + + # my stack procedures + my $stackstatus = $dbh->prepare( + 'SELECT authorised_value + FROM marc_subfield_structure + WHERE kohafield="items.stack" + ' + ); + $stackstatus->execute; + my ($authorised_value_stack) = $stackstatus->fetchrow; + + my $user_branch; + if (C4::Context->preference("IndependantBranches")){ + my $userenv = C4::Context->userenv; + $user_branch = $userenv->{branch} if $userenv && ( $userenv->{flags} % 2 != 1 ); + } + # note biblioitems.* must be avoided to prevent large marc and marcxml fields from killing performance. - my $query = " - SELECT items.*, - biblio.*, - biblioitems.volume, - biblioitems.number, - biblioitems.itemtype, - biblioitems.isbn, - biblioitems.issn, - biblioitems.publicationyear, - biblioitems.publishercode, - biblioitems.volumedate, - biblioitems.volumedesc, - biblioitems.lccn, - biblioitems.url, - items.notforloan as itemnotforloan, - itemtypes.description, - itemtypes.notforloan as notforloan_per_itemtype, - branchurl - FROM items - LEFT JOIN branches ON items.homebranch = branches.branchcode - LEFT JOIN biblio ON biblio.biblionumber = items.biblionumber - LEFT JOIN biblioitems ON biblioitems.biblioitemnumber = items.biblioitemnumber - LEFT JOIN itemtypes ON itemtypes.itemtype = " - . (C4::Context->preference('item-level_itypes') ? 'items.itype' : 'biblioitems.itemtype'); - $query .= " WHERE items.biblionumber = ? ORDER BY branches.branchname,items.dateaccessioned desc" ; + my $fields = <preference('item-level_itypes') ? 'items.itype' + : 'biblioitems.itemtype'; + my $from = <prepare($query); $sth->execute($biblionumber); - my $i = 0; + my @results; my $serial; - - my $isth = $dbh->prepare( - "SELECT issues.*,borrowers.cardnumber,borrowers.surname,borrowers.firstname,borrowers.branchcode as bcode - FROM issues LEFT JOIN borrowers ON issues.borrowernumber=borrowers.borrowernumber - WHERE itemnumber = ?" - ); - my $ssth = $dbh->prepare("SELECT serialseq,publisheddate from serialitems left join serial on serialitems.serialid=serial.serialid where serialitems.itemnumber=? "); while ( my $data = $sth->fetchrow_hashref ) { - my $datedue = ''; + my $datedue = $data->{'date_due'}; my $count_reserves; - $isth->execute( $data->{'itemnumber'} ); - if ( my $idata = $isth->fetchrow_hashref ) { - $data->{borrowernumber} = $idata->{borrowernumber}; - $data->{cardnumber} = $idata->{cardnumber}; - $data->{surname} = $idata->{surname}; - $data->{firstname} = $idata->{firstname}; - $datedue = $idata->{'date_due'}; - if (C4::Context->preference("IndependantBranches")){ - my $userenv = C4::Context->userenv; - if ( ($userenv) && ( $userenv->{flags} % 2 != 1 ) ) { - $data->{'NOTSAMEBRANCH'} = 1 if ($idata->{'bcode'} ne $userenv->{branch}); - } - } - } - if ( $data->{'serial'}) { - $ssth->execute($data->{'itemnumber'}) ; - ($data->{'serialseq'} , $data->{'publisheddate'}) = $ssth->fetchrow_array(); - $serial = 1; - } - if ( $datedue eq '' ) { + + $data->{'NOTSAMEBRANCH'} = $data->{'borrower_branchcode'} ne $user_branch + if $data->{'borrower_branchcode'} && $user_branch; + + $serial = 1 if $data->{'serial'}; + + unless ( $datedue ) { my ( $restype, $reserves ) = - C4::Reserves::CheckReserves( $data->{'itemnumber'} ); + C4::Reserves::MostImportantReserve( $data->{'biblioitemnumber'}, $data->{'biblionumber'}, $data->{'itemnumber'} ); # Previous conditional check with if ($restype) is not needed because a true # result for one item will result in subsequent items defaulting to this true # value. $count_reserves = $restype; } - #get branch information..... - my $bsth = $dbh->prepare( - "SELECT * FROM branches WHERE branchcode = ? - " - ); - $bsth->execute( $data->{'holdingbranch'} ); - if ( my $bdata = $bsth->fetchrow_hashref ) { - $data->{'branchname'} = $bdata->{'branchname'}; - } $data->{'datedue'} = $datedue; $data->{'count_reserves'} = $count_reserves; - # get notforloan complete status if applicable - my $sthnflstatus = $dbh->prepare( - 'SELECT authorised_value - FROM marc_subfield_structure - WHERE kohafield="items.notforloan" - ' - ); - - $sthnflstatus->execute; - my ($authorised_valuecode) = $sthnflstatus->fetchrow; - if ($authorised_valuecode) { - $sthnflstatus = $dbh->prepare( - "SELECT lib FROM authorised_values - WHERE category=? - AND authorised_value=?" - ); - $sthnflstatus->execute( $authorised_valuecode, - $data->{itemnotforloan} ); - my ($lib) = $sthnflstatus->fetchrow; - $data->{notforloanvalue} = $lib; - } - - # my stack procedures - my $stackstatus = $dbh->prepare( - 'SELECT authorised_value - FROM marc_subfield_structure - WHERE kohafield="items.stack" - ' - ); - $stackstatus->execute; - - ($authorised_valuecode) = $stackstatus->fetchrow; - if ($authorised_valuecode) { - $stackstatus = $dbh->prepare( - "SELECT lib - FROM authorised_values - WHERE category=? - AND authorised_value=? - " - ); - $stackstatus->execute( $authorised_valuecode, $data->{stack} ); - my ($lib) = $stackstatus->fetchrow; - $data->{stack} = $lib; - } # Find the last 3 people who borrowed this item. my $sth2 = $dbh->prepare("SELECT * FROM old_issues,borrowers WHERE itemnumber = ? @@ -1307,8 +1298,7 @@ sub GetItemsInfo { $ii++; } - $results[$i] = $data; - $i++; + push @results, $data; } if($serial) { return( sort { ($b->{'publisheddate'} || $b->{'enumchron'}) cmp ($a->{'publisheddate'} || $a->{'enumchron'}) } @results ); diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 9b7014e..dc514e1 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -797,26 +797,10 @@ sub GetReserveStatus { ($status, $reserve) = &CheckReserves($itemnumber); ($status, $reserve) = &CheckReserves(undef, $barcode); -Find a book in the reserves. +Find the book by its itemnumber or barcode, and call C C<$itemnumber> is the book's item number. - -As I understand it, C<&CheckReserves> looks for the given item in the -reserves. If it is found, that's a match, and C<$status> is set to -C. - -Otherwise, it finds the most important item in the reserves with the -same biblio number as this book (I'm not clear on this) and returns it -with C<$status> set to C. - -C<&CheckReserves> returns a two-element list: - -C<$status> is either C, C (see above), or 0. - -C<$reserve> is the reserve item that matched. It is a -reference-to-hash whose keys are mostly the fields of the reserves -table in the Koha database. - +C<$barcode> is the book's barcode =cut sub CheckReserves { @@ -851,6 +835,39 @@ sub CheckReserves { # execpt where items.notforloan < 0 : This indicates the item is holdable. return ( 0, 0 ) if ( $notforloan_per_item > 0 ) or $notforloan_per_itemtype; + return MostImportantReserve($bibitem, $biblio, $itemnumber); +} + +=head2 MostImportantReserve + + ($status, $reserve) = &MostImportantReserve($itemnumber); + +Find the most important reserve for a book. + +C<$bibitem> is the book's biblioitem number. +C<$biblio> is the book's biblio number. +C<$itemnumber> is the book's item number. + +As I understand it, C<&MostImportantReserve> looks for the given item in the +reserves. If it is found, that's a match, and C<$status> is set to +C. + +Otherwise, it finds the most important item in the reserves with the +same biblio number as this book (I'm not clear on this) and returns it +with C<$status> set to C. + +C<&MostImportantReserve> returns a two-element list: + +C<$status> is either C, C (see above), or 0. + +C<$reserve> is the reserve item that matched. It is a +reference-to-hash whose keys are mostly the fields of the reserves +table in the Koha database. + +=cut + +sub MostImportantReserve { + my ( $bibitem, $biblio, $itemnumber ) = @_; # Find this item in the reserves my @reserves = _Findgroupreserve( $bibitem, $biblio, $itemnumber ); @@ -877,7 +894,7 @@ sub CheckReserves { # If we get this far, then no exact match was found. # We return the most important (i.e. next) reservation. if ($highest) { - $highest->{'itemnumber'} = $item; + $highest->{'itemnumber'} = $itemnumber; return ( "Reserved", $highest ); } else { diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index e42652c..b04a9c5 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -1517,7 +1517,8 @@ CREATE TABLE `reserveconstraints` ( `reservedate` date default NULL, `biblionumber` int(11) NOT NULL default 0, `biblioitemnumber` int(11) default NULL, - `timestamp` timestamp NOT NULL default CURRENT_TIMESTAMP on update CURRENT_TIMESTAMP + `timestamp` timestamp NOT NULL default CURRENT_TIMESTAMP on update CURRENT_TIMESTAMP, + KEY `biblio` (`biblionumber`, `biblioitemnumber`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8; -- diff --git a/installer/data/mysql/updatedatabase.pl b/installer/data/mysql/updatedatabase.pl index 2bd4e21..3fb576f 100755 --- a/installer/data/mysql/updatedatabase.pl +++ b/installer/data/mysql/updatedatabase.pl @@ -3744,6 +3744,13 @@ if (C4::Context->preference("Version") < TransformToNum($DBversion)) { SetVersion ($DBversion); } +$DBversion = "3.01.00.XXX"; +if (C4::Context->preference("Version") < TransformToNum($DBversion)) { + $dbh->do("CREATE INDEX `reserveconstraints_biblio_idx` ON reserveconstraints (`biblionumber`, `biblioitemnumber`) "); + print "Upgrade to $DBversion done (reserveconstraints_biblio_idx index added)\n"; + SetVersion ($DBversion); +} + =item DropAllForeignKeys($table) -- 1.6.5