Bug 20377

Summary: Add transaction and skip_merge to 'remove_unused_authorities.pl' script
Product: Koha Reporter: Mason James <mtj>
Component: Command-line UtilitiesAssignee: Mason James <mtj>
Status: Failed QA --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: bob, dilan, jonathan.druart, m.de.rooy, robin
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21865
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 20377: Various speed tweaks to 'remove_unused_authorities.pl' script
Bug 20377: Various speed tweaks to 'remove_unused_authorities.pl' script
Bug 20377: Various speed tweaks to 'remove_unused_authorities.pl' script

Description Mason James 2018-03-12 23:18:04 UTC
Here is a patch to make 'remove_unused_authorities.pl' a bit faster
i get a 5x speedup after this patch

fyi: i pinched most of this patch from bulkmarcimport.pl


1/ import a bunch of authority record into a Koha, and index

2/ run script, note execution time
  $ time perl ./misc/migration_tools/remove_unused_authorities.pl 
  real    0m50s

3/ apply patch

4/ run script again, and notice it's a bit faster
  $ time perl ./misc/migration_tools/remove_unused_authorities.pl 
  real    0m10s
Comment 1 Mason James 2018-03-12 23:24:20 UTC
Created attachment 72727 [details] [review]
Bug 20377: Various speed tweaks to 'remove_unused_authorities.pl' script

Here is a patch to make 'remove_unused_authorities.pl' a bit faster
i get a 5x speedup after this patch

fyi: i pinched most of this patch from bulkmarcimport.pl

1/ import a bunch of authority record into a Koha, and index

2/ run script, note execution time
  $ time perl ./misc/migration_tools/remove_unused_authorities.pl
  real    0m50s

3/ apply patch

4/ run script again, and notice it's a bit faster
  $ time perl ./misc/migration_tools/remove_unused_authorities.pl
  real    0m10s
Comment 2 Brendan Gallagher 2018-03-13 13:49:12 UTC
Created attachment 72775 [details] [review]
Bug 20377: Various speed tweaks to 'remove_unused_authorities.pl' script

Here is a patch to make 'remove_unused_authorities.pl' a bit faster
i get a 5x speedup after this patch

fyi: i pinched most of this patch from bulkmarcimport.pl

1/ import a bunch of authority record into a Koha, and index

2/ run script, note execution time
  $ time perl ./misc/migration_tools/remove_unused_authorities.pl
  real    0m50s

3/ apply patch

4/ run script again, and notice it's a bit faster
  $ time perl ./misc/migration_tools/remove_unused_authorities.pl
  real    0m10s

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 3 Marcel de Rooy 2018-03-23 09:18:40 UTC
Note: My first impression about your use of $commit was that we should not use it that way (I would interpret it as a confirm flag like in other cron jobs), but bulkmarcimport is using it like you do here..
Comment 4 Marcel de Rooy 2018-03-23 09:42:04 UTC
Created attachment 73166 [details] [review]
Bug 20377: Various speed tweaks to 'remove_unused_authorities.pl' script

Here is a patch to make 'remove_unused_authorities.pl' a bit faster
i get a 5x speedup after this patch

fyi: i pinched most of this patch from bulkmarcimport.pl

1/ import a bunch of authority record into a Koha, and index

2/ run script, note execution time
  $ time perl ./misc/migration_tools/remove_unused_authorities.pl
  real    0m50s

3/ apply patch

4/ run script again, and notice it's a bit faster
  $ time perl ./misc/migration_tools/remove_unused_authorities.pl
  real    0m10s

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2018-03-23 09:43:14 UTC
Combining -test and -commit is kind of confusing but could be considered outside the scope? Would welcome a confirm flag instead of having to add -test.
Comment 6 Jonathan Druart 2018-03-26 18:26:36 UTC
- Please improve bug title and commit messages (there are no various tweaks, just using a transaction)
- specify default value in POD
- maybe we should remove 'c' to avoid the ambiguity with confirm?
- transaction must be done using $schema->txn_do
Comment 7 Jonathan Druart 2018-03-26 18:27:47 UTC
+        DelAuthority({ authid => $data->{'authid'}, skip_merge => 1 }) unless $test;

Checking $test must be done on the commit statements