Bugzilla – Attachment 79225 Details for
Bug 20664
Optimize retrieval of biblio and item data
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 20664: Optimize retrieval of biblio and item data
Bug-20664-Optimize-retrieval-of-biblio-and-item-da.patch (text/plain), 15.04 KB, created by
Ere Maijala
on 2018-09-21 11:44:57 UTC
(
hide
)
Description:
Bug 20664: Optimize retrieval of biblio and item data
Filename:
MIME Type:
Creator:
Ere Maijala
Created:
2018-09-21 11:44:57 UTC
Size:
15.04 KB
patch
obsolete
>From 90c055edd5ec25f44f9bbfb7e83749a3d9fd4b2a 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: > >diff -u unoptimized.xml optimized.xml > >6.) Run tests to verify that they still pass: > >prove t/db_dependent/Items.t >--- > 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
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=79225">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
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