Bug 29474 - Automatic renewals cronjob is slow on systems with large numbers of reserves
Summary: Automatic renewals cronjob is slow on systems with large numbers of reserves
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 29483 29537
  Show dependency treegraph
 
Reported: 2021-11-13 14:42 UTC by Nick Clemens
Modified: 2021-11-23 12:00 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 29474: Return too_soon before checking renewals (1.87 KB, patch)
2021-11-13 15:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Return too_soon before checking renewals (2.49 KB, patch)
2021-11-13 18:50 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Return too_soon before checking renewals (2.53 KB, patch)
2021-11-13 19:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Check ItemsAnyAvailableAndNOtRestricted outside of loop (2.07 KB, patch)
2021-11-13 20:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Return too_soon before checking renewals (2.53 KB, patch)
2021-11-14 12:48 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29474: Fix incorrect comment (957 bytes, patch)
2021-11-14 12:48 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed (3.45 KB, patch)
2021-11-14 12:49 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29474: Remove one layer of indendation by adding if check in the begginning (5.72 KB, patch)
2021-11-14 12:49 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29474: Return too_soon before checking renewals (2.10 KB, patch)
2021-11-15 12:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed (3.45 KB, patch)
2021-11-15 12:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Remove one layer of indendation by adding if check in the begginning (5.72 KB, patch)
2021-11-15 12:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Pass rules/branchcode to subroutines, further simplify logic (5.65 KB, patch)
2021-11-15 12:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Small efficiency improvement (1.06 KB, patch)
2021-11-15 15:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29474: Return too_soon before checking renewals (2.25 KB, patch)
2021-11-20 12:14 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed (3.45 KB, patch)
2021-11-20 12:14 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29474: Remove one layer of indendation by adding if check in the begginning (5.72 KB, patch)
2021-11-20 12:14 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29474: Pass rules/branchcode to subroutines, further simplify logic (5.79 KB, patch)
2021-11-20 12:14 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29474: Small efficiency improvement (1.19 KB, patch)
2021-11-20 12:14 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29474: Return too_soon before checking renewals (2.25 KB, patch)
2021-11-23 12:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed (3.50 KB, patch)
2021-11-23 12:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Remove one layer of indendation by adding if check in the begginning (5.78 KB, patch)
2021-11-23 12:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Pass rules/branchcode to subroutines, further simplify logic (5.79 KB, patch)
2021-11-23 12:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29474: Small efficiency improvement (1.19 KB, patch)
2021-11-23 12:00 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-11-13 14:42:47 UTC
When a site has many holds the automatic renewals cron can take a long time to run

This is increased if the system preference 'AllowRenewalIfOtherItemsAvailable' is set to allow
Comment 1 Nick Clemens 2021-11-13 15:29:31 UTC
Created attachment 127610 [details] [review]
Bug 29474: Return too_soon before checking renewals

This patch allows us to avoid checking reserves when the issue is
too_soon and we are running the cron

Code is covered by existing tests

To test:
1 - prove -v t/db_dependent/Circulation.t
Comment 2 Joonas Kylmälä 2021-11-13 16:06:37 UTC
If you remove the unless then you can delete the line

> return ( 0, "on_reserve" ) if $resfound && $cron;    # '' when no hold was found

Makes the code much simpler.

Please also move the cron return right after the call to 

