Summary: | Add transaction and skip_merge to 'remove_unused_authorities.pl' script | ||
---|---|---|---|
Product: | Koha | Reporter: | Mason James <mtj> |
Component: | Command-line Utilities | Assignee: | 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: | ||
Circulation function: | |||
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
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 |