From b4ae93f3d9704da75b0cda96710913ca385b403c Mon Sep 17 00:00:00 2001 From: Ere Maijala Date: Tue, 28 Nov 2017 10:14:20 +0200 Subject: [PATCH] Bug 20664: Optimize retrieval of biblio and item data https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20664 Optimizes embedding of item data in MARC and fixes several bottlenecks encountered while profiling OAI-PMH and exporting of records. There are two changes to accomplish this: 1.) Create optimized method for fetching item fields for MARC embedding. 2.) Use the cache service more and where repeated calls are made. Also the now-unnecessary frameworkcode parameter to _strip_item_fields() has been removed. 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: diff -u unoptimized.xml optimized.xml --- C4/Biblio.pm | 76 ++++++++++++++++------------------ C4/Items.pm | 76 +++++++++++++++++++++++++++++++--- misc/migration_tools/bulkmarcimport.pl | 2 +- t/db_dependent/Items.t | 9 ++-- 4 files changed, 111 insertions(+), 52 deletions(-) diff --git a/C4/Biblio.pm b/C4/Biblio.pm index 0c5a917..7ef7b01 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 @@ -2792,36 +2799,25 @@ sub EmbedItemsInMarcBiblio { $itemnumbers = [] unless defined $itemnumbers; - my $frameworkcode = GetFrameworkCode($biblionumber); - _strip_item_fields($marc, $frameworkcode); + _strip_item_fields($marc); + + my $hidingrules = {}; + my $yaml = $opac ? 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 : $@"; + } + } - # ... and embed the current items - my $dbh = C4::Context->dbh; - my $sth = $dbh->prepare("SELECT itemnumber FROM items WHERE biblionumber = ?"); - $sth->execute($biblionumber); - my @item_fields; - my ( $itemtag, $itemsubfield ) = GetMarcFromKohaField( "items.itemnumber", $frameworkcode ); - my @items; - my $opachiddenitems = $opac - && ( C4::Context->preference('OpacHiddenItems') !~ /^\s*$/ ); 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 @hiddenitems = - $opachiddenitems - ? C4::Items::GetHiddenItemnumbers( map { $_->{item} } @items ) - : (); - # 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); + + my $item_fields = C4::Items::GetMarcItemFields( $biblionumber, $itemnumbers, $hidingrules ); + + $marc->append_fields(@$item_fields) if ( @$item_fields ); } =head1 INTERNAL FUNCTIONS diff --git a/C4/Items.pm b/C4/Items.pm index 3f68375..37a6930 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -1410,6 +1410,71 @@ 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 $sth = C4::Context->dbh->prepare('SELECT * FROM items WHERE biblionumber = ?'); + $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 itemnumbers + next if (@$itemnumbers && !any { $_ == $item->{itemnumber} } @$itemnumbers); + + # 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 @@ -1546,10 +1611,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'}; } @@ -2214,7 +2280,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 92151d6..af71144 100755 --- a/t/db_dependent/Items.t +++ b/t/db_dependent/Items.t @@ -664,7 +664,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}; @@ -705,9 +705,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 ); @@ -722,8 +720,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