Attachment 79227 Details for Bug 20664 – Bug 20664: Optimize retrieval of biblio and item data

[patch] Bug 20664: Optimize retrieval of biblio and item data

Bug-20664-Optimize-retrieval-of-biblio-and-item-da.patch (text/plain), 14.93 KB, created by Ere Maijala on 2018-09-21 12:03:42 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Ere Maijala

Created: 2018-09-21 12:03:42 UTC

Size: 14.93 KB

patch

obsolete

>From 48219ae96dfe914786848b28ee998423b53d24b4 Mon Sep 17 00:00:00 2001
>From: Ere Maijala <ere.maijala@helsinki.fi>
>Date: Fri, 21 Sep 2018 14:43:38 +0300
>Subject: [PATCH] Bug 20664: Optimize retrieval of biblio and item data
>
>Optimizes embedding of item data in MARC and fixes several bottlenecks encountered while profiling OAI-PMH and exporting of records. There are two ways this is accomplished:
>
>1.) Create optimized method for fetching item fields for MARC embedding.
>2.) Use the cache service more and where repeated calls are made.
>
>Test plan:
>
>1.) Before applying the patch, time an export_records.pl run for a set of biblios that also have items. Run it a couple of times to account for initial slowness and possible fluctuations. For example:
>
>time misc/export_records.pl --record-type bibs --starting_biblionumber 960000 --ending_biblionumber 965000 --format xml --filename unoptimized.xml
>
>2.) Apply the patch.
>
>3.) Time the export process again with a different output file:
>
>time misc/export_records.pl --record-type bibs --starting_biblionumber 960000 --ending_biblionumber 965000 --format xml --filename optimized.xml
>
>4.) Verify that the optimized process is faster.
>
>5.) Compare the resulting export files to make sure they're identical:
>---
> C4/Biblio.pm                           |  87 ++++++++++++--------------
> C4/Items.pm                            | 110 +++++++++++++++++++++++++++++----
> misc/migration_tools/bulkmarcimport.pl |   2 +-
> t/db_dependent/Items.t                 |   9 +--
> 4 files changed, 142 insertions(+), 66 deletions(-)
>
>diff --git a/C4/Biblio.pm b/C4/Biblio.pm
>index 153241f..4b89cea 100644
>--- a/C4/Biblio.pm
>+++ b/C4/Biblio.pm
>@@ -310,7 +310,7 @@ sub ModBiblio {
> 
>     $frameworkcode = "" if !$frameworkcode || $frameworkcode eq "Default"; # XXX
> 
>-    _strip_item_fields($record, $frameworkcode);
>+    _strip_item_fields($record);
> 
>     # update biblionumber and biblioitemnumber in MARC
>     # FIXME - this is assuming a 1 to 1 relationship between
>@@ -344,7 +344,7 @@ sub ModBiblio {
> 
> =head2 _strip_item_fields
> 
>-  _strip_item_fields($record, $frameworkcode)
>+  _strip_item_fields($record)
> 
> Utility routine to remove item tags from a
> MARC bib.
>@@ -353,9 +353,8 @@ MARC bib.
> 
> sub _strip_item_fields {
>     my $record = shift;
>-    my $frameworkcode = shift;
>     # get the items before and append them to the biblio before updating the record, atm we just have the biblio
>-    my ( $itemtag, $itemsubfield ) = GetMarcFromKohaField( "items.itemnumber", $frameworkcode );
>+    my ( $itemtag, $itemsubfield ) = GetMarcFromKohaField( "items.itemnumber" );
> 
>     # delete any item fields from incoming record to avoid
>     # duplication or incorrect data - use AddItem() or ModItem()
>@@ -1089,14 +1088,22 @@ sub GetMarcSubfieldStructure {
> sub GetMarcFromKohaField {
>     my ( $kohafield ) = @_;
>     return unless $kohafield;
>-    # The next call uses the Default framework since it is AUTHORITATIVE
>-    # for all Koha to MARC mappings.
>-    my $mss = GetMarcSubfieldStructure( '', { unsafe => 1 } ); # Do not change framework
>-    my @retval;
>-    foreach( @{ $mss->{$kohafield} } ) {
>-        push @retval, $_->{tagfield}, $_->{tagsubfield};
>-    }
>-    return wantarray ? @retval : ( @retval ? $retval[0] : undef );
>+
>+    my $cache     = Koha::Caches->get_instance();
>+    my $cache_key = "MarcFromKohaField-$kohafield";
>+    my $cached    = $cache->get_from_cache($cache_key);
>+    if ( !defined $cached ) {
>+        # The next call uses the Default framework since it is AUTHORITATIVE
>+        # for all Koha to MARC mappings.
>+        my $mss = GetMarcSubfieldStructure( '', { unsafe => 1 } ); # Do not change framework
>+        my @retval;
>+        foreach( @{ $mss->{$kohafield} } ) {
>+            push @retval, $_->{tagfield}, $_->{tagsubfield};
>+        }
>+        $cached = \@retval;
>+        $cache->set_in_cache( $cache_key, $cached );
>+    }
>+    return wantarray ? @$cached : ( @$cached ? $cached->[0] : undef );
> }
> 
> =head2 GetMarcSubfieldStructureFromKohaField
>@@ -2804,7 +2811,7 @@ override if necessary.
> 
> sub EmbedItemsInMarcBiblio {
>     my ($params) = @_;
>-    my ($marc, $biblionumber, $itemnumbers, $opac, $borcat);
>+    my ($marc, $biblionumber, $itemnumbers, $use_hidingrules, $borcat);
>     $marc = $params->{marc_record};
>     if ( !$marc ) {
>         carp 'EmbedItemsInMarcBiblio: No MARC record passed';
>@@ -2812,49 +2819,33 @@ sub EmbedItemsInMarcBiblio {
>     }
>     $biblionumber = $params->{biblionumber};
>     $itemnumbers = $params->{item_numbers};
>-    $opac = $params->{opac};
>+    $use_hidingrules = $params->{opac};
>     $borcat = $params->{borcat} // q{};
> 
>     $itemnumbers = [] unless defined $itemnumbers;
> 
>-    my $frameworkcode = GetFrameworkCode($biblionumber);
>-    _strip_item_fields($marc, $frameworkcode);
>+    _strip_item_fields($marc);
> 
>-    # ... and embed the current items
>-    my $dbh = C4::Context->dbh;
>-    my $sth = $dbh->prepare("SELECT itemnumber FROM items WHERE biblionumber = ?");
>-    $sth->execute($biblionumber);
>-    my ( $itemtag, $itemsubfield ) = GetMarcFromKohaField( "items.itemnumber", $frameworkcode );
>+    my $hidingrules = {};
>+    if ($use_hidingrules && $borcat && !UseHidingRulesWithBorrowerCategory($borcat)) {
>+        # don't hide anything from this borrower category
>+        $use_hidingrules = 0;
>+    }
> 
>-    my @item_fields; # Array holding the actual MARC data for items to be included.
>-    my @items;       # Array holding items which are both in the list (sitenumbers)
>-                     # and on this biblionumber
>+    my $yaml = $use_hidingrules ? C4::Context->preference('OpacHiddenItems') : '';
>+    if ( $yaml =~ /\S/ ) {
>+        $yaml = "$yaml\n\n"; # YAML is anal on ending \n. Surplus does not hurt
>+        eval {
>+            $hidingrules = YAML::Load($yaml);
>+        };
>+        if ($@) {
>+            carp "Unable to parse OpacHiddenItems syspref : $@";
>+        }
>+    }
> 
>-    # Flag indicating if there is potential hiding.
>-    my $opachiddenitems = $opac
>-      && ( C4::Context->preference('OpacHiddenItems') !~ /^\s*$/ );
>+    my $item_fields = C4::Items::GetMarcItemFields( $biblionumber, $itemnumbers, $hidingrules );
> 
>-    require C4::Items;
>-    while ( my ($itemnumber) = $sth->fetchrow_array ) {
>-        next if @$itemnumbers and not grep { $_ == $itemnumber } @$itemnumbers;
>-        my $i = $opachiddenitems ? C4::Items::GetItem($itemnumber) : undef;
>-        push @items, { itemnumber => $itemnumber, item => $i };
>-    }
>-    my @items2pass = map { $_->{item} } @items;
>-    my @hiddenitems =
>-      $opachiddenitems
>-      ? C4::Items::GetHiddenItemnumbers({
>-            items  => \@items2pass,
>-            borcat => $borcat })
>-      : ();
>-    # Convert to a hash for quick searching
>-    my %hiddenitems = map { $_ => 1 } @hiddenitems;
>-    foreach my $itemnumber ( map { $_->{itemnumber} } @items ) {
>-        next if $hiddenitems{$itemnumber};
>-        my $item_marc = C4::Items::GetMarcItem( $biblionumber, $itemnumber );
>-        push @item_fields, $item_marc->field($itemtag);
>-    }
>-    $marc->append_fields(@item_fields);
>+    $marc->append_fields(@$item_fields) if ( @$item_fields );
> }
> 
> =head1 INTERNAL FUNCTIONS
>diff --git a/C4/Items.pm b/C4/Items.pm
>index e46737d..66815af 100644
>--- a/C4/Items.pm
>+++ b/C4/Items.pm
>@@ -74,6 +74,7 @@ BEGIN {
> 	GetHostItemsInfo
> 	get_hostitemnumbers_of
>         GetHiddenItemnumbers
>+        UseHidingRulesWithBorrowerCategory
>         ItemSafeToDelete
>         DelItemCheck
>     MoveItemFromBiblio
>@@ -1299,12 +1300,10 @@ to be excluded
> sub GetHiddenItemnumbers {
>     my $params = shift;
>     my $items = $params->{items};
>-    if (my $exceptions = C4::Context->preference('OpacHiddenItemsExceptions') and $params->{'borcat'}){
>-        foreach my $except (split(/\|/, $exceptions)){
>-            if ($params->{'borcat'} eq $except){
>-                return; # we dont hide anything for this borrower category
>-            }
>-        }
>+
>+    if ($params->{'borcat'} && !UseHidingRulesWithBorrowerCategory($params->{'borcat'})) {
>+        # Don't hide anything from this borrower category
>+        return;
>     }
>     my @resultitems;
> 
>@@ -1350,6 +1349,27 @@ sub GetHiddenItemnumbers {
>     return @resultitems;
> }
> 
>+=head2 UseHidingRulesWithBorrowerCategory
>+
>+    if (UseHidingRulesWithBorrowerCategory($category}) { ... }
>+
>+Checks if item hiding rules should be used with the given borrower category.
>+
>+=cut
>+
>+sub UseHidingRulesWithBorrowerCategory {
>+    my ($borcat) = @_;
>+
>+    if ($borcat && (my $exceptions = C4::Context->preference('OpacHiddenItemsExceptions'))) {
>+        foreach my $except (split(/\|/, $exceptions)) {
>+            if ($borcat eq $except) {
>+                return 0;
>+            }
>+        }
>+    }
>+    return 1;
>+}
>+
> =head1 LIMITED USE FUNCTIONS
> 
> The following functions, while part of the public API,
>@@ -1419,6 +1439,73 @@ sub Item2Marc {
> 	return $itemmarc;
> }
> 
>+=head2 GetMarcItemFields
>+
>+  my @marc_fields = GetMarcItemFields($biblionumber);
>+
>+Returns an array of MARC::Record objects of the items for the biblio.
>+
>+=cut
>+
>+sub GetMarcItemFields {
>+	my ( $biblionumber, $itemnumbers, $hidingrules ) = @_;
>+
>+    my $item_level_itype = C4::Context->preference('item-level_itypes');
>+    # This is so much faster than using Koha::Items->search that it makes sense even if it's ugly.
>+    my $query = 'SELECT * FROM items WHERE biblionumber = ?';
>+    if (@$itemnumbers) {
>+        $query .= ' AND itemnumber IN (' . join(',', @$itemnumbers) . ')';
>+    }
>+    my $sth = C4::Context->dbh->prepare($query);
>+    $sth->execute($biblionumber);
>+    my $items = $sth->fetchall_arrayref({});
>+    $sth->finish();
>+    my @item_fields;
>+    my ($itemtag, $itemsubfield) = GetMarcFromKohaField('items.itemnumber');
>+
>+    ITEMLOOP: foreach my $item (@$items) {
>+
>+        # Check hiding rules
>+        if (defined $hidingrules) {
>+            foreach my $field (keys %$hidingrules) {
>+                my $val = $item->{$field};
>+                $val = '' unless defined $val;
>+
>+                # If the results matches the values in the hiding rules, skip the item
>+                if (any { $val eq $_ } @{$hidingrules->{$field}}) {
>+                    next ITEMLOOP;
>+                }
>+            }
>+        }
>+
>+        # Set correct item type
>+        if (!$item_level_itype || !$item->{itype}) {
>+            warn 'item-level_itypes set but no itemtype set for item (' . $item->{itemnumber} . ')' if (!$item->{itype});
>+            my $biblioitem = Koha::Biblioitems->find($item->{biblioitemnumber});
>+            $item->{itype} = $biblioitem->itemtype();
>+        }
>+
>+        my $mungeditem = {
>+            map {
>+                defined($item->{$_}) && $item->{$_} ne '' ? ("items.$_" => $item->{$_}) : ()
>+            } keys %{ $item }
>+        };
>+        my $itemmarc = TransformKohaToMarc($mungeditem);
>+
>+        my $unlinked_item_subfields = _parse_unlinked_item_subfields_from_xml($mungeditem->{'items.more_subfields_xml'});
>+        if (defined $unlinked_item_subfields and $#$unlinked_item_subfields > -1) {
>+            foreach my $field ($itemmarc->field($itemtag)){
>+                $field->add_subfields(@$unlinked_item_subfields);
>+            }
>+        }
>+
>+        push @item_fields, $itemmarc->field($itemtag);
>+    }
>+
>+    return \@item_fields;
>+}
>+
>+
> =head1 PRIVATE FUNCTIONS AND VARIABLES
> 
> The following functions are not meant to be called
>@@ -1555,10 +1642,11 @@ sub _do_column_fixes_for_mod {
>         (not defined $item->{'withdrawn'} or $item->{'withdrawn'} eq '')) {
>         $item->{'withdrawn'} = 0;
>     }
>-    if (exists $item->{location}
>-        and $item->{location} ne 'CART'
>-        and $item->{location} ne 'PROC'
>-        and not $item->{permanent_location}
>+    if (exists $item->{'location'}
>+        and defined $item->{'location'}
>+        and $item->{'location'} ne 'CART'
>+        and $item->{'location'} ne 'PROC'
>+        and (not defined $item->{'permanent_location'} or not $item->{'permanent_location'})
>     ) {
>         $item->{'permanent_location'} = $item->{'location'};
>     }
>@@ -2223,7 +2311,7 @@ sub _SearchItems_build_where_fragment {
>         push @columns, Koha::Database->new()->schema()->resultset('Biblioitem')->result_source->columns;
>         my @operators = qw(= != > < >= <= like);
>         my $field = $filter->{field};
>-        if ( (0 < grep /^$field$/, @columns) or (substr($field, 0, 5) eq 'marc:') ) {
>+        if ( defined $field and ((0 < grep /^$field$/, @columns) or (substr($field, 0, 5) eq 'marc:')) ) {
>             my $op = $filter->{operator};
>             my $query = $filter->{query};
> 
>diff --git a/misc/migration_tools/bulkmarcimport.pl b/misc/migration_tools/bulkmarcimport.pl
>index d820d3e..333b3da 100755
>--- a/misc/migration_tools/bulkmarcimport.pl
>+++ b/misc/migration_tools/bulkmarcimport.pl
>@@ -486,7 +486,7 @@ RECORD: while (  ) {
>             # Work on a clone so that if there are real errors, we can maybe
>             # fix them up later.
> 			my $clone_record = $record->clone();
>-            C4::Biblio::_strip_item_fields($clone_record, '');
>+            C4::Biblio::_strip_item_fields($clone_record);
>             # This sets the marc fields if there was an error, and also calls
>             # defer_marc_save.
>             ModBiblioMarc( $clone_record, $biblionumber, $framework );
>diff --git a/t/db_dependent/Items.t b/t/db_dependent/Items.t
>index 6ae721a..0ed0422 100755
>--- a/t/db_dependent/Items.t
>+++ b/t/db_dependent/Items.t
>@@ -690,7 +690,7 @@ subtest 'C4::Items::_build_default_values_for_mod_marc' => sub {
>     Koha::MarcSubfieldStructure->new({ frameworkcode => '', tagfield => '952', tagsubfield => '9', kohafield => 'items.itemnumber' })->store;
>     Koha::MarcSubfieldStructure->new({ frameworkcode => '', tagfield => '952', tagsubfield => 'p', kohafield => 'items.barcode' })->store;
>     Koha::MarcSubfieldStructure->new({ frameworkcode => '', tagfield => '952', tagsubfield => 'y', kohafield => 'items.itype' })->store;
>-    Koha::Caches->get_instance->clear_from_cache( "MarcSubfieldStructure-" );
>+    Koha::Caches->get_instance()->flush_all();
> 
>     my $itemtype = $builder->build({ source => 'Itemtype' })->{itemtype};
> 
>@@ -731,9 +731,7 @@ subtest 'C4::Items::_build_default_values_for_mod_marc' => sub {
>     Koha::MarcSubfieldStructures->search({ frameworkcode => '', tagfield => '952', tagsubfield => 'p' })->delete;
> 
>     # And make sure the caches are cleared
>-    my $cache = Koha::Caches->get_instance();
>-    $cache->clear_from_cache("default_value_for_mod_marc-");
>-    $cache->clear_from_cache("MarcSubfieldStructure-");
>+    Koha::Caches->get_instance()->flush_all();
> 
>     # Update the MARC field with another value
>     $item_record->delete_fields( $barcode_field );
>@@ -748,8 +746,7 @@ subtest 'C4::Items::_build_default_values_for_mod_marc' => sub {
>     $item = GetItem($item_itemnumber);
>     is ( $item->{barcode}, $a_barcode, 'items.barcode is not mapped anymore, so the DB column has not been updated' );
> 
>-    $cache->clear_from_cache("default_value_for_mod_marc-");
>-    $cache->clear_from_cache( "MarcSubfieldStructure-" );
>+    Koha::Caches->get_instance()->flush_all();
>     $schema->storage->txn_rollback;
> };
> 
>-- 
>2.7.4

Actions: View | Diff | Splinter Review

Attachments on bug 20664: 74873 | 75968 | 76045 | 76046 | 76669 | 78868 | 78869 | 78870 | 79225 | 79227 | 79228 | 79229 | 79276 | 79277 | 79278 | 79279 | 84516 | 84517 | 84518 | 84519 | 84520 | 85106 | 85107 | 85108 | 85109 | 85110 | 85776 | 85777 | 85778 | 85779 | 85780 | 85781 | 85898 | 85899 | 85900 | 85901 | 85902 | 85903 | 85904 | 85905 | 86002 | 86003 | 86004 | 86005 | 86006 | 86007 | 86008 | 86009