> $auto_renew = _CanBookBeAutoRenewed({

so that now that the return value decision is not in an if-else condition at the end the requirement for returning the values in a different order is clear and the order won't be changed by some other developer accidentally. I think adding a comment like "cron wants 'too_soon' over 'on_reserve'"

Please include stats too, but maybe you are at it, I noticed that this bug is not even yet for sign-off and I'm here reviewing ;D
Comment 3 Joonas Kylmälä 2021-11-13 16:12:04 UTC
(In reply to Joonas Kylmälä from comment #2)
>  I think adding a comment like "cron wants 'too_soon' over 'on_reserve'"

This sentence was unfinished.. I meant adding this comment next to the code when returning the 'too_soon' reason near the _CanBookBeAutoRenewed function call.
Comment 4 Nick Clemens 2021-11-13 18:50:50 UTC
Created attachment 127611 [details] [review]
Bug 29474: Return too_soon before checking renewals

This patch allows us to avoid checking reserves when the issue is
too_soon and we are running the cron

Code is covered by existing tests

To test:
1 - prove -v t/db_dependent/Circulation.t
Comment 5 Nick Clemens 2021-11-13 18:53:23 UTC
(In reply to Joonas Kylmälä from comment #2)
> If you remove the unless ... makes the code much simpler.

Done

> Please also move the cron return right after the call to 
> > $auto_renew = _CanBookBeAutoRenewed({

I don't believe I can. That is inside "unless ($override_limit)" so it would make 'too_soon' subject to override

> Please include stats too, but maybe you are at it, I noticed that this bug
> is not even yet for sign-off and I'm here reviewing ;D

Yes, seeing what else I can improve first, will add when ready.

Feel free to add to/alter my patches - I appreciate the help!
Comment 6 Nick Clemens 2021-11-13 19:23:23 UTC
Created attachment 127612 [details] [review]
Bug 29474: Return too_soon before checking renewals

This patch allows us to avoid checking reserves when the issue is
too_soon and we are running the cron

Code is covered by existing tests

To test:
1 - prove -v t/db_dependent/Circulation.t
Comment 7 Nick Clemens 2021-11-13 19:23:59 UTC
(In reply to Nick Clemens from comment #5)
> > Please also move the cron return right after the call to 
> > > $auto_renew = _CanBookBeAutoRenewed({
> 
> I don't believe I can. That is inside "unless ($override_limit)" so it would
> make 'too_soon' subject to override
> 
I was wrong, done
Comment 8 Nick Clemens 2021-11-13 20:01:38 UTC
Created attachment 127613 [details] [review]
Bug 29474: Check ItemsAnyAvailableAndNOtRestricted outside of loop

IsAvailableForItemLevelRequest takes $items_any_available to avoid
a double loop - this was done in request.pl, but it makes sense
to do the same here
Comment 9 Nick Clemens 2021-11-13 20:51:45 UTC
5 renewals allowed
No renewal before unset (i.e. not before due)

For a single biblio
100 items on record
10 items checked out and marked auto_renew
10 record level holds

Without these patches:
~3.7 second

With these patches
~1.5 second


Then I set 'On shelf holds allowed' to 'If all unavailable'

Without these patches:
~3 MINUTES!

With these patches
~1.5 seconds

To further test 'ItemsAnyAvailable' I set all the items as overdue, changed nothing else

Without these patches:
~3 minutes

With these patches
~1.25 minutes
Comment 10 Joonas Kylmälä 2021-11-14 07:55:17 UTC
The last patch is wrong, it checks the availability for the patron who is wanting to do the renewal and not the other patrons as it should (notice that the $patron variable is redefined here). You could however achieve the performance boost by adding the $items_any_available value to a hash table where the keys are borrowernumbers. However, I think we might be able to avoid that (it makes the code pretty ugly imho if we use the hash table) by splitting somehow the while loops and maybe ordering it so that we iterate patrons in the outer loop. Like I think "next if IsItemOnHoldAndFound( $item->itemnumber );" could be moved to its own loop.
Comment 11 Joonas Kylmälä 2021-11-14 08:26:38 UTC
oh, I think I discovered a bug in the CanBookBeRenewed function. We are calling

> next unless IsAvailableForItemLevelRequest($item, $patron); 

and we are not passing the ignore_hold_counts=>1 here even though we should be. The patch "Bug 28013: (follow-up) Correct and update call to CanItemBeReserved" which is pushed already fixed it for the CanItemBeReserved() function call just the line below but this has the same bug.

If you follow this code path you will notice the bug:

IsAvailableForItemLevelRequest -> ItemsAnyAvailableAndNotRestricted -> CanItemBeReserved

So this needs to kept also in mind while doing the perf work here.
Comment 12 Joonas Kylmälä 2021-11-14 10:45:49 UTC
In the first patch you added the comment:

# override_limit will override anything else except on_reserve or too_soon

This is actually not what the code does, but it should do this I think, it used to do this but after the patch "Bug 19014: Return auto_too_soon before on_reserve" (51f5f6) the behaviour was changed. It was unadvertised change so it seems like a regression introduced by bug 19014. So actually the too_soon return should be outside the $override_limit if block as you had it the first time but before that we need to fix the regression from bug 19014.
Comment 13 Joonas Kylmälä 2021-11-14 10:53:29 UTC
(In reply to Joonas Kylmälä from comment #12)
> In the first patch you added the comment:
> 
> # override_limit will override anything else except on_reserve or too_soon
> 
> This is actually not what the code does, but it should do this I think, it
> used to do this but after the patch "Bug 19014: Return auto_too_soon before
> on_reserve" (51f5f6) the behaviour was changed. It was unadvertised change
> so it seems like a regression introduced by bug 19014. So actually the
> too_soon return should be outside the $override_limit if block as you had it
> the first time but before that we need to fix the regression from bug 19014.

Hmm, actually there is no regression, there was the $override_limit check also before bug 19014. So the comment you added should be removed. Sorry for the noise, the code is just so hard to understand...
Comment 14 Joonas Kylmälä 2021-11-14 12:48:54 UTC
Created attachment 127614 [details] [review]
Bug 29474: Return too_soon before checking renewals

This patch allows us to avoid checking reserves when the issue is
too_soon and we are running the cron

Code is covered by existing tests

To test:
1 - prove -v t/db_dependent/Circulation.t
Comment 15 Joonas Kylmälä 2021-11-14 12:48:57 UTC
Created attachment 127615 [details] [review]
Bug 29474: Fix incorrect comment

The too_soon is actually being overriden if $override_limit is true.
Comment 16 Joonas Kylmälä 2021-11-14 12:49:01 UTC
Created attachment 127616 [details] [review]
Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed

The too_soon check does not relate at all to auto-renewed issues and
should be moved outside the _CanBookBeAutoRenewed() function.

To test:
   1) prove t/db_dependent/Circulation.t
Comment 17 Joonas Kylmälä 2021-11-14 12:49:04 UTC
Created attachment 127617 [details] [review]
Bug 29474: Remove one layer of indendation by adding if check in the begginning

If we simply return "no" immediately from the function when the
checkout is not an autorenewed checkout we can drop one layer of
indendation and the code becomes much easier to read.

To test:
 1) prove t/db_dependent/Circulation.t
Comment 18 Joonas Kylmälä 2021-11-14 13:01:17 UTC
I fixed the incorrect comment about too_soon not being overriden and added couple improvements to the code as well. I obsoleted also the patch from comment #8 now as it was buggy (see comment #10). I could try to work on those perf improvements as what was tried in the patch attached in comment #8 but I'm not sure I will manage to do that in the nearby days so please go ahead and have a new attempt at it as if I will it might take time.
Comment 19 Nick Clemens 2021-11-15 12:14:27 UTC
Created attachment 127632 [details] [review]
Bug 29474: Return too_soon before checking renewals

This patch allows us to avoid checking reserves when the issue is
too_soon and we are running the cron

Code is covered by existing tests

To test:
1 - prove -v t/db_dependent/Circulation.t
Comment 20 Nick Clemens 2021-11-15 12:14:30 UTC
Created attachment 127633 [details] [review]
Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed

The too_soon check does not relate at all to auto-renewed issues and
should be moved outside the _CanBookBeAutoRenewed() function.

To test:
   1) prove t/db_dependent/Circulation.t
Comment 21 Nick Clemens 2021-11-15 12:14:33 UTC
Created attachment 127634 [details] [review]
Bug 29474: Remove one layer of indendation by adding if check in the begginning

If we simply return "no" immediately from the function when the
checkout is not an autorenewed checkout we can drop one layer of
indendation and the code becomes much easier to read.

To test:
 1) prove t/db_dependent/Circulation.t
Comment 22 Nick Clemens 2021-11-15 12:14:37 UTC
Created attachment 127635 [details] [review]
Bug 29474: Pass rules/branchcode to subroutines, further simplify logic

If we are going to decouple 'too_soon' from auto_renewals we should only call GetSoonestDate once

Additionally, we can fetch all needed rules at once and pass to subroutines as needed

Rather than check in the sub, let's not call _CanBookBeAutoRenewed if we aren't set for auto_renew
Comment 23 Nick Clemens 2021-11-15 12:35:30 UTC
(In reply to Joonas Kylmälä from comment #18)
> I fixed the incorrect comment about too_soon not being overriden and added
> couple improvements to the code as well. 

Excellent! Thanks! I squashed the comment removal, I thought I had pulled that, I was confused when I added it :-)

> I obsoleted also the patch from
> comment #8 now as it was buggy (see comment #10). I could try to work on
> those perf improvements as what was tried in the patch attached in comment
> #8 but I'm not sure I will manage to do that in the nearby days so please go
> ahead and have a new attempt at it as if I will it might take time.

I am going to move that to it's own bug since it will take a little more reworking.
Comment 24 Kyle M Hall 2021-11-15 15:53:09 UTC
Created attachment 127658 [details] [review]
Bug 29474: Small efficiency improvement
Comment 25 Joonas Kylmälä 2021-11-20 11:40:26 UTC
The renewal date calculation is buggy for auto-renewals, I made a fix for that a few days ago in bug 29476. I will mark this bug to now depend on that bug as "Bug 29474: Pass rules/branchcode to subroutines, further simplify logic" uses now GetSoonestRenewDate also for auto-renewals as well, thus requires the bug fix. Also, now that we depend on bug 29476 I can make at least one further simplification in the code, will attach a patch shortly.
Comment 26 Joonas Kylmälä 2021-11-20 12:07:50 UTC
(In reply to Joonas Kylmälä from comment #25)
> The renewal date calculation is buggy for auto-renewals, I made a fix for
> that a few days ago in bug 29476. I will mark this bug to now depend on that
> bug as "Bug 29474: Pass rules/branchcode to subroutines, further simplify
> logic" uses now GetSoonestRenewDate also for auto-renewals as well, thus
> requires the bug fix. Also, now that we depend on bug 29476 I can make at
> least one further simplification in the code, will attach a patch shortly.

After further verification, there is no dependency so I created a completely new bug report for that: bug 29537. It should help *tiny* bit with performance as well so you might consider signing-off that.
Comment 27 Joonas Kylmälä 2021-11-20 12:14:08 UTC
Created attachment 127903 [details] [review]
Bug 29474: Return too_soon before checking renewals

This patch allows us to avoid checking reserves when the issue is
too_soon and we are running the cron

Code is covered by existing tests

To test:
1 - prove -v t/db_dependent/Circulation.t

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 28 Joonas Kylmälä 2021-11-20 12:14:12 UTC
Created attachment 127904 [details] [review]
Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed

The too_soon check does not relate at all to auto-renewed issues and
should be moved outside the _CanBookBeAutoRenewed() function.

To test:
   1) prove t/db_dependent/Circulation.t
Comment 29 Joonas Kylmälä 2021-11-20 12:14:16 UTC
Created attachment 127905 [details] [review]
Bug 29474: Remove one layer of indendation by adding if check in the begginning

If we simply return "no" immediately from the function when the
checkout is not an autorenewed checkout we can drop one layer of
indendation and the code becomes much easier to read.

To test:
 1) prove t/db_dependent/Circulation.t
Comment 30 Joonas Kylmälä 2021-11-20 12:14:19 UTC
Created attachment 127906 [details] [review]
Bug 29474: Pass rules/branchcode to subroutines, further simplify logic

If we are going to decouple 'too_soon' from auto_renewals we should only call GetSoonestDate once

Additionally, we can fetch all needed rules at once and pass to subroutines as needed

Rather than check in the sub, let's not call _CanBookBeAutoRenewed if we aren't set for auto_renew

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 31 Joonas Kylmälä 2021-11-20 12:14:23 UTC
Created attachment 127907 [details] [review]
Bug 29474: Small efficiency improvement

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 32 Joonas Kylmälä 2021-11-20 12:15:31 UTC
Added sign-off to all the patches that were not from me, Nick or Kyle, you could sign-off the patches from me to get this to signed-off state :)
Comment 33 Nick Clemens 2021-11-23 12:00:08 UTC
Created attachment 127949 [details] [review]
Bug 29474: Return too_soon before checking renewals

This patch allows us to avoid checking reserves when the issue is
too_soon and we are running the cron

Code is covered by existing tests

To test:
1 - prove -v t/db_dependent/Circulation.t

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 34 Nick Clemens 2021-11-23 12:00:12 UTC
Created attachment 127950 [details] [review]
Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed

The too_soon check does not relate at all to auto-renewed issues and
should be moved outside the _CanBookBeAutoRenewed() function.

To test:
   1) prove t/db_dependent/Circulation.t

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 35 Nick Clemens 2021-11-23 12:00:15 UTC
Created attachment 127951 [details] [review]
Bug 29474: Remove one layer of indendation by adding if check in the begginning

If we simply return "no" immediately from the function when the
checkout is not an autorenewed checkout we can drop one layer of
indendation and the code becomes much easier to read.

To test:
 1) prove t/db_dependent/Circulation.t

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 36 Nick Clemens 2021-11-23 12:00:18 UTC
Created attachment 127952 [details] [review]
Bug 29474: Pass rules/branchcode to subroutines, further simplify logic

If we are going to decouple 'too_soon' from auto_renewals we should only call GetSoonestDate once

Additionally, we can fetch all needed rules at once and pass to subroutines as needed

Rather than check in the sub, let's not call _CanBookBeAutoRenewed if we aren't set for auto_renew

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 37 Nick Clemens 2021-11-23 12:00:22 UTC
Created attachment 127953 [details] [review]
Bug 29474: Small efficiency improvement

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>