Summary: | Too many borrower_relationships causes patron page to not load | ||
---|---|---|---|
Product: | Koha | Reporter: | Lucas Gass (lukeg) <lucas> |
Component: | Patrons | Assignee: | Lucas Gass (lukeg) <lucas> |
Status: | Pushed to stable --- | QA Contact: | Martin Renvoize (ashimema) <martin.renvoize> |
Severity: | normal | ||
Priority: | P5 - low | CC: | baptiste.wojtkowski, bdaeuber, dcook, gmcharlt, kyle.m.hall, lucas, martin.renvoize, nick, paul.derscheid |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35892 | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | --- | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: |
25.05.00,24.11.05
|
Circulation function: | |
Bug Depends on: | |||
Bug Blocks: | 39819 | ||
Attachments: |
Bug 39644: Skip processing guarantor if already seen
A script to make a bunch of Guarantor/guarantee relationships for Edna Acosta Bug 39644: Skip processing guarantor if already seen Bug 39644: Skip processing guarantor if already seen |
Description
Lucas Gass (lukeg)
2025-04-15 17:57:35 UTC
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. Maybe doing 'foreach my $guarantor (@guarantors)' and then an inner loop of ` foreach my $guarantee (@guarantees)` is part of the issue? 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. 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. Anyway got lots to do today, but that's where I'd look! Created attachment 181679 [details] [review] 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. 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 Created attachment 181683 [details]
A script to make a bunch of Guarantor/guarantee relationships for Edna Acosta
Created attachment 181685 [details] [review] 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. 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. (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? Not reproduced with 46 guarantors. How do you add so much guarantors ? > 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.
(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. (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. 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
> 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.
(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. (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. Created attachment 182082 [details] [review] 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> 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). Pushed for 25.05! Well done everyone, thank you! Nice work everyone! Pushed to 24.11.x for 24.11.05 |