[Koha-patches] [PATCH] bug 5304: GetItemsInfo() - optimise query

Robin Sheat robin at catalyst.net.nz
Mon Oct 18 23:28:44 CEST 2010


From: Srdjan Jankovic <srdjan at catalyst.net.nz>

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.

Signed-off-by: Robin Sheat <robin at catalyst.net.nz>
---
 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<One Order>.
 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 = <<EOS;
+        items.*,
+        biblio.*,
+        issues.*,
+        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,
+        homebranch.branchurl,
+        COALESCE(holdingbranch.branchname, homebranch.branchname) AS branchname,
+        serial.serialseq,
+        serial.publisheddate
+EOS
+    my $itemtypes_join_field = C4::Context->preference('item-level_itypes') ? 'items.itype'
+                                                                            : 'biblioitems.itemtype';
+    my $from = <<EOS;
+items
+LEFT JOIN biblio      USING(biblionumber)
+LEFT JOIN biblioitems USING(biblioitemnumber)
+LEFT OUTER JOIN (SELECT itemnumber, serialseq, publisheddate
+                 FROM serialitems LEFT JOIN serial USING(serialid)) serial
+                      USING(itemnumber)
+LEFT OUTER JOIN (SELECT itemnumber, borrowernumber, cardnumber, surname, firstname, date_due,
+                        borrowers.branchcode AS borrower_branchcode
+                 FROM issues LEFT JOIN borrowers USING(borrowernumber)) issues
+                      USING(itemnumber)
+LEFT JOIN itemtypes   ON   itemtypes.itemtype         = $itemtypes_join_field
+LEFT       JOIN branches homebranch    ON items.homebranch    = homebranch.branchcode
+LEFT OUTER JOIN branches holdingbranch ON items.holdingbranch = holdingbranch.branchcode
+EOS
+    if ($authorised_value_nfl) {
+        $from .= <<EOS;
+LEFT OUTER JOIN authorised_values authorised_values_nfl
+    ON authorised_values_nfl.category = '$authorised_value_nfl'
+    AND authorised_values_nfl.authorised_value = items.notforloan
+EOS
+        $fields .= ", authorised_values_nfl.lib AS notforloanvalue";
+    }
+
+    if ($authorised_value_stack) {
+        $from .= <<EOS;
+LEFT OUTER JOIN authorised_values authorised_values_stack
+    ON authorised_values_nfl.category = '$authorised_value_stack'
+    AND authorised_values_nfl.authorised_value = items.stack
+EOS
+        $fields .= ", authorised_values_stack.lib AS stack";
+    }
+
+    my $query = "SELECT $fields FROM $from WHERE items.biblionumber = ?
+                 ORDER BY homebranch.branchname,items.dateaccessioned desc" ;
     my $sth = $dbh->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<MostImportantReserve>
 
 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<Waiting>.
-
-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<Reserved>.
-
-C<&CheckReserves> returns a two-element list:
-
-C<$status> is either C<Waiting>, C<Reserved> (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<Waiting>.
+
+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<Reserved>.
+
+C<&MostImportantReserve> returns a two-element list:
+
+C<$status> is either C<Waiting>, C<Reserved> (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.7.1



More information about the Koha-patches mailing list