From 86f6774a7063073531713f5952ee4eb6b5dedd51 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 Optimizes embedding of item data in MARC and fixes several bottlenecks encountered while profiling OAI-PMH and exporting of records. There are three ways this is accomplished: 1.) Use state variables to hold prepared SQL statements so that the preparation is done only once. 2.) Create optimized method for fetching item fields for MARC embedding. 3.) 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: diff -u unoptimized.xml optimized.xml --- C4/Biblio.pm | 94 ++++++++++++++++++++++++++------------------------ C4/Items.pm | 77 ++++++++++++++++++++++++++++++++++++++--- t/db_dependent/Items.t | 9 ++--- 3 files changed, 124 insertions(+), 56 deletions(-) diff --git a/C4/Biblio.pm b/C4/Biblio.pm index eaff256..0aa00b1 100644 --- a/C4/Biblio.pm +++ b/C4/Biblio.pm @@ -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. @@ -355,7 +355,7 @@ 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() @@ -1053,14 +1053,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( '' ); # 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( '' ); # 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 @@ -1137,10 +1145,11 @@ sub GetMarcBiblio { return; } - my $dbh = C4::Context->dbh; - my $sth = $dbh->prepare("SELECT biblioitemnumber FROM biblioitems WHERE biblionumber=? "); + # Use state to speed up repeated calls in batch processes + state $sth = C4::Context->dbh->prepare("SELECT biblioitemnumber FROM biblioitems WHERE biblionumber=? "); $sth->execute($biblionumber); my $row = $sth->fetchrow_hashref; + $sth->finish; my $biblioitemnumber = $row->{'biblioitemnumber'}; my $marcxml = GetXmlBiblio( $biblionumber ); $marcxml = StripNonXmlChars( $marcxml ); @@ -1179,17 +1188,22 @@ The XML should only contain biblio information (item information is no longer st sub GetXmlBiblio { my ($biblionumber) = @_; - my $dbh = C4::Context->dbh; return unless $biblionumber; - my ($marcxml) = $dbh->selectrow_array( + + # Use state to speed up repeated calls in batch processes + state $sth = C4::Context->dbh->prepare( q| SELECT metadata FROM biblio_metadata WHERE biblionumber=? AND format='marcxml' AND marcflavour=? - |, undef, $biblionumber, C4::Context->preference('marcflavour') + | ); + + $sth->execute( $biblionumber, C4::Context->preference('marcflavour') ); + my ($marcxml) = $sth->fetchrow(); + $sth->finish; return $marcxml; } @@ -2090,10 +2104,11 @@ sub UpsertMarcControlField { sub GetFrameworkCode { my ($biblionumber) = @_; - my $dbh = C4::Context->dbh; - my $sth = $dbh->prepare("SELECT frameworkcode FROM biblio WHERE biblionumber=?"); + # Use state to speed up repeated calls in batch processes + state $sth = C4::Context->dbh->prepare("SELECT frameworkcode FROM biblio WHERE biblionumber=?"); $sth->execute($biblionumber); my ($frameworkcode) = $sth->fetchrow; + $sth->finish; return $frameworkcode; } @@ -2756,36 +2771,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 e7279ff..751a96e 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -21,6 +21,7 @@ package C4::Items; use strict; #use warnings; FIXME - Bug 2505 +use Modern::Perl; use Carp; use C4::Context; use C4::Koha; @@ -1524,6 +1525,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. + state $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 @@ -1660,10 +1726,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'}; } @@ -2328,7 +2395,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/t/db_dependent/Items.t b/t/db_dependent/Items.t index ef09873..8c46b00 100755 --- a/t/db_dependent/Items.t +++ b/t/db_dependent/Items.t @@ -667,7 +667,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}; @@ -708,9 +708,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 ); @@ -725,8 +723,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