From b929e2cab2719870d12f4d32e0fa9e70d5fc3307 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 --- C4/Biblio.pm | 56 ++++++++++++++++++++++++++--------------------- C4/Circulation.pm | 18 +++++++++++++-- C4/Items.pm | 21 ++++++++++++++---- tools/batchMod.pl | 19 ++++++++++++---- 4 files changed, 79 insertions(+), 35 deletions(-) diff --git a/C4/Biblio.pm b/C4/Biblio.pm index da86e62fdd..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 update MARC record if it's available. If it's not supplied -and is needed, it'll 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,16 +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; + } } - my $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"; @@ -2540,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 a9cf02ca3d..26c3ee6dd2 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -3842,9 +3842,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 @@ -3895,7 +3909,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 f2db7a4894..29be91b952 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 fe40f84c7e..9e20ba4ac9 100755 --- a/tools/batchMod.pl +++ b/tools/batchMod.pl @@ -184,6 +184,7 @@ if ($op eq "action") { } } + my $biblionumbers; try { my $schema = Koha::Database->new->schema; $schema->txn_do( @@ -201,6 +202,7 @@ if ($op eq "action") { my $return = $item->safe_delete; if ( ref( $return ) ) { $deleted_items++; + push @$biblionumbers, $itemdata->{'biblionumber'}; } else { $not_deleted_items++; @@ -280,15 +282,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) { $modified_items++ if $modified; @@ -317,7 +325,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.20.1