[Bug 39644] New: Too many borrower_relationships causes patron page to not load
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 Bug ID: 39644 Summary: Too many borrower_relationships causes patron page to not load Change sponsored?: --- Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Patrons Assignee: koha-bugs@lists.koha-community.org Reporter: lucas@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com To recreate: In k-t-d, if I add 53 guarantees for Edna Acosta it takes about 8-10 seconds for the patron page to load. If I add 700 guarantees for Edna Acosta the page won't load and I ultimately get a time out. -- 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=39644 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au, | |lucas@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 --- Comment #1 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- I think the bottleneck is sub relationships_debt in Koha/Patron.pm. It is called inside of the 'is_patron_inside_charge_limits' and seems to bog things down significantly. -- 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=39644 --- Comment #2 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Maybe doing 'foreach my $guarantor (@guarantors)' and then an inner loop of ` foreach my $guarantee (@guarantees)` is part of the issue? -- 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=39644 --- Comment #3 from David Cook <dcook@prosentient.com.au> --- Sounds interesting. I'm not loving the code in "sub relationships_debt". Take a look at this section: 7a7a0a2474c Koha/Patron.pm (Kyle M Hall 2020-01-10 11:59:56 -0500 622) # I am a guarantor, I need to get all the guarantors of all my guarantees d659526b5ae Koha/Patron.pm (Koha Development Team 2025-01-27 13:21:28 +0100 623) @guarantors = map { $_->guarantor_relationships->guarantors->as_list } d659526b5ae Koha/Patron.pm (Koha Development Team 2025-01-27 13:21:28 +0100 624) $self->guarantee_relationships->guarantees->as_list; It looks like $self->guarantee_relationships->guarantees->as_list does 1 query to get all the guarantee relationships, fetches the guarantee IDs as a list, then fetches the borrowers using those IDs. So that's 2 queries that should get us 700 results. Probably not too bad. I don't recall the DBIC internals for as_list. I don't know if that fetches 1 by 1 or all at once. $_->guarantor_relationships->guarantors->as_list then does 1 query to get the guarantor relationships, which should be 1, deduplicates down to 1, and then fetches the borrower using that ID. 2 queries again. so (700x2) + (700x2) = 2800 DB queries. And if I'm not mistaken... @guarantors should now have 700 identical entries? We then loop through 700 of the same guarantors... but don't get duplicate charges because of the $seen variable. But then that means for 700 guarantors we're re-fetching the guarantee relationships in "my @guarantees = map { $_->guarantee } $guarantor->guarantee_relationships->as_list;" So now we have 700 guarantees. 700 guarantors times 700 guarantees = 490,000 iterations... although we're not re-counting the guarantees against because of the $seen variable. But yeah... that seems like a lot of work. -- 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=39644 --- Comment #4 from David Cook <dcook@prosentient.com.au> --- Now I could be wrong. This is just based off a code review. I haven't tested it. But I'd look at that @guarantors variable, and if it does have duplicates... deduplicating before looping would be good. Or if we don't deduplicate, doing a "next if $seen->{ $guarantor->id }" at the very least would be helpful I imagine. -- 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=39644 --- Comment #5 from David Cook <dcook@prosentient.com.au> --- Anyway got lots to do today, but that's where I'd look! -- 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=39644 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- 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=39644 --- Comment #6 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 181679 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=181679&action=edit Bug 39644: Skip processing guarantor if already seen To test: 1. In k-t-d try loading both member/moremember.pl and circ/circulation.pl for a patron with many guarantors. 2. Note the load times. 3. APPLY PATCH 4. Load times should now be reduced pretty significantly. 5. prove -v /kohadevbox/koha/t/db_dependent/Koha/Patron.t 6. No behavior should change. -- 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=39644 --- Comment #7 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- I made every patron in the default k-t-d a guarantor of Edna Acosta ( 53 guarantors ) Without the patch members/moremember.pl loads in 11236ms With the patch members/moremember.pl load in 2931ms -- 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=39644 --- Comment #8 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 181683 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=181683&action=edit A script to make a bunch of Guarantor/guarantee relationships for Edna Acosta -- 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=39644 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=39644 Roman Dolny <roman.dolny@jezuici.pl> 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=39644 Roman Dolny <roman.dolny@jezuici.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #181679|0 |1 is obsolete| | --- Comment #9 from Roman Dolny <roman.dolny@jezuici.pl> --- Created attachment 181685 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=181685&action=edit Bug 39644: Skip processing guarantor if already seen To test: 1. In k-t-d try loading both member/moremember.pl and circ/circulation.pl for a patron with many guarantors. 2. Note the load times. 3. APPLY PATCH 4. Load times should now be reduced pretty significantly. 5. prove -v /kohadevbox/koha/t/db_dependent/Koha/Patron.t 6. No behavior should change. Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Test/SO note: before 12, after 3 seconds. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 --- Comment #10 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- With 325 guarantees for Edna Acosta it times out without the patch With the patch it loads in 9747ms, not great but at least it loads. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 --- Comment #11 from David Cook <dcook@prosentient.com.au> --- (In reply to Lucas Gass (lukeg) from comment #10)
With 325 guarantees for Edna Acosta it times out without the patch
With the patch it loads in 9747ms, not great but at least it loads.
Yeah I think there's a number of further optimizations that could probably be done to this script. Just out of curiosity how'd you bump into this one? I can't imagine that having many guarantees is that common? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |baptiste.wojtkowski@biblibr | |e.com --- Comment #12 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Not reproduced with 46 guarantors. How do you add so much guarantors ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 --- Comment #13 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> ---
Just out of curiosity how'd you bump into this one? I can't imagine that having many guarantees is that common?
It seems to be fairly common for law libraries to use the feature with law firms ( guarantor ) and lawyers ( guarantees ). Sometimes this can mean hundreds of guarantees. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 --- Comment #14 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- (In reply to Baptiste Wojtkowski (bwoj) from comment #12)
Not reproduced with 46 guarantors. How do you add so much guarantors ?
Hey Baptiste, I did attach a script to this report that will add many guarantors. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 --- Comment #15 from David Cook <dcook@prosentient.com.au> --- (In reply to Lucas Gass (lukeg) from comment #13)
Just out of curiosity how'd you bump into this one? I can't imagine that having many guarantees is that common?
It seems to be fairly common for law libraries to use the feature with law firms ( guarantor ) and lawyers ( guarantees ). Sometimes this can mean hundreds of guarantees.
That makes sense. Many years ago, I used to work at a private law firm library, and we'd borrow from the courthouse library. Can't recall the particulars of the borrower setup, but that model would make sense. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 --- Comment #16 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- I think there is more cleanup to be done here. relationships_debt is only called twice in the code base: once in moremember.pl once in Koha::Patron->is_patron_inside_charge_limits (which is also called in moremember) The two calls are: - moremember: $patron->relationships_debt( { include_guarantors => 0, only_this_guarantor => 1, include_this_patron => 1 } ); - is_patron_inside_charge_limits: $patron->relationships_debt( { include_guarantors => 1, only_this_guarantor => 0, include_this_patron => 1 } ); The difference being that the first returns only the single patrons fine + their guarantees, the latter returns the patron and thier gusarantees and the guarantees other guarantors The first being a subset of the second, we should be able to return this additional info from "is_patron_inside_charge_limits" and avoid a double call. Additionally, moremember does checks against "NoIssuesChargeGuarantorsWithGuarantees" and "NoIssuesChargeGuarantees" - both of which are checked inside "is_patron_inside_charge_limits" so we can simply use values we are already calculating and not calculate them again -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 Benjamin Daeuber <bdaeuber@cityoffargo.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |bdaeuber@cityoffargo.com --- Comment #17 from Benjamin Daeuber <bdaeuber@cityoffargo.com> ---
Just out of curiosity how'd you bump into this one? I can't imagine that having many guarantees is that common?
We do a number of lobby stops with group homes or nursing facilities. The general practice for us to to make each resident at the site a guarantee on the site's account. That way any fines can be dealt with centrally, and the staff can see all the items that are at that site at a given time, but we also know what each resident has taken out and if they've got items. It's actually very well set up for our outreach operation, except for the speed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 --- Comment #18 from David Cook <dcook@prosentient.com.au> --- (In reply to Nick Clemens (kidclamp) from comment #16)
I think there is more cleanup to be done here.
Yeah, I eyeballed a few places that could use refactoring for sure, although I think those could be follow-up reports. I've had a lot on my plate, but I'd be happy to QA the current patch as is. We can call it part 1 and do another one for part 2 and so on I reckon. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |39819 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39819 [Bug 39819] Additional performance improvements for when loading borrower relationships. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|39819 | --- Comment #19 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- (In reply to David Cook from comment #18)
(In reply to Nick Clemens (kidclamp) from comment #16)
I think there is more cleanup to be done here.
Yeah, I eyeballed a few places that could use refactoring for sure, although I think those could be follow-up reports.
I've had a lot on my plate, but I'd be happy to QA the current patch as is. We can call it part 1 and do another one for part 2 and so on I reckon.
Thanks David, I filed Bug 39819 as a dependency of this bug. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39819 [Bug 39819] Additional performance improvements when loading borrower relationships. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |39819 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39819 [Bug 39819] Additional performance improvements when loading borrower relationships. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |martin.renvoize@openfifth.c |y.org |o.uk CC| |martin.renvoize@openfifth.c | |o.uk -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #181685|0 |1 is obsolete| | --- Comment #20 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 182082 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=182082&action=edit Bug 39644: Skip processing guarantor if already seen To test: 1. In k-t-d try loading both member/moremember.pl and circ/circulation.pl for a patron with many guarantors. 2. Note the load times. 3. APPLY PATCH 4. Load times should now be reduced pretty significantly. 5. prove -v /kohadevbox/koha/t/db_dependent/Koha/Patron.t 6. No behavior should change. Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> 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=39644 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #21 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Fully agree we can go further, but that should be done in a follow-up bug to prevent this already significant improvement from further delay. Passing QA, no regressions found, QA scripts happy and Unit Tests passing (and covering the use case well). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |25.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 --- Comment #22 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 25.05! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_24_11_candidate -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=35892 CC| |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|25.05.00 |25.05.00,24.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=39644 --- Comment #23 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Nice work everyone! Pushed to 24.11.x for 24.11.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |paul.derscheid@lmscloud.de Keywords|rel_24_11_candidate | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39644 Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |wainuiwitikapark@catalyst.n | |et.nz Status|Pushed to stable |Needs documenting --- Comment #24 from Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> --- Not backporting to 22.11.x as it wasn't backported to 24.05.x -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org