Attachment 70606 Details for Bug 19884 – Add new functions GetMarcBiblios and EmbedItemsInMarcBiblios for embedding items in multiple records at once for improved performance

[patch] Add new functions GetMarcBiblios and EmbedItemsInMarcBiblios for embedding items in multiple records at once for improved performance

Add-new-functions-GetMarcBiblios-and-EmbedItemsInM.patch (text/plain), 13.21 KB, created by David Gustafsson on 2018-01-17 12:39:56 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: David Gustafsson

Created: 2018-01-17 12:39:56 UTC

Size: 13.21 KB

patch

obsolete

>From 6a63cccf7890abe2d0411fa54b9f71a4992d821d Mon Sep 17 00:00:00 2001
>From: David Gustafsson <david.gustafsson@ub.gu.se>
>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

Actions: View | Diff | Splinter Review

Attachments on bug 19884: 70172 | 70182 | 70183 | 70184 | 70185 | 70247 | 70248 | 70251 | 70284 | 70285 | 70286 | 70606 | 82870 | 82871 | 82979 | 83111 | 83112 | 93949