Bug 29483 - AllowRenewalIfOtherItemsAvailable has poor performance for records with many items
Summary: AllowRenewalIfOtherItemsAvailable has poor performance for records with many ...
Status: RESOLVED FIXED
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: Martin Renvoize
URL:
Keywords:
Depends on: 29474
Blocks:
  Show dependency treegraph
 
Reported: 2021-11-15 12:34 UTC by Nick Clemens
Modified: 2022-06-16 14:45 UTC (History)
8 users (show)

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


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
Bug 29483: Unit tests (11.15 KB, patch)
2022-03-23 22:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron (3.65 KB, patch)
2022-03-23 22:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29483: Further improve performance of script (5.06 KB, patch)
2022-03-23 22:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29483: Further improve performance of script (8.19 KB, patch)
2022-03-24 10:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29483: Unit tests (11.21 KB, patch)
2022-04-19 08:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron (3.71 KB, patch)
2022-04-19 08:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29483: Further improve performance of script (8.26 KB, patch)
2022-04-19 08:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29483: [21.11.x] Check ItemsAnyAvailableAndNotRestricted once per patron (19.79 KB, patch)
2022-05-06 11:34 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-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.
Comment 17 Jonathan Druart 2022-02-18 09:24:01 UTC
I don't understand what happened here, this is in master already:

Bug 29843: Use in opac/opac-privacy.pl
Bug 29843: Use in tools/cleanborrowers.pl
Bug 29843: Use in batch_anonymise.pl
Bug 29843: Add ->anonymize and ->filter_by_anonymizable to Koha::Old::Checkouts
Bug 29843: Unit tests
Comment 18 Jonathan Druart 2022-02-18 09:28:08 UTC
(In reply to Jonathan Druart from comment #17)
> I don't understand what happened here, this is in master already:
> 
> Bug 29843: Use in opac/opac-privacy.pl
> Bug 29843: Use in tools/cleanborrowers.pl
> Bug 29843: Use in batch_anonymise.pl
> Bug 29843: Add ->anonymize and ->filter_by_anonymizable to
> Koha::Old::Checkouts
> Bug 29843: Unit tests

ok got it, all sorted out now.
Comment 19 Jonathan Druart 2022-02-18 09:31:15 UTC
And so finally it does not apply cleanly.

CONFLICT (content): Merge conflict in C4/Circulation.pm
Recorded preimage for 'C4/Circulation.pm'
error: Failed to merge in the changes.
Patch failed at 0001 Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron
Comment 20 Nick Clemens 2022-03-23 22:22:03 UTC
Created attachment 132094 [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 21 Nick Clemens 2022-03-23 22:22:08 UTC
Created attachment 132095 [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 22 Nick Clemens 2022-03-23 22:22:12 UTC
Created attachment 132096 [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 23 Jonathan Druart 2022-03-24 09:47:27 UTC
Did you benchmark? Do you have numbers?
Comment 24 Nick Clemens 2022-03-24 10:06:22 UTC
Created attachment 132113 [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 25 Nick Clemens 2022-03-24 11:32:42 UTC
(In reply to Jonathan Druart from comment #23)
> Did you benchmark? Do you have numbers?

Following the test plan I setup a patron with an item checked out from a bib with 100 books

Loading the checkouts directly:
http://localhost:8081/cgi-bin/koha/svc/checkouts?borrowernumber=5

Before patch: > 4 seconds (~4.4s)
After patch: < 1 second (~750ms)
Comment 26 Martin Renvoize 2022-04-19 08:14:50 UTC
Created attachment 133389 [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>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Martin Renvoize 2022-04-19 08:14:55 UTC
Created attachment 133390 [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>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Martin Renvoize 2022-04-19 08:14:59 UTC
Created attachment 133391 [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>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Martin Renvoize 2022-04-19 08:16:02 UTC
It took me a little while to wrap my head around the logic change, but all made sense in the end.

I concur, it does improve performance and I found no regressions.

QA scripts are happy and unit tests are all passing.

Passing QA
Comment 30 Fridolin Somers 2022-04-20 07:23:47 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 31 Nick Clemens 2022-05-06 11:34:58 UTC
Created attachment 134723 [details] [review]
Bug 29483: [21.11.x] 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>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

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>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

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>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Kyle M Hall 2022-05-06 14:55:12 UTC
Pushed to 21.11.x for 21.11.06
Comment 33 Victor Grousset/tuxayo 2022-06-16 03:24:00 UTC
Backported: Pushed to 21.05.x branch for 21.05.16
Comment 34 Victor Grousset/tuxayo 2022-06-16 14:45:07 UTC
↑↑↑↑↑ errata, wrong message

Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.