Bugzilla – Attachment 100316 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), 16.72 KB, created by
Michal Denar
on 2020-03-08 21:41:00 UTC
(
hide
)
Description:
Bug 22690: Refactor merging of records to improve performance (Elasticsearch)
Filename:
MIME Type:
Creator:
Michal Denar
Created:
2020-03-08 21:41:00 UTC
Size:
16.72 KB
patch
obsolete
>From 69fa1c6e71a3226ab54d39d0c27973799aec1153 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) > >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 > >Signed-off-by: Michal Denar <black23@gmail.com> >--- > C4/Items.pm | 53 +++++---------- > Koha/Biblio.pm | 22 ++++++ > Koha/Item.pm | 56 ++++++++++++++++ > cataloguing/merge.pl | 25 +++---- > t/db_dependent/Items/MoveItemFromBiblio.t | 3 + > t/db_dependent/Koha/Item.t | 108 +++++++++++++++++++++++++++++- > 6 files changed, 214 insertions(+), 53 deletions(-) > >diff --git a/C4/Items.pm b/C4/Items.pm >index f83851fcd2..5e2ef5afb6 100644 >--- a/C4/Items.pm >+++ b/C4/Items.pm >@@ -72,6 +72,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; >@@ -1652,7 +1653,7 @@ sub _koha_new_item { > > =head2 MoveItemFromBiblio > >- MoveItemFromBiblio($itenumber, $frombiblio, $tobiblio); >+ MoveItemFromBiblio($itemnumber, $frombiblio, $tobiblio); > > Moves an item from a biblio to another > >@@ -1662,42 +1663,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 ItemSafeToDelete >diff --git a/Koha/Biblio.pm b/Koha/Biblio.pm >index 4fe30113e3..0796c3bac5 100644 >--- a/Koha/Biblio.pm >+++ b/Koha/Biblio.pm >@@ -815,6 +815,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 d2b1b9d154..6e9fa3640c 100644 >--- a/Koha/Item.pm >+++ b/Koha/Item.pm >@@ -547,6 +547,62 @@ sub to_api_mapping { > }; > } > >+=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(); >+ >+ my $schema = Koha::Database->new()->schema(); >+ >+ # Acquisition orders related to the item >+ my $orders = $schema->resultset('Aqorder')->search( >+ { itemnumber => $self->itemnumber }, >+ { join => 'aqorders_items'} >+ ); >+ $orders->update_all({ biblionumber => $biblionumber }); >+ >+ # reserves >+ my $reserves = $self->_result->reserves; >+ $reserves->update_all({ biblionumber => $biblionumber }); >+ >+ # hold_fill_target >+ 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); >+ } >+ ); >+ #my $tmp_holdsqueues = $self->_result->tmp_holdsqueues; >+ #$tmp_holdsqueues->update_all({ biblionumber => $biblionumber }); >+ >+ # linktrackers >+ my $linktrackers = $schema->resultset('Linktracker')->search({ itemnumber => $self->itemnumber }); >+ $linktrackers->update_all({ biblionumber => $biblionumber }); >+} >+ > =head2 Internal methods > > =head3 _type >diff --git a/cataloguing/merge.pl b/cataloguing/merge.pl >index 146e88e598..c95c49424d 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; > >@@ -81,25 +82,15 @@ 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 >+ 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); > } > > my $sth_subscription = $dbh->prepare(" >@@ -114,6 +105,9 @@ if ($merge) { > my $sth_suggestions = $dbh->prepare(" > UPDATE suggestions SET biblionumber = ? WHERE biblionumber = ? > "); >+ my $sth_articlerequests = $dbh->prepare(" >+ UPDATE article_requests SET biblionumber = ? WHERE biblionumber = ? >+ "); > > my $report_header = {}; > foreach my $biblionumber ($ref_biblionumber, @biblionumbers) { >@@ -163,7 +157,10 @@ 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 article requests >+ $sth_articlerequests->execute($ref_biblionumber, $biblionumber); >+ >+ # 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 fec7e51442..956d4cee01 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' ); >+ > # NOTE This is a trick, if we want to populate the biblioitems table, we should not create a Biblio but a Biblioitem > my $param = { source => 'Biblioitem' }; > my $from_biblio = { >diff --git a/t/db_dependent/Koha/Item.t b/t/db_dependent/Koha/Item.t >index 3a2d2a8c67..4fd1ec8434 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 => 4; >+use Test::More tests => 5; > > use C4::Biblio; > >@@ -152,11 +152,11 @@ subtest 'pickup_locations' => sub { > my $dbh = C4::Context->dbh; > > # Cleanup database >+ $dbh->do('DELETE FROM issues'); > Koha::Holds->search->delete; > Koha::Patrons->search->delete; > Koha::Items->search->delete; > Koha::Libraries->search->delete; >- $dbh->do('DELETE FROM issues'); > Koha::CirculationRules->search->delete; > Koha::CirculationRules->set_rules( > { >@@ -329,3 +329,107 @@ subtest 'pickup_locations' => 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