Bug 29483 - AllowRenewalIfOtherItemsAvailable has poor performance for records with many items
Summary: AllowRenewalIfOtherItemsAvailable has poor performance for records with many ...
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on: 29474
Blocks:
  Show dependency treegraph
 
Reported: 2021-11-15 12:34 UTC by Nick Clemens
Modified: 2022-01-14 16:58 UTC (History)
2 users (show)

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


Attachments
Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron (3.64 KB, patch)
2021-11-15 14:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29483: Unit tests (11.13 KB, patch)
2021-11-19 00:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron (3.64 KB, patch)
2021-11-19 00:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29483: Unit tests (11.08 KB, patch)
2021-11-19 16:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron (3.64 KB, patch)
2021-11-19 16:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29483: Further improve performance of script (8.22 KB, patch)
2021-11-19 16:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29843: Unit tests (6.71 KB, patch)
2022-01-13 20:58 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29843: Add ->anonymize and ->filter_by_anonymizable to Koha::Old::Checkouts (2.67 KB, patch)
2022-01-13 20:58 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29843: Use in batch_anonymise.pl (1.66 KB, patch)
2022-01-13 20:58 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29843: Use in opac/opac-privacy.pl (1.85 KB, patch)
2022-01-13 20:58 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29843: Use in tools/cleanborrowers.pl (1.54 KB, patch)
2022-01-13 20:58 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29483: Unit tests (11.15 KB, patch)
2022-01-14 16:56 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron (3.70 KB, patch)
2022-01-14 16:56 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29483: Further improve performance of script (8.29 KB, patch)
2022-01-14 16:56 UTC, Andrew Fuerste-Henry
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-15 12:34:58 UTC
When a record has many items and holds and the check to see if other items available can take a long time to return
Comment 1 Nick Clemens 2021-11-15 14:15:14 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2021-11-15 14:15:50 UTC Comment hidden (obsolete)
Comment 3 Nick Clemens 2021-11-19 00:48:23 UTC Comment hidden (obsolete)
Comment 4 Nick Clemens 2021-11-19 00:48:26 UTC Comment hidden (obsolete)
Comment 5 Nick Clemens 2021-11-19 16:19:32 UTC
Created attachment 127883 [details] [review]
Bug 29483: Unit tests

This patch updates the AllowRenewalIfOtherItemsAvailable tests to
remove deletion of all data, and create specific circ rules for this
test. It adjust several other tests that were relying on the rules from
this test, so thy too create their opwn specific rules.

Additionally, we add tests to cover the case of mutliple items on the record,
and some items cannot fill some reserves.

What is uncovered here is that the same patron is checked twice, so two holds can
be filled, but they only satisfy a single patron
Comment 6 Nick Clemens 2021-11-19 16:19:35 UTC
Created attachment 127884 [details] [review]
Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron

ItemsAnyAvailableAndNotRestricted can take a long time and create nested loops.
We can check it once per patron, however, this requires us to flip the loops.
Since an item can only be used once, we now add a check to see if this item has already
been assigned to a borrower.

To test:
1 - Find or create a biblio with 100 items
2 - Place ten 'Next available' holds on a biblio
3 - Set preference 'AllowRenewalIfOtherItemsAvailable' to 'Allow'
    Set circ rules 'On shelf holds allowed' to 'If any unavailable'
4 - Checkout one of the items to a patron, backdated to be overdue
5 - Note a long loading time for the patron's checkouts
6 - Apply patch, restart_all
7 - Patron loads much faster
Comment 7 Nick Clemens 2021-11-19 16:19:38 UTC
Created attachment 127885 [details] [review]
Bug 29483: Further improve performance of script

This patch adds a few tests to cover more cases, and to highlight current functionality.

The script only allows renewal if all outstanding holds can be filled by available items.
This means we can return as soon as we have determined that not all holds can be filled.

I add FIXME and some explanatory comments - I will file a follow-up bug for those, but
I feel we can accept these improvements to the performance and deal with the issues
of how it 'should' work versus how it does work on another report.

