Bug 39644

Summary: Too many borrower_relationships causes patron page to not load
Product: Koha Reporter: Lucas Gass (lukeg) <lucas>
Component: PatronsAssignee: 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
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.
Comment 1 Lucas Gass (lukeg) 2025-04-28 22:30:16 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.
Comment 2 Lucas Gass (lukeg) 2025-04-28 22:54:57 UTC
Maybe doing 'foreach my $guarantor (@guarantors)' and then an inner loop of ` foreach my $guarantee (@guarantees)` is part of the issue?
Comment 3 David Cook 2025-04-28 23:41:20 UTC
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.
Comment 4 David Cook 2025-04-28 23:42:35 UTC
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.
Comment 5 David Cook 2025-04-28 23:42:48 UTC
Anyway got lots to do today, but that's where I'd look!
Comment 6 Lucas Gass (lukeg) 2025-04-29 17:31:43 UTC
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.
Comment 7 Lucas Gass (lukeg) 2025-04-29 17:34:04 UTC
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
Comment 8 Lucas Gass (lukeg) 2025-04-29 17:38:56 UTC
Created attachment 181683 [details]
A script to make a bunch of Guarantor/guarantee relationships for Edna Acosta
Comment 9 Roman Dolny 2025-04-29 19:44:00 UTC
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.
Comment 10 Lucas Gass (lukeg) 2025-04-29 22:13:43 UTC
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.
Comment 11 David Cook 2025-04-29 23:23:51 UTC
(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?
Comment 12 Baptiste Wojtkowski (bwoj) 2025-04-30 12:21:13 UTC
Not reproduced with 46 guarantors. How do you add so much guarantors ?
Comment 13 Lucas Gass (lukeg) 2025-04-30 14:06:33 UTC
> 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.
Comment 14 Lucas Gass (lukeg) 2025-04-30 14:07:08 UTC
(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.
Comment 15 David Cook 2025-04-30 23:15:09 UTC
(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.
Comment 16 Nick Clemens (kidclamp) 2025-05-01 13:21:40 UTC
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
Comment 17 Benjamin Daeuber 2025-05-01 19:39:32 UTC
> 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.
Comment 18 David Cook 2025-05-01 23:21:44 UTC
(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.
Comment 19 Lucas Gass (lukeg) 2025-05-02 14:49:06 UTC
(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.
Comment 20 Martin Renvoize (ashimema) 2025-05-08 13:06:32 UTC
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>
Comment 21 Martin Renvoize (ashimema) 2025-05-08 13:07:27 UTC
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).
Comment 22 Katrin Fischer 2025-05-09 07:14:18 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 23 Paul Derscheid 2025-05-22 18:25:10 UTC
Nice work everyone!

Pushed to 24.11.x for 24.11.05