From 6a63cccf7890abe2d0411fa54b9f71a4992d821d Mon Sep 17 00:00:00 2001 From: David Gustafsson Date: Thu, 4 Jan 2018 18:14:47 +0100 Subject: [PATCH] Add new functions GetMarcBiblios and EmbedItemsInMarcBiblios for embedding items in multiple records at once for improved performance https://bugs.koha-community.org/show_bug.cgi?id=19884 --- C4/Biblio.pm | 174 +++++++++++++++++++--------- Koha/BiblioUtils.pm | 80 +++++++++---- Koha/Items.pm | 4 +- misc/search_tools/rebuild_elastic_search.pl | 2 +- 4 files changed, 181 insertions(+), 79 deletions(-) diff --git a/C4/Biblio.pm b/C4/Biblio.pm index b26c767ef8..ed95e076f9 100644 --- a/C4/Biblio.pm +++ b/C4/Biblio.pm @@ -1161,6 +1161,26 @@ sub GetMarcSubfieldStructureFromKohaField { return wantarray ? @{$mss->{$kohafield}} : $mss->{$kohafield}->[0]; } + +sub GetMarcBiblio { + my ($params) = @_; + + if (not defined $params) { + carp 'GetMarcBiblio called without parameters'; + return; + } + my $biblionumber = $params->{biblionumber}; + + if (not defined $biblionumber) { + carp 'GetMarcBiblio called with undefined biblionumber'; + return; + } + delete $params->{biblionumber}; + $params->{biblionumbers} = [$biblionumber]; + my ($biblio) = GetMarcBiblios($params); + return $biblio; +} + =head2 GetMarcBiblio my $record = GetMarcBiblio({ @@ -1194,52 +1214,65 @@ OpacHiddenItems to be applied. =cut -sub GetMarcBiblio { +sub GetMarcBiblios { my ($params) = @_; if (not defined $params) { - carp 'GetMarcBiblio called without parameters'; + carp 'GetMarcBiblios called without parameters'; return; } - my $biblionumber = $params->{biblionumber}; + my $biblionumbers = $params->{biblionumbers}; my $embeditems = $params->{embed_items} || 0; my $opac = $params->{opac} || 0; - if (not defined $biblionumber) { - carp 'GetMarcBiblio called with undefined biblionumber'; + if (not defined $biblionumbers) { + carp 'GetMarcBiblio called with undefined biblionumbers'; return; } - my $dbh = C4::Context->dbh; - my $sth = $dbh->prepare("SELECT biblioitemnumber FROM biblioitems WHERE biblionumber=? "); - $sth->execute($biblionumber); - my $row = $sth->fetchrow_hashref; - my $biblioitemnumber = $row->{'biblioitemnumber'}; - my $marcxml = GetXmlBiblio( $biblionumber ); - $marcxml = StripNonXmlChars( $marcxml ); - my $frameworkcode = GetFrameworkCode($biblionumber); - MARC::File::XML->default_record_format( C4::Context->preference('marcflavour') ); - my $record = MARC::Record->new(); - - if ($marcxml) { - $record = eval { - MARC::Record::new_from_xml( $marcxml, "utf8", - C4::Context->preference('marcflavour') ); - }; - if ($@) { warn " problem with :$biblionumber : $@ \n$marcxml"; } - return unless $record; - - C4::Biblio::_koha_marc_update_bib_ids( $record, $frameworkcode, $biblionumber, - $biblioitemnumber ); - C4::Biblio::EmbedItemsInMarcBiblio( $record, $biblionumber, undef, $opac ) - if ($embeditems); - - return $record; + my $in_placeholders = join ', ', (('?') x @{$biblionumbers}); + my $dbh = C4::Context->dbh; + my $sth = $dbh->prepare("SELECT biblionumber, biblioitemnumber FROM biblioitems WHERE biblionumber IN ($in_placeholders)"); + $sth->execute(@{$biblionumbers}); + + my %marc_records; + my ($biblionumber, $biblioitemnumber); + my $marc_flavour = C4::Context->preference('marcflavour'); + while (my $biblio_keys = $sth->fetchrow_arrayref) { + ($biblionumber, $biblioitemnumber) = @{$biblio_keys}; + + my $marcxml = GetXmlBiblio( $biblionumber ); + $marcxml = StripNonXmlChars( $marcxml ); + my $frameworkcode = GetFrameworkCode($biblionumber); + MARC::File::XML->default_record_format( $marc_flavour ); + my $record = MARC::Record->new(); + + if ($marcxml) { + $record = eval { + MARC::Record::new_from_xml( $marcxml, "utf8", $marc_flavour); + }; + if ($@) { warn " problem with :$biblionumber : $@ \n$marcxml"; } + next unless $record; + + C4::Biblio::_koha_marc_update_bib_ids( + $record, + $frameworkcode, + $biblionumber, + $biblioitemnumber + ); + $marc_records{$biblionumber} = $record; + } } - else { - return; + + if ($embeditems) { + C4::Biblio::EmbedItemsInMarcBiblios( + \%marc_records, + undef, + $opac + ); } + return wantarray ? values %marc_records : \%marc_records; } =head2 GetXmlBiblio @@ -2810,28 +2843,41 @@ sub EmbedItemsInMarcBiblio { carp 'EmbedItemsInMarcBiblio: No MARC record passed'; return; } - $itemnumbers = [] unless defined $itemnumbers; + my ($marc_with_items) = EmbedItemsInMarcBiblios( + { $biblionumber => $marc }, + $itemnumbers, + $opac + ); + return $marc_with_items; +} + +sub EmbedItemsInMarcBiblios { + my ($marc_records, $itemnumbers, $opac) = @_; - # Could this be moved lower down and run only when items - # exists for improved performance? Probably.. - my $frameworkcode = GetFrameworkCode($biblionumber); - _strip_item_fields($marc, $frameworkcode); + $itemnumbers = [] unless defined $itemnumbers; # ... and embed the current items my $dbh = C4::Context->dbh; - my $sth = $dbh->prepare("SELECT itemnumber FROM items WHERE biblionumber = ?"); - $sth->execute($biblionumber); - my @all_itemnumbers; - while ( my ($itemnumber) = $sth->fetchrow_array ) { - push @all_itemnumbers, $itemnumber; - } + + my @biblionumbers = keys %{$marc_records}; + my $in_placeholders = join ', ', (('?') x @biblionumbers); + + my $biblio_itemnumbers = $dbh->selectcol_arrayref( + "SELECT itemnumber FROM items WHERE biblionumber IN ($in_placeholders)", + undef, + @biblionumbers + ); + # Remove invalid item numbers by intersecting with all valid item numbers - if(@$itemnumbers) { + # if $itemnumbers argument was provided + + # TODO: MAKE SURE THERE IS A TEST FOR THIS + if(@{$itemnumbers}) { my %h = map { $_ => undef } @{$itemnumbers}; - $itemnumbers = [grep { exists $h{$_} } @all_itemnumbers]; + $itemnumbers = [grep { exists $h{$_} } @{$biblio_itemnumbers}]; } else { - $itemnumbers = \@all_itemnumbers; + $itemnumbers = $biblio_itemnumbers; } # If no item numbers then no point in continuing return unless (@{$itemnumbers}); @@ -2841,19 +2887,35 @@ sub EmbedItemsInMarcBiblio { require C4::Items; my $items = C4::Items::GetItems($itemnumbers); - if ($opachiddenitems) { - my %hidden_items = map { $_ => undef } C4::Items::GetHiddenItemnumbers(@{$items}); - # Reduce items to non hidden items - $items = [grep { !(exists $hidden_items{$_->{itemnumber}}) } @{$items}]; + # We have now fetched all items, time to partion by biblionumber + my %items_by_biblionumber = (map { ($_, []) } @biblionumbers); + foreach my $item (@{$items}) { + push @{$items_by_biblionumber{$item->{biblionumber}}}, $item; } - my ($itemtag) = GetMarcFromKohaField("items.itemnumber", $frameworkcode); - my @item_fields; - foreach my $item (@{$items}) { - my $item_marc = C4::Items::GetMarcItem($biblionumber, $item); - push @item_fields, $item_marc->field($itemtag); + foreach my $biblionumber (@biblionumbers) { + my $marc_record = $marc_records->{$biblionumber}; + my $items = $items_by_biblionumber{$biblionumber}; + # Could this be moved lower down and run only when items + # exists for improved performance? Probably.. + my $frameworkcode = GetFrameworkCode($biblionumber); + _strip_item_fields($marc_record, $frameworkcode); + + if ($opachiddenitems) { + my %hidden_items = map { $_ => undef } C4::Items::GetHiddenItemnumbers(@{$items}); + # Reduce items to non hidden items + $items = [grep { !(exists $hidden_items{$_->{itemnumber}}) } @{$items}]; + } + + my ($itemtag) = GetMarcFromKohaField("items.itemnumber", $frameworkcode); + my @item_fields; + foreach my $item (@{$items}) { + my $item_marc = C4::Items::GetMarcItem($biblionumber, $item); + push @item_fields, $item_marc->field($itemtag); + } + $marc_record->append_fields(@item_fields); } - $marc->append_fields(@item_fields); + return $marc_records; } =head1 INTERNAL FUNCTIONS diff --git a/Koha/BiblioUtils.pm b/Koha/BiblioUtils.pm index 73fc12aa4b..44cd563aed 100644 --- a/Koha/BiblioUtils.pm +++ b/Koha/BiblioUtils.pm @@ -111,29 +111,69 @@ The iterator is a Koha::MetadataIterator object. =cut sub get_all_biblios_iterator { + my ($self, $params) = @_; + my $batched; + my $batch_size; + if ($params) { + $batched //= $params->{batched}; + $batch_size = $params->{batch_size} || 2000; + } + my $database = Koha::Database->new(); my $schema = $database->schema(); - my $rs = - $schema->resultset('Biblio')->search( {}, - { columns => [qw/ biblionumber /] } ); - my $next_func = sub { - # Warn and skip bad records, otherwise we break the loop - while (1) { - my $row = $rs->next(); - return if !$row; - my $marc = C4::Biblio::GetMarcBiblio({ - biblionumber => $row->biblionumber, - embed_items => 1 }); - my $next = eval { - __PACKAGE__->new($marc, $row->biblionumber); - }; - if ($@) { - warn "Something went wrong reading record for biblio $row->biblionumber: $@\n"; - next; + my $rs = $schema->resultset('Biblio')->search( + {}, + { columns => [qw/ biblionumber /] } + ); + + my $next_func; + if ($batched) { + my @marc_records_batch; + $next_func = sub { + unless (@marc_records_batch) { + #Batch empty, nead to buffer up more records + my $limit = $batch_size; + my @biblionumbers; + while ($limit--) { + my $row = $rs->next(); + last if !$row; + push @biblionumbers, $row->biblionumber; + } + if (@biblionumbers) { + my $marc_records = C4::Biblio::GetMarcBiblios({ + biblionumbers => \@biblionumbers, + embed_items => 1 + }); + while (my ($biblionumber, $marc_record) = each %{$marc_records}) { + my $next = __PACKAGE__->new($marc_record, $biblionumber); + push @marc_records_batch, $next; + } + } } - return $next; - } - }; + return pop @marc_records_batch; + }; + } + else { + $next_func = sub { + # Warn and skip bad records, otherwise we break the loop + while (1) { + my $row = $rs->next(); + return if !$row; + my $marc = C4::Biblio::GetMarcBiblio({ + biblionumber => $row->biblionumber, + embed_items => 1 + }); + my $next = eval { + __PACKAGE__->new($marc, $row->biblionumber); + }; + if ($@) { + warn "Something went wrong reading record for biblio $row->biblionumber: $@\n"; + next; + } + return $next; + } + }; + } return Koha::MetadataIterator->new($next_func); } diff --git a/Koha/Items.pm b/Koha/Items.pm index d6ab08b3b5..29b46949de 100644 --- a/Koha/Items.pm +++ b/Koha/Items.pm @@ -106,8 +106,8 @@ sub search_unblessed { push @{$items}, $item; } elsif (ref($values) eq 'ARRAY') { - my $in = join ', ', (('?') x @{$values}); - my $sth = C4::Context->dbh->prepare(qq{SELECT * FROM items WHERE $field IN($in)}); + my $in_placeholders = join ', ', (('?') x @{$values}); + my $sth = C4::Context->dbh->prepare(qq{SELECT * FROM items WHERE $field IN($in_placeholders)}); $sth->execute(@{$values}); while (my $item = $sth->fetchrow_hashref) { push @{$items}, $item; diff --git a/misc/search_tools/rebuild_elastic_search.pl b/misc/search_tools/rebuild_elastic_search.pl index 6faa32c63e..fe64b859ec 100755 --- a/misc/search_tools/rebuild_elastic_search.pl +++ b/misc/search_tools/rebuild_elastic_search.pl @@ -131,7 +131,7 @@ if ($index_biblios) { return ($r, Koha::BiblioUtils->get_from_biblionumber($r, item_data => 1 )); }; } else { - my $records = Koha::BiblioUtils->get_all_biblios_iterator(); + my $records = Koha::BiblioUtils->get_all_biblios_iterator({ batched => 1 }); $next = sub { $records->next(); } -- 2.11.0