To test:
1 - prove -v t/db_dependent/Circulation.t
Comment 8 Andrew Fuerste-Henry 2022-01-13 20:58:06 UTC
Created attachment 129450 [details] [review]
Bug 29843: Unit tests

This patch adds unit tests for the introduced methods.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 9 Andrew Fuerste-Henry 2022-01-13 20:58:09 UTC
Created attachment 129451 [details] [review]
Bug 29843: Add ->anonymize and ->filter_by_anonymizable to Koha::Old::Checkouts

This patch implements high-level methods for:

* Filtering a Koha::Old::Checkouts resultset so only anonymizable rows
  are left
* Anonymizing the rows in a resultset

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Old/Checkouts.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 10 Andrew Fuerste-Henry 2022-01-13 20:58:13 UTC
Created attachment 129452 [details] [review]
Bug 29843: Use in batch_anonymise.pl

This patch makes the batch_anonymise.pl cronjob script use the newly
introduced methods instead of the old ones.

To test:
1. Try the tool
=> SUCCESS: No behavior change
2. Sign off :-D

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 11 Andrew Fuerste-Henry 2022-01-13 20:58:17 UTC
Created attachment 129453 [details] [review]
Bug 29843: Use in opac/opac-privacy.pl

This patch makes the opac/opac-privacy.pl OPAC page use the new
anonymize method.

To test:
1. Have some checked-in materiales
2. Have OPACPrivacy enabled
3. Notice your checkouts history contains what you expect
4. Go to 'your privacy'
5. Click on 'Delete checkout history'
=> SUCCESS: It works, no crash.
6. Sign off :-D

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 12 Andrew Fuerste-Henry 2022-01-13 20:58:20 UTC
Created attachment 129454 [details] [review]
Bug 29843: Use in tools/cleanborrowers.pl

This patch makes tools/cleanborrowers.pl use the new methods.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 13 Andrew Fuerste-Henry 2022-01-14 16:56:45 UTC
Created attachment 129499 [details] [review]
Bug 29483: Unit tests

This patch updates the AllowRenewalIfOtherItemsAvailable tests to
remove deletion of all data, and create specific circ rules for this
test. It adjust several other tests that were relying on the rules from
this test, so thy too create their opwn specific rules.

Additionally, we add tests to cover the case of mutliple items on the record,
and some items cannot fill some reserves.

What is uncovered here is that the same patron is checked twice, so two holds can
be filled, but they only satisfy a single patron

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 14 Andrew Fuerste-Henry 2022-01-14 16:56:50 UTC
Created attachment 129500 [details] [review]
Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron

ItemsAnyAvailableAndNotRestricted can take a long time and create nested loops.
We can check it once per patron, however, this requires us to flip the loops.
Since an item can only be used once, we now add a check to see if this item has already
been assigned to a borrower.

To test:
1 - Find or create a biblio with 100 items
2 - Place ten 'Next available' holds on a biblio
3 - Set preference 'AllowRenewalIfOtherItemsAvailable' to 'Allow'
    Set circ rules 'On shelf holds allowed' to 'If any unavailable'
4 - Checkout one of the items to a patron, backdated to be overdue
5 - Note a long loading time for the patron's checkouts
6 - Apply patch, restart_all
7 - Patron loads much faster

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 15 Andrew Fuerste-Henry 2022-01-14 16:56:54 UTC
Created attachment 129501 [details] [review]
Bug 29483: Further improve performance of script

This patch adds a few tests to cover more cases, and to highlight current functionality.

The script only allows renewal if all outstanding holds can be filled by available items.
This means we can return as soon as we have determined that not all holds can be filled.

I add FIXME and some explanatory comments - I will file a follow-up bug for those, but
I feel we can accept these improvements to the performance and deal with the issues
of how it 'should' work versus how it does work on another report.

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

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 16 Andrew Fuerste-Henry 2022-01-14 16:58:50 UTC
Accidentally put a signoff for another bug here.

I've now *actually* signed off here.