Bug 30004

Summary: Prevent TooMany from executing too many SQL queries
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: Architecture, internals, and plumbingAssignee: Julian Maurice <julian.maurice>
Status: Pushed to stable --- QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: david, fridolin.somers, jonathan.druart+koha, kyle
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30167
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.05
Bug Depends on:    
Bug Blocks: 7923    
Attachments: Bug 30004: Prevent TooMany from executing too many SQL queries
Bug 30004: Prevent TooMany from executing too many SQL queries
Bug 30004: Prevent TooMany from executing too many SQL queries

Description Julian Maurice 2022-02-02 14:10:25 UTC
If a maximum number of checkouts allowed is defined in circulation rules, C4::Circulation::TooMany will loop over all patron's checkouts.  When a patron has several hundreds of checkouts, it can really slow down the checkout process by several seconds (or even tens of seconds)
Comment 1 Julian Maurice 2022-02-02 14:17:02 UTC
Created attachment 130106 [details] [review]
Bug 30004: Prevent TooMany from executing too many SQL queries

If a maximum number of checkouts allowed is defined in circulation
rules, C4::Circulation::TooMany will loop over all patron's checkouts.
When a patron has several hundreds of checkouts, it can really slow down
the checkout process by several seconds (or even tens of seconds)

This patch does two things:
- Always prefetch item data so that `$c->item` does not execute an
  additional SQL query at every iteration of the loop. Item data is
  always needed at the first line of the loop, so there is really no
  downside for doing this.
- Build the `@types` array only once, out of the checkouts loop. Since
  it does not depend at all on patron's checkouts data, it does not make
  sense to build it inside the loop.

Test plan:
1. Before applying the patch, create a patron with a lot of checkouts.
   I tested with 1000 checkouts, but the slowness should be noticeable
   with less.
2. Make sure you have a circulation rule (one that apply to your patron
   and the item(s) you will check out for testing) with a maximum number
   of checkouts allowed
3. Check out an item for the patron with a lot of checkouts. Measure the
   time it takes.
4. Apply the patch
5. Check out another item (or check in and then check out the same item
   used in step 3). Measure the time it takes and compare it to step 3.
   It should be faster now.
6. Run `prove t/db_dependent/Circulation/TooMany.t`
Comment 2 Julian Maurice 2022-02-02 14:18:44 UTC
This problem affects at least versions 20.11, 21.05, 21.11, and master
Comment 3 Fridolin Somers 2022-02-02 19:50:24 UTC
Performance issue so i change from enhancement to major bug

Good catch Julian ;)
Comment 4 David Nind 2022-02-05 12:48:16 UTC
Is there an easy way to generate 1,000 checkouts for a patron?
Comment 5 Julian Maurice 2022-02-18 14:56:23 UTC
(In reply to David Nind from comment #4)
> Is there an easy way to generate 1,000 checkouts for a patron?

You can create multiple items at once by clicking on "Add multiple copies" button when creating an item.
For checkouts, there is the "batch check out" feature.
(Maybe do not create 1000 checkouts at once as it can take a lot of time, but by batches of 100 it should be doable)
Comment 6 Nick Clemens 2022-02-24 16:12:14 UTC
Created attachment 131066 [details] [review]
Bug 30004: Prevent TooMany from executing too many SQL queries

If a maximum number of checkouts allowed is defined in circulation
rules, C4::Circulation::TooMany will loop over all patron's checkouts.
When a patron has several hundreds of checkouts, it can really slow down
the checkout process by several seconds (or even tens of seconds)

This patch does two things:
- Always prefetch item data so that `$c->item` does not execute an
  additional SQL query at every iteration of the loop. Item data is
  always needed at the first line of the loop, so there is really no
  downside for doing this.
- Build the `@types` array only once, out of the checkouts loop. Since
  it does not depend at all on patron's checkouts data, it does not make
  sense to build it inside the loop.

Test plan:
1. Before applying the patch, create a patron with a lot of checkouts.
   I tested with 1000 checkouts, but the slowness should be noticeable
   with less.
2. Make sure you have a circulation rule (one that apply to your patron
   and the item(s) you will check out for testing) with a maximum number
   of checkouts allowed
3. Check out an item for the patron with a lot of checkouts. Measure the
   time it takes.
4. Apply the patch
5. Check out another item (or check in and then check out the same item
   used in step 3). Measure the time it takes and compare it to step 3.
   It should be faster now.
6. Run `prove t/db_dependent/Circulation/TooMany.t`

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 7 Jonathan Druart 2022-03-21 14:26:15 UTC
Compared a call to CanBookBeIssued in a pl script, with 960 checkouts:

Without patch: 4.3s
With patch   : 1.7s
Comment 8 Jonathan Druart 2022-03-21 14:26:56 UTC
Created attachment 131972 [details] [review]
Bug 30004: Prevent TooMany from executing too many SQL queries

If a maximum number of checkouts allowed is defined in circulation
rules, C4::Circulation::TooMany will loop over all patron's checkouts.
When a patron has several hundreds of checkouts, it can really slow down
the checkout process by several seconds (or even tens of seconds)

This patch does two things:
- Always prefetch item data so that `$c->item` does not execute an
  additional SQL query at every iteration of the loop. Item data is
  always needed at the first line of the loop, so there is really no
  downside for doing this.
- Build the `@types` array only once, out of the checkouts loop. Since
  it does not depend at all on patron's checkouts data, it does not make
  sense to build it inside the loop.

Test plan:
1. Before applying the patch, create a patron with a lot of checkouts.
   I tested with 1000 checkouts, but the slowness should be noticeable
   with less.
2. Make sure you have a circulation rule (one that apply to your patron
   and the item(s) you will check out for testing) with a maximum number
   of checkouts allowed
3. Check out an item for the patron with a lot of checkouts. Measure the
   time it takes.
4. Apply the patch
5. Check out another item (or check in and then check out the same item
   used in step 3). Measure the time it takes and compare it to step 3.
   It should be faster now.
6. Run `prove t/db_dependent/Circulation/TooMany.t`

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Jonathan Druart 2022-03-21 14:28:40 UTC
(In reply to David Nind from comment #4)
> Is there an easy way to generate 1,000 checkouts for a patron?

The following code will checkout all the items from the DB to the patron with borrowernumber=51

use Modern::Perl;
use C4::Circulation qw( AddIssue );
use Koha::Items;
use Koha::Patrons;
use t::lib::Mocks;
my $patron = Koha::Patrons->find(51);
t::lib::Mocks::mock_userenv({patron => $patron});
my $items = Koha::Items->search;
my $patron_unblessed = $patron->unblessed;
while ( my $i = $items->next ) {
    AddIssue($patron_unblessed, $i->barcode);
}
Comment 10 Fridolin Somers 2022-03-22 20:19:05 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 11 Kyle M Hall 2022-03-25 13:43:36 UTC
Pushed to 21.11.x for 21.11.05