[Bug 41801] New: FixPriority recursive calls for lowestPriority holds can be removed
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 Bug ID: 41801 Summary: FixPriority recursive calls for lowestPriority holds can be removed Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Hold requests Assignee: koha-bugs@lists.koha-community.org Reporter: nick@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, lisette@bywatersolutions.com, tomascohen@gmail.com Currently, C4::Reserves->FixPriority adjusts the holds not marked for lowestPriority, then recursively calls itself for each lowestPriority hold, touching all the non-found holds on the record each time. This can be removed as we can simply sort the lowestPriority holds to the bottom when getting the list of holds to adjust -- 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=41801 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | -- 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=41801 Nick Clemens (kidclamp) <nick@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=41801 --- Comment #1 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 192820 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192820&action=edit Bug 41801: Remove recursive calls from FixPriority This patch removes the recursive calls that set each 'lowestPriority' hold to the bottom of the priority list and replaces it them by simply ordering the list of 'remaining' holds by lowestPriorty, then priority. A check is also added to ensure that if a lowestPriority hold is being adjusted, it cannot be moved above holds not marked as lowest priority. To test: 1 - Place many holds on a record, 50 - 150 https://github.com/kidclamp/handy-koha-script/blob/main/randhold.pl This will randomly make some of the holds lowest priority 2 - View the record in the staff interface: http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=312 3 - Click on the 'Holds' tab 4 - Adjust the priority of several holds using the dropdowns 5 - Note the timing 6 - Apply the patch 7 - Adjust the priority of several holds using the dropdowns 8 - Confirm the timing is improved 9 - Confirm that holds adjust as expected 10 - Confirm you cannot move a lowest priority hold above a hold not marked lowest priority 11 - Confirm holds move correctly on a record where all holds are lowestPriority 12 - Confirm holds move correctly on a record where no holds are lowestPriority -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=23269 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com Status|Needs Signoff |Patch doesn't apply --- Comment #2 from David Nind <david@davidnind.com> --- Getting the sha1/fake ancestor error when applying the patch. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 --- Comment #3 from David Nind <david@davidnind.com> --- Testing notes: 1. Using the script: perl ./randhold.pl -b 312 -n 50 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff --- Comment #4 from David Nind <david@davidnind.com> --- (In reply to David Nind from comment #2)
Getting the sha1/fake ancestor error when applying the patch.
My bad - I was in the wrong git repository! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #5 from David Nind <david@davidnind.com> --- I don't think this is working correctly: 1. Place 150 holds: perl ./randhold.pl -b 312 -n 150 2. Note the details for the first hold (priority = 1) (I copied the info in the note column) 3. Click the button to move the hold with priority 1 to "Move hold to the bottom" 4. It becomes the hold with priority of 79, instead of the bottom, with is hold with the priority of 150 5. I tried this in reverse, the bottom one to "Move hold to the top" (priority = 150), this became the hold with priority = 80 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 --- Comment #6 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- (In reply to David Nind from comment #5)
I don't think this is working correctly: 1. Place 150 holds: perl ./randhold.pl -b 312 -n 150 2. Note the details for the first hold (priority = 1) (I copied the info in the note column) 3. Click the button to move the hold with priority 1 to "Move hold to the bottom" 4. It becomes the hold with priority of 79, instead of the bottom, with is hold with the priority of 150 5. I tried this in reverse, the bottom one to "Move hold to the top" (priority = 150), this became the hold with priority = 80
Hi David, Can you confirm the requests you are looking at are marked as 'lowest priority'? The column between 'Details' and 'Delete' has two down arrows pointing down - when there is a line udner this, the hold is marked 'lowest priority' and cannot be moved above a hold that is not marked this way, and vice versa. These are the holds that cause the performance hit and are expected to work this way -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 --- Comment #7 from David Nind <david@davidnind.com> --- (In reply to Nick Clemens (kidclamp) from comment #6)
(In reply to David Nind from comment #5)
I don't think this is working correctly: 1. Place 150 holds: perl ./randhold.pl -b 312 -n 150 2. Note the details for the first hold (priority = 1) (I copied the info in the note column) 3. Click the button to move the hold with priority 1 to "Move hold to the bottom" 4. It becomes the hold with priority of 79, instead of the bottom, with is hold with the priority of 150 5. I tried this in reverse, the bottom one to "Move hold to the top" (priority = 150), this became the hold with priority = 80
Hi David,
Can you confirm the requests you are looking at are marked as 'lowest priority'? The column between 'Details' and 'Delete' has two down arrows pointing down - when there is a line udner this, the hold is marked 'lowest priority' and cannot be moved above a hold that is not marked this way, and vice versa.
These are the holds that cause the performance hit and are expected to work this way
I was testing in the wrong place 8-) I was using the tick box (first column), Priority (second column), and Change priority (third column) to change the priority; not the double down arrow between the Details and Delete columns. The patch does indead improve the performance, from around 20 seconds to 2 seconds. However, I don't really understand what it is doing, so will leave to someone who is more familiar with it to test. Observed behavour: 1. The holds with the "Set lowest priority" icon (double down arrow without the underline) are listed with a priority of 1 to 64 2. Action performed: "Set lowest priority" for the first hold in the list: - Had a priority of 1, priority changed to 150, label for arrow was "Set lowest priority", moves down to bottom of the holds list, 2.08s - Icon doesn't change: still says "Set lowest priority" 3. Action performed: "Remove lowest priority" for the item with a current priority of 149: - Had a priority of 149, priority changed to 150, 2.14s - Icon changed to "Set lowest priority" - The hold that previously had a priority of 150 (from step 2), now has a priorty of 65, with the "Set lowest priority" icon (no change to icon) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> 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=41801 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192820|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=41801 --- Comment #8 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194985 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194985&action=edit Bug 41801: Remove recursive calls from FixPriority This patch removes the recursive calls that set each 'lowestPriority' hold to the bottom of the priority list and replaces it them by simply ordering the list of 'remaining' holds by lowestPriorty, then priority. A check is also added to ensure that if a lowestPriority hold is being adjusted, it cannot be moved above holds not marked as lowest priority. To test: 1 - Place many holds on a record, 50 - 150 https://github.com/kidclamp/handy-koha-script/blob/main/randhold.pl This will randomly make some of the holds lowest priority 2 - View the record in the staff interface: http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=312 3 - Click on the 'Holds' tab 4 - Adjust the priority of several holds using the dropdowns 5 - Note the timing 6 - Apply the patch 7 - Adjust the priority of several holds using the dropdowns 8 - Confirm the timing is improved 9 - Confirm that holds adjust as expected 10 - Confirm you cannot move a lowest priority hold above a hold not marked lowest priority 11 - Confirm holds move correctly on a record where all holds are lowestPriority 12 - Confirm holds move correctly on a record where no holds are lowestPriority Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 --- Comment #9 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194986 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194986&action=edit Bug 41801: (QA follow-up) Remove dead ignoreSetLowestRank param and update POD The recursive self-call loop that ignoreSetLowestRank guarded against was removed by the main patch. Remove the now-unused parameter from both the function body and the POD documentation, and fix a double-word typo in the POD while here. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 --- Comment #10 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194987 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194987&action=edit Bug 41801: (QA follow-up) Guard lowestPriority rank clamping against empty rank When FixPriority is called with a reserve_id but no rank (rank defaults to ''), the empty string evaluates to 0 numerically, satisfying `$rank <= $highest_non_lowest_priority`. This caused the clamping block to unexpectedly set rank to a positive value, which then triggered an unintended splice and moved the hold. Add `$rank > 0` to the guard condition so the block only fires when an explicit positive rank was requested, matching the pre-existing semantics of the splice block below it. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 --- Comment #11 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194988 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194988&action=edit Bug 41801: (QA follow-up) Add automated tests for FixPriority lowestPriority behaviour Cover the four scenarios identified during QA: - lowestPriority hold cannot be moved above non-lowestPriority holds (rank is clamped to just after the last non-lowest hold) - lowestPriority hold can be reordered within the lowest-priority group - when all holds are lowestPriority, movement is unconstrained - when no holds are lowestPriority, FixPriority works as before Holds are created with explicit priorities to avoid non-deterministic tie-breaking in ORDER BY when all new holds default to priority=1. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 --- Comment #12 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194989 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194989&action=edit Bug 41801: (QA follow-up) Replace MAX subquery with in-memory array count for rank clamping The lowestPriority rank-clamping block fired a second SQL query to get max(priority) WHERE lowestPriority=0 and used that value as both a threshold and a splice index. This was fragile: if priorities had become discontiguous (e.g. due to a race or a bug elsewhere), the DB priority value would not match the 1-based position in @priority, and the splice could land the hold in the wrong slot. The @priority array already contains all the information needed: it is sorted lowestPriority ASC so the count of rows where lowestPriority=0 is exactly the number of non-lowest holds, which equals the last valid rank for a normal hold. Fetch lowestPriority in the existing SELECT and use scalar grep { !$_->{lowestPriority} } @priority instead. This eliminates the extra DB round-trip and makes the clamping operate on the same data snapshot that will be written back. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 --- Comment #13 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194990 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194990&action=edit Bug 41801: (QA follow-up) Prevent race condition with txn_do and FOR UPDATE FixPriority has a read-modify-write structure: it SELECTs all active holds for a biblionumber, computes new priority order in Perl, then issues N UPDATEs. Two concurrent callers (e.g. two patrons adjusting holds on a popular title at the same moment) could both read the same snapshot and then overwrite each other's renumbering, leaving duplicate or incorrect priority values. Fix by: 1. Wrapping the entire function body in txn_do so it runs atomically. txn_do uses MySQL savepoints when called inside an existing transaction, so callers that already hold a transaction are unaffected. 2. Adding FOR UPDATE to the initial SELECT so the first caller acquires row-level locks on all holds for that bib. A second concurrent caller will block at the SELECT until the first commits, then reads the already-corrected priorities and produces a consistent result. Note: FOR UPDATE locks only rows that exist at read time; a concurrent INSERT of a brand-new hold can still slip through, but priority assignment for new holds goes via AddReserve → FixPriority, which will also acquire the lock and renumber correctly. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@openfifth.c | |o.uk --- Comment #14 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- OK, code looks good to me.. I added a few bits of polish to it, hopefully split out to be clear what each one does. Would appreciate a final review -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #15 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- The followups all look good, but in further testing I found that toggling lowestPriority off moves a hold to the bottom - then you can move it up one by one and it doesn't bubble up to where it should - this needs a fix -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |23269 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23269 [Bug 23269] Long hold queues are slowing the service -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 --- Comment #16 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 195131 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195131&action=edit Bug 41801: (QA folllow-up) Ensure toggling priority sets correct rank -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 --- Comment #17 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- I made this depend on 23269 to fix lowest priority toggle in the correct place -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |This performance update release notes| |optimizes the | |C4::Reserves->FixPriority | |function. Previously, when | |adjusting holds on a record | |with many "lowest priority" | |requests, Koha would use | |recursive calls that | |repeatedly touched every | |hold on the record. This | |caused significant | |slowdowns (lag) when | |managing large hold queues. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Medium patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |26.05.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=41801 --- Comment #18 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Thanks for all the hard work! Pushed to main for the next 26.05.00 release as RM Assistant -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 Jacob O'Mara <jacob.omara@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|26.05.00 |26.05.00,25.11.05 released in| | Status|Pushed to main |Pushed to stable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41801 --- Comment #19 from Jacob O'Mara <jacob.omara@openfifth.co.uk> --- Thanks all, pushed to 25.11.x -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org