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
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
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>
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..
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>
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.
- 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
+ DelAuthority({ authid => $data->{'authid'}, skip_merge => 1 }) unless $test; Checking $test must be done on the commit statements