Bugzilla – Attachment 109694 Details for
Bug 22690
Merging records with many items too slow (Elasticsearch)
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 22690: Refactor merging of records to improve performance (Elasticsearch)
Bug-22690-Refactor-merging-of-records-to-improve-p.patch (text/plain), 17.25 KB, created by
Joonas Kylmälä
on 2020-09-07 10:17:01 UTC
(
hide
)
Description:
Bug 22690: Refactor merging of records to improve performance (Elasticsearch)
Filename:
MIME Type:
Creator:
Joonas Kylmälä
Created:
2020-09-07 10:17:01 UTC
Size:
17.25 KB
patch
obsolete
>From 03d50ccc73ee1ef564bdfab1ee807dc675e497a1 Mon Sep 17 00:00:00 2001 >From: Ere Maijala <ere.maijala@helsinki.fi> >Date: Mon, 16 Sep 2019 13:55:37 +0300 >Subject: [PATCH] Bug 22690: Refactor merging of records to improve performance > (Elasticsearch) >MIME-Version: 1.0 >Content-Type: text/plain; charset=UTF-8 >Content-Transfer-Encoding: 8bit > >This patch allows merging of records with many items without the web server timing out. > >Test plan: > >Without the patch: > >- Create 2 records (one with e.g. 1000 items). >- Do a cataloguing search that displays both records, select them and click "Merge selected". >- Choose the record with many items as the one to be eliminated. >- Start the merging. >- After a while the web server should give you a timeout error (the merging process may still continue) > >With the patch: >- Do the same as above >- This time verify that the records are merged without timeout >- Create a new biblio with an item >- Add with the item: > * acquisition order > * hold (reserve) >- Merge the biblio to another one >- Verify that the item and its related data was moved >- Verify that tests pass: > prove -v t/db_dependent/Koha/Item.t > prove -v t/db_dependent/Items/MoveItemFromBiblio.t > >Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> >--- > C4/Items.pm | 53 +++++---------- > Koha/Biblio.pm | 22 ++++++ > Koha/Item.pm | 63 +++++++++++++++++ > Koha/Schema/Result/Item.pm | 13 ++++ > cataloguing/merge.pl | 22 ++---- > t/db_dependent/Items/MoveItemFromBiblio.t | 3 + > t/db_dependent/Koha/Item.t | 108 +++++++++++++++++++++++++++++- > 7 files changed, 230 insertions(+), 54 deletions(-) > >diff --git a/C4/Items.pm b/C4/Items.pm >index 22ac2de12c..44359a0e77 100644 >--- a/C4/Items.pm >+++ b/C4/Items.pm >@@ -66,6 +66,7 @@ use Koha::AuthorisedValues; > use Koha::DateUtils qw(dt_from_string); > use Koha::Database; > >+use Koha::Biblios; > use Koha::Biblioitems; > use Koha::Items; > use Koha::ItemTypes; >@@ -1070,7 +1071,7 @@ the inner workings of C<C4::Items>. > > =head2 MoveItemFromBiblio > >- MoveItemFromBiblio($itenumber, $frombiblio, $tobiblio); >+ MoveItemFromBiblio($itemnumber, $frombiblio, $tobiblio); > > Moves an item from a biblio to another > >@@ -1080,42 +1081,20 @@ Returns undef if the move failed or the biblionumber of the destination record o > > sub MoveItemFromBiblio { > my ($itemnumber, $frombiblio, $tobiblio) = @_; >- my $dbh = C4::Context->dbh; >- my ( $tobiblioitem ) = $dbh->selectrow_array(q| >- SELECT biblioitemnumber >- FROM biblioitems >- WHERE biblionumber = ? >- |, undef, $tobiblio ); >- my $return = $dbh->do(q| >- UPDATE items >- SET biblioitemnumber = ?, >- biblionumber = ? >- WHERE itemnumber = ? >- AND biblionumber = ? >- |, undef, $tobiblioitem, $tobiblio, $itemnumber, $frombiblio ); >- if ($return == 1) { >- ModZebra( $tobiblio, "specialUpdate", "biblioserver" ); >- ModZebra( $frombiblio, "specialUpdate", "biblioserver" ); >- # Checking if the item we want to move is in an order >- require C4::Acquisition; >- my $order = C4::Acquisition::GetOrderFromItemnumber($itemnumber); >- if ($order) { >- # Replacing the biblionumber within the order if necessary >- $order->{'biblionumber'} = $tobiblio; >- C4::Acquisition::ModOrder($order); >- } >- >- # Update reserves, hold_fill_targets, tmp_holdsqueue and linktracker tables >- for my $table_name ( qw( reserves hold_fill_targets tmp_holdsqueue linktracker ) ) { >- $dbh->do( qq| >- UPDATE $table_name >- SET biblionumber = ? >- WHERE itemnumber = ? >- |, undef, $tobiblio, $itemnumber ); >- } >- return $tobiblio; >- } >- return; >+ >+ my $item = Koha::Items->find($itemnumber); >+ return unless $item; >+ >+ my $biblio = Koha::Biblios->find($tobiblio); >+ return unless $biblio; >+ >+ return unless $item->biblionumber != $biblio->biblionumber; >+ >+ $item->move_to_biblio($biblio); >+ ModZebra($tobiblio, 'specialUpdate', 'biblioserver'); >+ ModZebra($frombiblio, 'specialUpdate', 'biblioserver'); >+ >+ return $tobiblio; > } > > =head2 _marc_from_item_hash >diff --git a/Koha/Biblio.pm b/Koha/Biblio.pm >index 830b9c4d73..6f32922b39 100644 >--- a/Koha/Biblio.pm >+++ b/Koha/Biblio.pm >@@ -816,6 +816,28 @@ sub to_api_mapping { > }; > } > >+=head3 adopt_items_from_biblio >+ >+$biblio->adopt_items_from_biblio($from_biblio); >+ >+Move items from the given biblio to this one. >+ >+=cut >+ >+sub adopt_items_from_biblio { >+ my ( $self, $from_biblio ) = @_; >+ >+ my $items = $from_biblio->items; >+ if ($items) { >+ while (my $item = $items->next()) { >+ $item->move_to_biblio($self); >+ } >+ C4::Biblio::ModZebra( $self->biblionumber, "specialUpdate", "biblioserver" ); >+ C4::Biblio::ModZebra( $from_biblio->biblionumber, "specialUpdate", "biblioserver" ); >+ } >+} >+ >+ > =head2 Internal methods > > =head3 type >diff --git a/Koha/Item.pm b/Koha/Item.pm >index 8dffac4db5..0eae35d8c0 100644 >--- a/Koha/Item.pm >+++ b/Koha/Item.pm >@@ -967,6 +967,69 @@ sub itemtype { > return Koha::ItemTypes->find( $self->effective_itemtype ); > } > >+=head3 item_orders >+ >+my $orders = $item->item_orders(); >+ >+Returns a Koha::Acquisition::Orders object >+ >+=cut >+ >+sub item_orders { >+ my ( $self ) = @_; >+ >+ my $orders = $self->_result->item_orders; >+ return Koha::Acquisition::Orders->_new_from_dbic($orders); >+} >+ >+=head3 move_to_biblio >+ >+ $item->move_to_biblio($to_biblio); >+ >+Move the item to another biblio and update any references also in other tables. >+ >+=cut >+ >+sub move_to_biblio { >+ my ( $self, $to_biblio ) = @_; >+ >+ my $biblionumber = $to_biblio->biblionumber; >+ >+ # Own biblionumber and biblioitemnumber >+ $self->set({ >+ biblionumber => $biblionumber, >+ biblioitemnumber => $to_biblio->biblioitem->biblioitemnumber >+ })->store({ skip_modzebra_update => 1 }); >+ >+ # Acquisition orders >+ $self->item_orders->update({ biblionumber => $biblionumber }, { no_triggers => 1 }); >+ >+ # Holds >+ $self->holds->update({ biblionumber => $biblionumber }, { no_triggers => 1 }); >+ >+ # hold_fill_target (there's no Koha object available) >+ my $hold_fill_target = $self->_result->hold_fill_target; >+ if ($hold_fill_target) { >+ $hold_fill_target->update({ biblionumber => $biblionumber }); >+ } >+ >+ # tmp_holdsqueues - Can't update with DBIx since the table is missing a primary key >+ # and can't even fake one since the significant columns are nullable. >+ my $storage = $self->_result->result_source->storage; >+ $storage->dbh_do( >+ sub { >+ my ($storage, $dbh, @cols) = @_; >+ >+ $dbh->do("UPDATE tmp_holdsqueue SET biblionumber=? WHERE itemnumber=?", undef, $biblionumber, $self->itemnumber); >+ } >+ ); >+ >+ # linktrackers >+ my $schema = Koha::Database->new()->schema(); >+ my $linktrackers = $schema->resultset('Linktracker')->search({ itemnumber => $self->itemnumber }); >+ $linktrackers->update_all({ biblionumber => $biblionumber }); >+} >+ > =head2 Internal methods > > =head3 _after_item_action_hooks >diff --git a/Koha/Schema/Result/Item.pm b/Koha/Schema/Result/Item.pm >index 7d8b9a6cc6..8138df912b 100644 >--- a/Koha/Schema/Result/Item.pm >+++ b/Koha/Schema/Result/Item.pm >@@ -770,6 +770,19 @@ __PACKAGE__->add_columns( > '+exclude_from_local_holds_priority' => { is_boolean => 1 }, > ); > >+# Relationship with orders via the aqorders_item table that not have foreign keys >+__PACKAGE__->has_many( >+ "aqorders_item", >+ "Koha::Schema::Result::AqordersItem", >+ { "foreign.itemnumber" => "self.itemnumber" }, >+ { cascade_copy => 0, cascade_delete => 0 }, >+); >+__PACKAGE__->many_to_many( >+ "item_orders", >+ "aqorders_item", >+ "ordernumber", >+); >+ > use C4::Context; > sub effective_itemtype { > my ( $self ) = @_; >diff --git a/cataloguing/merge.pl b/cataloguing/merge.pl >index baf2b3057a..b0a97aa8b5 100755 >--- a/cataloguing/merge.pl >+++ b/cataloguing/merge.pl >@@ -31,6 +31,7 @@ use C4::Reserves qw/MergeHolds/; > use C4::Acquisition qw/ModOrder GetOrdersByBiblionumber/; > > use Koha::BiblioFrameworks; >+use Koha::Biblios; > use Koha::Items; > use Koha::MetadataRecord; > >@@ -80,25 +81,16 @@ if ($merge) { > $record->leader(GetMarcBiblio({ biblionumber => $ref_biblionumber })->leader()); > > my $frameworkcode = $input->param('frameworkcode'); >- my @notmoveditems; > > # Modifying the reference record > ModBiblio($record, $ref_biblionumber, $frameworkcode); > >- # Moving items from the other record to the reference record >+ # Moving items and article requests from the other record to the reference record >+ my $biblio = Koha::Biblios->find($ref_biblionumber); > foreach my $biblionumber (@biblionumbers) { >- my $items = Koha::Items->search({ biblionumber => $biblionumber }); >- while ( my $item = $items->next) { >- my $res = MoveItemFromBiblio( $item->itemnumber, $biblionumber, $ref_biblionumber ); >- if ( not defined $res ) { >- push @notmoveditems, $item->itemnumber; >- } >- } >- } >- # If some items could not be moved : >- if (scalar(@notmoveditems) > 0) { >- my $itemlist = join(' ',@notmoveditems); >- push @errors, { code => "CANNOT_MOVE", value => $itemlist }; >+ my $from_biblio = Koha::Biblios->find($biblionumber); >+ $biblio->adopt_items_from_biblio($from_biblio); >+ $from_biblio->article_requests->update({ biblionumber => $ref_biblionumber }, { no_triggers => 1 }); > } > > my $sth_subscription = $dbh->prepare(" >@@ -162,7 +154,7 @@ if ($merge) { > # Moving suggestions > $sth_suggestions->execute($ref_biblionumber, $biblionumber); > >- # Moving orders (orders linked to items of frombiblio have already been moved by MoveItemFromBiblio) >+ # Moving orders (orders linked to items of frombiblio have already been moved by adopt_items_from_biblio) > my @allorders = GetOrdersByBiblionumber($biblionumber); > foreach my $myorder (@allorders) { > $myorder->{'biblionumber'} = $ref_biblionumber; >diff --git a/t/db_dependent/Items/MoveItemFromBiblio.t b/t/db_dependent/Items/MoveItemFromBiblio.t >index 75ae92afe0..137806ebd1 100644 >--- a/t/db_dependent/Items/MoveItemFromBiblio.t >+++ b/t/db_dependent/Items/MoveItemFromBiblio.t >@@ -25,12 +25,15 @@ use Koha::Holds; > use Koha::Items; > > use t::lib::TestBuilder; >+use t::lib::Mocks; > use Data::Dumper qw|Dumper|; > > my $schema = Koha::Database->new->schema; > $schema->storage->txn_begin; > my $builder = t::lib::TestBuilder->new; > >+t::lib::Mocks::mock_preference( 'SearchEngine', 'Zebra' ); >+ > my $from_biblio = $builder->build_sample_biblio; > my $to_biblio = $builder->build_sample_biblio; > >diff --git a/t/db_dependent/Koha/Item.t b/t/db_dependent/Koha/Item.t >index 0309fa94f6..ccb064acf2 100644 >--- a/t/db_dependent/Koha/Item.t >+++ b/t/db_dependent/Koha/Item.t >@@ -19,7 +19,7 @@ > > use Modern::Perl; > >-use Test::More tests => 7; >+use Test::More tests => 8; > > use C4::Biblio; > use C4::Circulation; >@@ -163,8 +163,8 @@ subtest 'pickup_locations' => sub { > my $dbh = C4::Context->dbh; > > # Cleanup database >- Koha::Holds->search->delete; > $dbh->do('DELETE FROM issues'); >+ Koha::Holds->search->delete; > Koha::Patrons->search->delete; > Koha::Items->search->delete; > Koha::Libraries->search->delete; >@@ -516,3 +516,107 @@ subtest 'Tests for itemtype' => sub { > > $schema->storage->txn_rollback; > }; >+ >+subtest 'move_to_biblio() tests' => sub { >+ >+ plan tests => 12; >+ >+ $schema->storage->txn_begin; >+ >+ my $dbh = C4::Context->dbh; >+ >+ my $source_biblio = $builder->build_sample_biblio(); >+ my $target_biblio = $builder->build_sample_biblio(); >+ >+ my $source_biblionumber = $source_biblio->biblionumber; >+ >+ my $item1 = $builder->build_sample_item({ biblionumber => $source_biblionumber }); >+ my $item2 = $builder->build_sample_item({ biblionumber => $source_biblionumber }); >+ >+ my $itemnumber1 = $item1->itemnumber; >+ my $itemnumber2 = $item2->itemnumber; >+ >+ my $library = $builder->build_object({ class => 'Koha::Libraries' }); >+ >+ my $patron = $builder->build_object({ >+ class => 'Koha::Patrons', >+ value => { branchcode => $library->branchcode, } >+ }); >+ my $borrowernumber = $patron->borrowernumber; >+ >+ $dbh->do("INSERT INTO aqbudgets (budget_notes) VALUES ('test')"); >+ my $budget_id = $dbh->last_insert_id(undef, undef, 'aqbudgets', 'budget_id'); >+ >+ $dbh->do('INSERT INTO aqorders (biblionumber, budget_id) VALUES (?, ?)', undef, $source_biblionumber, $budget_id); >+ my $ordernumber1 = $dbh->last_insert_id(undef, undef, 'aqorders', 'ordernumber'); >+ $dbh->do('INSERT INTO aqorders_items (ordernumber, itemnumber) VALUES (?, ?)', undef, $ordernumber1, $itemnumber1); >+ >+ $dbh->do('INSERT INTO aqorders (biblionumber, budget_id) VALUES (?, ?)', undef, $source_biblionumber, $budget_id); >+ my $ordernumber2 = $dbh->last_insert_id(undef, undef, 'aqorders', 'ordernumber'); >+ $dbh->do('INSERT INTO aqorders_items (ordernumber, itemnumber) VALUES (?, ?)', undef, $ordernumber2, $source_biblionumber); >+ >+ my $reserve_id1 = C4::Reserves::AddReserve({ >+ branch => $library->branchcode, >+ borrowernumber => $borrowernumber, >+ biblionumber => $source_biblionumber, >+ itemnumber => $itemnumber1, >+ title => 'title' >+ }); >+ >+ my $reserve_id2 = C4::Reserves::AddReserve({ >+ branch => $library->branchcode, >+ borrowernumber => $patron->borrowernumber, >+ biblionumber => $source_biblionumber, >+ itemnumber => $itemnumber2, >+ title => 'title' >+ }); >+ >+ $dbh->do("INSERT INTO tmp_holdsqueue (surname,borrowernumber,itemnumber,biblionumber) VALUES ('Clamp',?,?,?)", undef, $borrowernumber, $itemnumber1, $source_biblionumber); >+ $dbh->do("INSERT INTO tmp_holdsqueue (surname,borrowernumber,itemnumber,biblionumber) VALUES ('Clamp',?,?,?)", undef, $borrowernumber, $itemnumber2, $source_biblionumber); >+ >+ $dbh->do('INSERT INTO hold_fill_targets (borrowernumber,itemnumber,biblionumber) VALUES (?,?,?)', undef, $borrowernumber, $itemnumber1, $source_biblionumber); >+ $dbh->do('INSERT INTO hold_fill_targets (borrowernumber,itemnumber,biblionumber) VALUES (?,?,?)', undef, $borrowernumber, $itemnumber2, $source_biblionumber); >+ >+ $dbh->do('INSERT INTO linktracker (borrowernumber,itemnumber,biblionumber) VALUES (?,?,?)', undef, $borrowernumber, $itemnumber1, $source_biblionumber); >+ $dbh->do('INSERT INTO linktracker (borrowernumber,itemnumber,biblionumber) VALUES (?,?,?)', undef, $borrowernumber, $itemnumber2, $source_biblionumber); >+ >+ $item1->move_to_biblio($target_biblio); >+ >+ my @result = $dbh->selectrow_array('SELECT biblionumber FROM items WHERE itemnumber = ?', undef, $itemnumber1); >+ is($result[0], $target_biblio->biblionumber, 'Correct biblionumber in the moved item'); >+ >+ @result = $dbh->selectrow_array('SELECT biblionumber FROM items WHERE itemnumber = ?', undef, $itemnumber2); >+ is($result[0], $source_biblionumber, 'Correct biblionumber in the unmoved item'); >+ >+ @result = $dbh->selectrow_array('SELECT biblionumber FROM aqorders WHERE ordernumber = ?', undef, $ordernumber1); >+ is($result[0], $target_biblio->biblionumber, 'Correct biblionumber in aqorders for order with the moved item'); >+ >+ @result = $dbh->selectrow_array('SELECT biblionumber FROM aqorders WHERE ordernumber = ?', undef, $ordernumber2); >+ is($result[0], $source_biblionumber, 'Correct biblionumber in aqorders for order with the unmoved item'); >+ >+ @result = $dbh->selectrow_array('SELECT biblionumber FROM reserves WHERE reserve_id = ?', undef, $reserve_id1); >+ is($result[0], $target_biblio->biblionumber, 'Correct biblionumber in reserves for moved item'); >+ >+ @result = $dbh->selectrow_array('SELECT biblionumber FROM reserves WHERE reserve_id = ?', undef, $reserve_id2); >+ is($result[0], $source_biblionumber, 'Correct biblionumber in reserves for unmoved item'); >+ >+ @result = $dbh->selectrow_array('SELECT biblionumber FROM tmp_holdsqueue WHERE itemnumber = ?', undef, $itemnumber1); >+ is($result[0], $target_biblio->biblionumber, 'Correct biblionumber in tmp_holdsqueue for moved item'); >+ >+ @result = $dbh->selectrow_array('SELECT biblionumber FROM tmp_holdsqueue WHERE itemnumber = ?', undef, $itemnumber2); >+ is($result[0], $source_biblionumber, 'Correct biblionumber in tmp_holdsqueue for unmoved item'); >+ >+ @result = $dbh->selectrow_array('SELECT biblionumber FROM hold_fill_targets WHERE itemnumber = ?', undef, $itemnumber1); >+ is($result[0], $target_biblio->biblionumber, 'Correct biblionumber in hold_fill_targets for moved item'); >+ >+ @result = $dbh->selectrow_array('SELECT biblionumber FROM hold_fill_targets WHERE itemnumber = ?', undef, $itemnumber2); >+ is($result[0], $source_biblionumber, 'Correct biblionumber in hold_fill_targets for unmoved item'); >+ >+ @result = $dbh->selectrow_array('SELECT biblionumber FROM linktracker WHERE itemnumber = ?', undef, $itemnumber1); >+ is($result[0], $target_biblio->biblionumber, 'Correct biblionumber in linktracker for moved item'); >+ >+ @result = $dbh->selectrow_array('SELECT biblionumber FROM linktracker WHERE itemnumber = ?', undef, $itemnumber2); >+ is($result[0], $source_biblionumber, 'Correct biblionumber in linktracker for unmoved item'); >+ >+ $schema->storage->txn_rollback; >+}; >-- >2.11.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 22690
:
90061
|
90115
|
90116
|
92828
|
94505
|
97696
|
98143
|
98210
|
98211
|
99376
|
99379
|
100316
|
104796
|
104797
|
104871
|
105445
|
106144
|
108777
|
109694
|
110991
|
110994
|
111196
|
111207
|
111236
|
111237
|
112100
|
112184
|
112567
|
112569
|
112754
|
113308
|
113309
|
113941
|
113942
|
113967
|
113968
|
118226
|
118227
|
118229
|
118231
|
118391
|
119433
|
119434
|
120820
|
120821
|
120852
|
120853
|
121389
|
121392
|
121474
|
121516
|
121517
|
121518
|
121519
|
121520
|
122846
|
122847
|
122848
|
122849
|
122850
|
122886
|
122887
|
122888
|
122889
|
122890
|
122936
|
122938
|
122939
|
123029
|
123030
|
123031
|
123032
|
123033
|
123034
|
123035
|
123036
|
123037
|
123038
|
123039
|
123040
|
123041
|
123042
|
123044
|
123062
|
123481
|
123482
|
123483
|
123484
|
123485
|
123486
|
123487
|
123488
|
123489
|
123490
|
123491
|
123492
|
123493
|
123494
|
123495
|
123496
|
123497
|
124176
|
124188
|
124257