Bug 30085 - Improve performance of CanItemBeReserved
Summary: Improve performance of CanItemBeReserved
Status: REOPENED
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: rel_21_11_candidate
Depends on:
Blocks: 7923
  Show dependency treegraph
 
Reported: 2022-02-11 19:21 UTC by Nick Clemens
Modified: 2022-07-22 19:30 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00


Attachments
Bug 30085: Reduce scope of holds count / today holds count (2.84 KB, patch)
2022-02-11 20:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30085: Don't fetch ReservesControlBranch twice (3.40 KB, patch)
2022-02-11 20:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30085: Consolidate querycoutn code and only check if needed (4.05 KB, patch)
2022-02-11 20:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30085: Move IndependentBranches check sooner (1.89 KB, patch)
2022-02-11 20:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30085: Reduce scope of holds count / today holds count (2.90 KB, patch)
2022-02-18 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30085: Don't fetch ReservesControlBranch twice (3.46 KB, patch)
2022-02-18 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30085: Consolidate querycoutn code and only check if needed (4.12 KB, patch)
2022-02-18 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30085: Move IndependentBranches check sooner (1.95 KB, patch)
2022-02-18 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30085: Reduce scope of holds count / today holds count (2.98 KB, patch)
2022-02-18 14:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30085: Don't fetch ReservesControlBranch twice (3.54 KB, patch)
2022-02-18 14:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30085: Consolidate querycoutn code and only check if needed (4.19 KB, patch)
2022-02-18 14:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30085: Move IndependentBranches check sooner (2.03 KB, patch)
2022-02-18 14:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30085: [21.11.x] Reduce scope of holds count / today holds count (10.91 KB, patch)
2022-07-22 19:29 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 2022-02-11 19:21:15 UTC

    
Comment 1 Nick Clemens 2022-02-11 20:00:14 UTC
Created attachment 130525 [details] [review]
Bug 30085: Reduce scope of holds count / today holds count

We retrieve two counts that are only needed if rules for hold limits are defined.
The DB counts should only be fetched once the rules are confirmed to exist

Further improvement would be possiblke by allowing them to be passed in (or cached?)
from CanBookBeReserved as they rely only on patron/biblionumber and not item specific information
Comment 2 Nick Clemens 2022-02-11 20:00:17 UTC
Created attachment 130526 [details] [review]
Bug 30085: Don't fetch ReservesControlBranch twice

We essentially copy the code from GetReservesControlBranch here, because we
also calculate 'branchfield'

Setting it to "" vs undef makes no difference in this code, so lets not fetch this
again later.

Rename the variable to make it clearer where the branchcode came from
Comment 3 Nick Clemens 2022-02-11 20:00:21 UTC
Created attachment 130527 [details] [review]
Bug 30085: Consolidate querycoutn code and only check if needed

Similar to first patch, move a count only conditionally used into
the conditional

This could be updated to use DBIC, but the itemtype conditionals
add complexity
Comment 4 Nick Clemens 2022-02-11 20:00:24 UTC
Created attachment 130528 [details] [review]
Bug 30085: Move IndependentBranches check sooner

This doesn't rely on the other statuses, and requires only cached
preference check, so lets allow the possibiliy of an early exit
Comment 5 Martin Renvoize 2022-02-18 12:34:06 UTC
Created attachment 130829 [details] [review]
Bug 30085: Reduce scope of holds count / today holds count

We retrieve two counts that are only needed if rules for hold limits are defined.
The DB counts should only be fetched once the rules are confirmed to exist

Further improvement would be possiblke by allowing them to be passed in (or cached?)
from CanBookBeReserved as they rely only on patron/biblionumber and not item specific information

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2022-02-18 12:34:10 UTC
Created attachment 130830 [details] [review]
Bug 30085: Don't fetch ReservesControlBranch twice

We essentially copy the code from GetReservesControlBranch here, because we
also calculate 'branchfield'

Setting it to "" vs undef makes no difference in this code, so lets not fetch this
again later.

Rename the variable to make it clearer where the branchcode came from

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2022-02-18 12:34:14 UTC
Created attachment 130831 [details] [review]
Bug 30085: Consolidate querycoutn code and only check if needed

Similar to first patch, move a count only conditionally used into
the conditional

This could be updated to use DBIC, but the itemtype conditionals
add complexity

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2022-02-18 12:34:18 UTC
Created attachment 130832 [details] [review]
Bug 30085: Move IndependentBranches check sooner

This doesn't rely on the other statuses, and requires only cached
preference check, so lets allow the possibiliy of an early exit

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2022-02-18 12:35:07 UTC
Code is clearer to read and all the unit tests that call this function are still passing.

I've not actually tested performance directly, but the changes all make sense to me as improvements.

Signing off.
Comment 10 Jonathan Druart 2022-02-18 14:32:01 UTC
Created attachment 130856 [details] [review]
Bug 30085: Reduce scope of holds count / today holds count

We retrieve two counts that are only needed if rules for hold limits are defined.
The DB counts should only be fetched once the rules are confirmed to exist

Further improvement would be possiblke by allowing them to be passed in (or cached?)
from CanBookBeReserved as they rely only on patron/biblionumber and not item specific information

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Jonathan Druart 2022-02-18 14:32:07 UTC
Created attachment 130857 [details] [review]
Bug 30085: Don't fetch ReservesControlBranch twice

We essentially copy the code from GetReservesControlBranch here, because we
also calculate 'branchfield'

Setting it to "" vs undef makes no difference in this code, so lets not fetch this
again later.

Rename the variable to make it clearer where the branchcode came from

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Jonathan Druart 2022-02-18 14:32:15 UTC
Created attachment 130858 [details] [review]
Bug 30085: Consolidate querycoutn code and only check if needed

Similar to first patch, move a count only conditionally used into
the conditional

This could be updated to use DBIC, but the itemtype conditionals
add complexity

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Jonathan Druart 2022-02-18 14:32:20 UTC
Created attachment 130859 [details] [review]
Bug 30085: Move IndependentBranches check sooner

This doesn't rely on the other statuses, and requires only cached
preference check, so lets allow the possibiliy of an early exit

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Fridolin Somers 2022-02-23 08:28:19 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 15 Nick Clemens 2022-03-14 17:34:02 UTC
Consider for backport?
Comment 16 Kyle M Hall 2022-04-07 13:55:20 UTC
(In reply to Nick Clemens from comment #15)
> Consider for backport?

Sure! Can you rebase against 21.11.x?
Comment 17 Nick Clemens 2022-07-22 19:29:41 UTC
Created attachment 138058 [details] [review]
Bug 30085: [21.11.x] Reduce scope of holds count / today holds count

We retrieve two counts that are only needed if rules for hold limits are defined.
The DB counts should only be fetched once the rules are confirmed to exist

Further improvement would be possiblke by allowing them to be passed in (or cached?)
from CanBookBeReserved as they rely only on patron/biblionumber and not item specific information

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 30085: Don't fetch ReservesControlBranch twice

We essentially copy the code from GetReservesControlBranch here, because we
also calculate 'branchfield'

Setting it to "" vs undef makes no difference in this code, so lets not fetch this
again later.

Rename the variable to make it clearer where the branchcode came from

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 30085: Consolidate querycoutn code and only check if needed

Similar to first patch, move a count only conditionally used into
the conditional

This could be updated to use DBIC, but the itemtype conditionals
add complexity

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 30085: Move IndependentBranches check sooner

This doesn't rely on the other statuses, and requires only cached
preference check, so lets allow the possibiliy of an early exit

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>