From 6bf3fa99b8cfb7c1171184b631746deda0f6215c Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Wed, 5 Aug 2020 14:20:47 +0200 Subject: [PATCH] Bug 25265: Prevent double reindex of the same item in batchmod When batch editing, 2 reindex calls are sent to ES/Zebra. We can easily avoid that reusing the skip_modzebra_update Additionally we should only send one request for biblio, and we should only do it if we succeed As the hwole batch mod is in a transaction it is possible to fail in which case Zebra queue is reset, but ES indexes have already been set In addition to the skip param this patchset makes ModZebra take lists of biblionumbers or records so that multiple records can be passed in one call Test plan: 1 - Have a bib with several items, none marked 'not for loan' 2 - Do a staff search that returns this biblio 3 - Items show as available 4 - Click on title to go to details page 5 - Edit->Item in a batch 6 - Set the not for loan status for all items 7 - Repeat your search 8 - Items show as not for loan Signed-off-by: Bob Bennhoff --- C4/Biblio.pm | 55 +++++++++++++++++++++++++++++++------------------------ C4/Circulation.pm | 18 ++++++++++++++++-- C4/Items.pm | 21 +++++++++++++++++---- tools/batchMod.pl | 19 +++++++++++++++---- 4 files changed, 79 insertions(+), 34 deletions(-) diff --git a/C4/Biblio.pm b/C4/Biblio.pm index 97fa95aeb1..3a8beff3f8 100644 --- a/C4/Biblio.pm +++ b/C4/Biblio.pm @@ -2491,21 +2491,23 @@ sub CountItemsIssued { ModZebra( $biblionumber, $op, $server, $record ); -$biblionumber is the biblionumber we want to index +$biblionumbers are an arrayref of the biblionumbers we want to index, if single passed we convert to arrayref $op is specialUpdate or recordDelete, and is used to know what we want to do $server is the server that we want to update -$record is the updated MARC record. If it's not supplied -and is needed it will be loaded from the database. +$records are an arrayref of the updated MARC records if they are available. If they are not supplied +and are needed, they will be loaded from the database using $biblionumbers param. If a single record passed we convert to array ref =cut sub ModZebra { ###Accepts a $server variable thus we can use it for biblios authorities or other zebra dbs - my ( $biblionumber, $op, $server, $record ) = @_; - $debug && warn "ModZebra: update requested for: $biblionumber $op $server\n"; + my ( $biblionumbers, $op, $server, $records ) = @_; + $biblionumbers = [$biblionumbers] if ref $biblionumbers ne 'ARRAY' && defined $biblionumbers; + $records = [$records] if ref $biblionumbers ne 'ARRAY' && defined $records; + $debug && warn "ModZebra: updates requested for: @$biblionumbers $op $server\n"; if ( C4::Context->preference('SearchEngine') eq 'Elasticsearch' ) { # TODO abstract to a standard API that'll work for whatever @@ -2518,15 +2520,18 @@ sub ModZebra { } ); if ( $op eq 'specialUpdate' ) { - unless ($record) { - $record = GetMarcBiblio({ - biblionumber => $biblionumber, - embed_items => 1 }); + unless ($records) { + foreach my $biblionumber ( @$biblionumbers ){ + my $record = GetMarcBiblio({ + biblionumber => $biblionumber, + embed_items => 1 }); + push @$records, $record; + } } - $indexer->update_index_background( [$biblionumber], [$record] ); + $indexer->update_index_background( $biblionumbers, $records ); } elsif ( $op eq 'recordDelete' ) { - $indexer->delete_index_background( [$biblionumber] ); + $indexer->delete_index_background( $biblionumbers ); } else { croak "ModZebra called with unknown operation: $op"; @@ -2539,19 +2544,21 @@ sub ModZebra { # at the same time # replaced by a zebraqueue table, that is filled with ModZebra to run. # the table is emptied by rebuild_zebra.pl script (using the -z switch) - my $check_sql = "SELECT COUNT(*) FROM zebraqueue - WHERE server = ? - AND biblio_auth_number = ? - AND operation = ? - AND done = 0"; - my $check_sth = $dbh->prepare_cached($check_sql); - $check_sth->execute( $server, $biblionumber, $op ); - my ($count) = $check_sth->fetchrow_array; - $check_sth->finish(); - if ( $count == 0 ) { - my $sth = $dbh->prepare("INSERT INTO zebraqueue (biblio_auth_number,server,operation) VALUES(?,?,?)"); - $sth->execute( $biblionumber, $server, $op ); - $sth->finish; + foreach my $biblionumber ( @$biblionumbers ){ + my $check_sql = "SELECT COUNT(*) FROM zebraqueue + WHERE server = ? + AND biblio_auth_number = ? + AND operation = ? + AND done = 0"; + my $check_sth = $dbh->prepare_cached($check_sql); + $check_sth->execute( $server, $biblionumber, $op ); + my ($count) = $check_sth->fetchrow_array; + $check_sth->finish(); + if ( $count == 0 ) { + my $sth = $dbh->prepare("INSERT INTO zebraqueue (biblio_auth_number,server,operation) VALUES(?,?,?)"); + $sth->execute( $biblionumber, $server, $op ); + $sth->finish; + } } } diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 3571051e84..20402407f9 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -3716,9 +3716,23 @@ sub ReturnLostItem{ MarkIssueReturned( $borrowernumber, $itemnum ); } +=head2 LostItem + + LostItem( $itemnumber, $mark_lost_from, $force_mark_returned, [$params] ); + +The final optional parameter, C<$params>, expected to contain +'skip_modzebra_update' key, which relayed down to Koha::Item/store, +there it prevents calling of ModZebra (and Elasticsearch update), +which takes most of the time in batch adds/deletes: ModZebra better +to be called later in C after the whole loop. + +$params: + skip_modzebra_update => 1|0 + +=cut sub LostItem{ - my ($itemnumber, $mark_lost_from, $force_mark_returned) = @_; + my ($itemnumber, $mark_lost_from, $force_mark_returned, $params) = @_; unless ( $mark_lost_from ) { # Temporary check to avoid regressions @@ -3769,7 +3783,7 @@ sub LostItem{ #When item is marked lost automatically cancel its outstanding transfers and set items holdingbranch to the transfer source branch (frombranch) if (my ( $datesent,$frombranch,$tobranch ) = GetTransfers($itemnumber)) { - Koha::Items->find($itemnumber)->holdingbranch($frombranch)->store; + Koha::Items->find($itemnumber)->holdingbranch($frombranch)->store({ skip_modzebra_update => $params->{skip_modzebra_update} }); } my $transferdeleted = DeleteTransfer($itemnumber); } diff --git a/C4/Items.pm b/C4/Items.pm index 22ac2de12c..2738751a41 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -281,10 +281,23 @@ sub AddItemBatchFromMarc { return (\@itemnumbers, \@errors); } +=head2 ModItemFromMarc + +my $item = ModItemFromMarc($item_marc, $biblionumber, $itemnumber[, $params]); + +The final optional parameter, C<$params>, expected to contain +'skip_modzebra_update' key, which relayed down to Koha::Item/store, +there it prevents calling of ModZebra (and Elasticsearch update), +which takes most of the time in batch adds/deletes: ModZebra better +to be called later in C after the whole loop. + +$params: + skip_modzebra_update => 1|0 + +=cut + sub ModItemFromMarc { - my $item_marc = shift; - my $biblionumber = shift; - my $itemnumber = shift; + my ( $item_marc, $biblionumber, $itemnumber, $params ) = @_; my $frameworkcode = C4::Biblio::GetFrameworkCode($biblionumber); my ( $itemtag, $itemsubfield ) = C4::Biblio::GetMarcFromKohaField( "items.itemnumber" ); @@ -313,7 +326,7 @@ sub ModItemFromMarc { $item_object = $item_object->set_or_blank($item); my $unlinked_item_subfields = _get_unlinked_item_subfields( $localitemmarc, $frameworkcode ); $item_object->more_subfields_xml(_get_unlinked_subfields_xml($unlinked_item_subfields)); - $item_object->store; + $item_object->store({ skip_modzebra_update => $params->{skip_modzebra_update} }); return $item_object->unblessed; } diff --git a/tools/batchMod.pl b/tools/batchMod.pl index 83a052b318..c72c763925 100755 --- a/tools/batchMod.pl +++ b/tools/batchMod.pl @@ -193,6 +193,7 @@ if ($op eq "action") { $ynhash->{'av'.$yn->authorised_value} = $yn->lib; } + my $biblionumbers; try { my $schema = Koha::Database->new->schema; $schema->txn_do( @@ -211,6 +212,7 @@ if ($op eq "action") { my $return = $item->safe_delete; if ( ref( $return ) ) { $deleted_items++; + push @$biblionumbers, $itemdata->{'biblionumber'}; } else { $not_deleted_items++; @@ -299,15 +301,21 @@ if ($op eq "action") { my $item = ModItemFromMarc( $localmarcitem, $itemdata->{biblionumber}, - $itemnumber + $itemnumber, + { skip_modzebra_update => 1 }, ) ) { - LostItem( $itemnumber, 'batchmod' ) - if $item->{itemlost} + LostItem( + $itemnumber, + 'batchmod', + undef, + { skip_modzebra_update => 1 } + ) if $item->{itemlost} and not $itemdata->{itemlost}; } }; + push @$biblionumbers, $itemdata->{'biblionumber'}; } } if ($runinbackground) { @@ -337,7 +345,10 @@ if ($op eq "action") { } die "Something terrible has happened!" if ($_ =~ /Rollback failed/); # Rollback failed - } + }; + $biblionumbers = [ uniq @$biblionumbers ]; + my $op = $del ? "delete" : "specialUpdate"; + ModZebra( $biblionumbers, $op, 'biblioserver', undef ); } } # -- 2.11.0