[Bug 43523] New: cleanborrowers.pl is slow
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 Bug ID: 43523 Summary: cleanborrowers.pl is slow Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: lucas@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org Target Milestone: --- I have been looking at cleanborrowers.pl and find that it can be slow when dealing with many borrowers. I have identified some areas where I think improvements can be easily made. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=42509 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 --- Comment #1 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 205598 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=205598&action=edit Bug 43523: Replace _skip_borrowers_with_nonzero_balance with SQL Patch from commit 8138883 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|cleanborrowers.pl is slow |Initial patron lookup in | |cleanborrowers.pl is slow -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 --- Comment #2 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- This improves the speed of the initial patron lookup but doesn't help with the actual deletion. That should be moved to a background job, in my opinion. I filed Bug 43525. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 --- Comment #3 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- I loaded 10,000 patrons into KTD. And did a query that would return all of the, I chose "patrons who have not borrowed since: 1900/01/01 WITHOUT THE PATCH: the initial lookup prior to deleting takes 43.14 seconds WITH PATCH: the initial lookup prior to deleting takes less than 1 second -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |lucas@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #205598|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 --- Comment #4 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 205631 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=205631&action=edit Bug 43523: Replace _skip_borrowers_with_nonzero_balance with SQL To test: 0. Add many borrowers to your system, I was testing with 10,000 each time 1. First without the patch, go to Tools -> Batch patron deletion and anonymization 2. Check 'Verify you want to delete patrons' and choose paramaters that will return most, or all, of the 10k borrowers. I used "who have not borrowed since: 1900/01/01" 3. Click Next >> and confirm. 4. The script should now take significant time to load. For me, with 10k borrowers, it was 3minutes 45 seconds 5. APPLY PATCH, restart_all 6. Load 10,000 new borrowers since the old ones are now deleted. 7. Try the steps again. 8. Load times should be better. For me, it now loads in under 1 second. 9. prove -v t/db_dependent/Members.t NOTE: This doesn't effect the actual deletion which still takes quite a long time. I have filed Bug 43525. as the deletion should be moved to a background job. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 --- Comment #5 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 205632 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=205632&action=edit Bug 43523: Add a new test Patch from commit c500a00 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43017 CC| |andrew@bywatersolutions.com Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43017 [Bug 43017] [OMNIBUS] Interface optimization -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=43525 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Target Milestone|--- |26.11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 Michelle Spinney <mspinney@clamsnet.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mspinney@clamsnet.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #205631|0 |1 is obsolete| | Attachment #205632|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 --- Comment #6 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 206679 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206679&action=edit Bug 43523: Replace _skip_borrowers_with_nonzero_balance with SQL To test: 0. Add many borrowers to your system, I was testing with 10,000 each time 1. First without the patch, go to Tools -> Batch patron deletion and anonymization 2. Check 'Verify you want to delete patrons' and choose paramaters that will return most, or all, of the 10k borrowers. I used "who have not borrowed since: 1900/01/01" 3. Click Next >> and confirm. 4. The script should now take significant time to load. For me, with 10k borrowers, it was 3minutes 45 seconds 5. APPLY PATCH, restart_all 6. Load 10,000 new borrowers since the old ones are now deleted. 7. Try the steps again. 8. Load times should be better. For me, it now loads in under 1 second. 9. prove -v t/db_dependent/Members.t NOTE: This doesn't effect the actual deletion which still takes quite a long time. I have filed Bug 43525. as the deletion should be moved to a background job. Signed-off-by: Michelle Spinney <mspinney@clamsnet.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 --- Comment #7 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 206680 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206680&action=edit Bug 43523: Add a new test Signed-off-by: Michelle Spinney <mspinney@clamsnet.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |me@paulderscheid.xyz |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|me@paulderscheid.xyz |paul.derscheid@lmscloud.de CC| |paul.derscheid@lmscloud.de -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |zoho.roboto@bywatersolution | |s.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #206679|0 |1 is obsolete| | Attachment #206680|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 --- Comment #8 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 207561 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207561&action=edit Bug 43523: Replace _skip_borrowers_with_nonzero_balance with SQL To test: 0. Add many borrowers to your system, I was testing with 10,000 each time 1. First without the patch, go to Tools -> Batch patron deletion and anonymization 2. Check 'Verify you want to delete patrons' and choose paramaters that will return most, or all, of the 10k borrowers. I used "who have not borrowed since: 1900/01/01" 3. Click Next >> and confirm. 4. The script should now take significant time to load. For me, with 10k borrowers, it was 3minutes 45 seconds 5. APPLY PATCH, restart_all 6. Load 10,000 new borrowers since the old ones are now deleted. 7. Try the steps again. 8. Load times should be better. For me, it now loads in under 1 second. 9. prove -v t/db_dependent/Members.t NOTE: This doesn't effect the actual deletion which still takes quite a long time. I have filed Bug 43525. as the deletion should be moved to a background job. Signed-off-by: Michelle Spinney <mspinney@clamsnet.org> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 --- Comment #9 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 207562 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207562&action=edit Bug 43523: Add a new test Signed-off-by: Michelle Spinney <mspinney@clamsnet.org> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 --- Comment #10 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 207563 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207563&action=edit Bug 43523: (QA follow-up) Throttle Finish button - Use throttled-button.inc for the step 2 Finish button - Keep it disabled for 10 minutes: the deletion can run for minutes and a second submit races the first (Can't call method "delete" on an undefined value at tools/cleanborrowers.pl) Test plan: 1. Make sure there are many deletable patrons (thousands) 2. Tools > Batch patron deletion and anonymization, tick "Verify you want to delete patrons", set "who have not borrowed since" to today, click Next 3. Choose "Move these patrons to the trash", click Finish 4. The button shows a spinner and stays disabled until the result page loads 5. Repeat with "Permanently delete these patrons" and cancel the confirmation dialog: the button stays enabled Assisted-by: Opus 5.5 (Anthropic) Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 --- Comment #11 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 207564 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207564&action=edit Bug 43523: (QA follow-up) Document exclude_nonzero_balance Assisted-by: Opus 5.5 (Anthropic) Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 --- Comment #12 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Hi Lucas, good stuff. Here's some more stuff I found while testing this but should probably be a follow-up if we want to take this further. --- What follows was written by Claude Opus 5.5. Some numbers from QA on bug 43523 (KTD, BorrowersLog on, per patron, measured in rolled-back transactions): - Raw SQL DELETE incl. FK cascades: 0.28 ms - Koha::Patron->delete: 8.2 ms (5.7 ms with BorrowersLog off) - Holds/lists/modifications checks: ~2.5 ms, even when there is nothing to clean up - BorrowersLog payload (attributes, permissions): ~2.5 ms - find + move_to_deleted in the cleanborrowers.pl loop: ~3 ms So the time goes into per-patron ORM work, not the database. Suggestions: 1. Replace the find/move_to_deleted/delete loop with the existing set-level API, as cleanup_database.pl already does: Koha::Patrons->search( { borrowernumber => \@ids } ) ->delete( { move => $radio eq 'trash' } ); ~20% faster (9.7 -> 7.9 ms/patron), and it avoids "Can't call method "delete" on an undefined value" when two requests run concurrently (rows already deleted drop out of the set). It runs in a single transaction, so a background job should probably chunk it. 2. The background job itself is the real fix: at ~8 ms/patron, 100k patrons is still ~13 minutes, far beyond an HTTP request. 3. A larger speedup would need set-based pre-checks in Koha::Patron->delete (one query for which candidates have holds/lists/modifications), but that is a separate refactor. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |26.11.00 released in| | Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43523 --- Comment #13 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Thanks everyone! Pushed to main for 26.11! -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon@bugs.koha-community.org