Bug 29483

Summary: AllowRenewalIfOtherItemsAvailable has poor performance for records with many items
Product: Koha Reporter: Nick Clemens <nick>
Component: Architecture, internals, and plumbingAssignee: Nick Clemens <nick>
Status: Pushed to stable --- QA Contact: Martin Renvoize <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: andrew, dcook, fridolin.somers, jonathan.druart+koha, kyle, martin.renvoize, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29531
Change sponsored?: --- Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.06
Bug Depends on: 29474    
Bug Blocks:    
Attachments: Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron
Bug 29483: Unit tests
Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron
Bug 29483: Unit tests
Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron
Bug 29483: Further improve performance of script
Bug 29843: Unit tests
Bug 29843: Add ->anonymize and ->filter_by_anonymizable to Koha::Old::Checkouts
Bug 29843: Use in batch_anonymise.pl
Bug 29843: Use in opac/opac-privacy.pl
Bug 29843: Use in tools/cleanborrowers.pl
Bug 29483: Unit tests
Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron
Bug 29483: Further improve performance of script
Bug 29483: Unit tests
Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron
Bug 29483: Further improve performance of script
Bug 29483: Further improve performance of script
Bug 29483: Unit tests
Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron
Bug 29483: Further improve performance of script
Bug 29483: [21.11.x] Check ItemsAnyAvailableAndNotRestricted once per patron

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