Bug 19599 - anonymise_issue_history can be very slow on large systems
Summary: anonymise_issue_history can be very slow on large systems
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Kyle M Hall
URL:
Whiteboard:
Keywords:
Depends on: 16966 19830
Blocks:
  Show dependency treegraph
 
Reported: 2017-11-09 11:54 UTC by Nick Clemens
Modified: 2018-01-16 13:30 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19599: Execute 1 query per patron (2.21 KB, patch)
2017-11-09 13:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19599: Simplify the query (1.39 KB, patch)
2017-11-09 13:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19599: Execute 1 query per patron (2.25 KB, patch)
2017-12-13 15:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19599: Speed anonymise_issue_history up (1.77 KB, patch)
2017-12-18 17:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19599: Speed anonymise_issue_history up (1.82 KB, patch)
2017-12-21 11:23 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2017-11-09 11:54:50 UTC
When running anonymise_issue_history we now call it on a set of patrons from search_patrons_to_anonymise, however, this generates a slow SQL query that can take hours to run.

UPDATE `old_issues` SET `old_issues`.`borrowernumber` = '9807' WHERE (  `issue_id` IN ( 
    SELECT * FROM ( 
        SELECT `old_issues`.`issue_id` FROM (
            SELECT `me`.`borrowernumber`, `me`.`cardnumber`, `me`.`surname`, `me`.`firstname`, `me`.`title`, `me`.`othernames`, `me`.`initials`, `me`.`streetnumber`, `me`.`streettype`, `me`.`address`, `me`.`address2`, `me`.`city`, `me`.`state`, `me`.`zipcode`, `me`.`country`, `me`.`email`, `me`.`phone`, `me`.`mobile`, `me`.`fax`, `me`.`emailpro`, `me`.`phonepro`, `me`.`B_streetnumber`, `me`.`B_streettype`, `me`.`B_address`, `me`.`B_address2`, `me`.`B_city`, `me`.`B_state`, `me`.`B_zipcode`, `me`.`B_country`, `me`.`B_email`, `me`.`B_phone`, `me`.`dateofbirth`, `me`.`branchcode`, `me`.`categorycode`, `me`.`dateenrolled`, `me`.`dateexpiry`, `me`.`gonenoaddress`, `me`.`lost`, `me`.`debarred`, `me`.`debarredcomment`, `me`.`contactname`, `me`.`contactfirstname`, `me`.`contacttitle`, `me`.`guarantorid`, `me`.`borrowernotes`, `me`.`relationship`, `me`.`sex`, `me`.`password`, `me`.`flags`, `me`.`userid`, `me`.`opacnote`, `me`.`contactnote`, `me`.`sort1`, `me`.`sort2`, `me`.`altcontactfirstname`, `me`.`altcontactsurname`, `me`.`altcontactaddress1`, `me`.`altcontactaddress2`, `me`.`altcontactaddress3`, `me`.`altcontactstate`, `me`.`altcontactzipcode`, `me`.`altcontactcountry`, `me`.`altcontactphone`, `me`.`smsalertnumber`, `me`.`sms_provider_id`, `me`.`privacy`, `me`.`privacy_guarantor_checkouts`, `me`.`checkprevcheckout`, `me`.`updated_on`, `me`.`lastseen`, `me`.`lang`, `me`.`login_attempts`, `me`.`overdrive_auth_token` 
            FROM `borrowers` `me` 
            LEFT JOIN `old_issues` `old_issues` ON `old_issues`.`borrowernumber` = `me`.`borrowernumber` 
            WHERE ( ( `old_issues`.`borrowernumber` IS NOT NULL AND `privacy` <> '0' AND `returndate` < '2017-05-13 00:00:00' ) ) 
            GROUP BY `borrowernumber`) `me`  
         JOIN `old_issues` `old_issues` ON `old_issues`.`borrowernumber` = `me`.`borrowernumber` 
         WHERE ( `returndate` < '2017-05-13 00:00:00' ) ) `_forced_double_subquery` )  
     )

I think anonymise_issue_history should be a standalone function taking the same params as search_patrons_to_anonymise to eliminate nesting of queries
Comment 1 Jonathan Druart 2017-11-09 13:55:29 UTC
Created attachment 69052 [details] [review]
Bug 19599: Execute 1 query per patron
Comment 2 Jonathan Druart 2017-11-09 13:56:37 UTC
Created attachment 69054 [details] [review]
Bug 19599: Simplify the query
Comment 3 Jonathan Druart 2017-11-09 13:58:13 UTC
Nick, can you compare these 2 approaches please?
They are not dependent on each others.
Comment 4 Nick Clemens 2017-12-13 15:45:25 UTC
Created attachment 69778 [details] [review]
Bug 19599: Execute 1 query per patron

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 5 Nick Clemens 2017-12-13 15:47:25 UTC
(In reply to Jonathan Druart from comment #3)
> Nick, can you compare these 2 approaches please?
> They are not dependent on each others.

The first approach is much faster on a large dataset. The second can generate a very long query, I think one per patron works best.

Signed off and obsoleted second patch
Comment 6 Julian Maurice 2017-12-15 14:58:16 UTC
koha-qa complains:

 FAIL   Koha/Patron.pm
   FAIL   pod coverage
                POD is missing for old_checkouts
Comment 7 Jonathan Druart 2017-12-18 17:36:15 UTC
Created attachment 69860 [details] [review]
Bug 19599: Speed anonymise_issue_history up

The query generated by anonymise_issue_history is unecessary complex and
can be very slow on large DB.
With this patch we will execute one query per patron instead.

See patch "Bug 19599: Simplify the query" on the bug report for an
alternative.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Jonathan Druart 2017-12-18 17:36:54 UTC
I have moved the code creating Koha::Patron->old_checkouts to its own bug report (bug 19830).
Comment 9 Kyle M Hall 2017-12-21 11:23:03 UTC
Created attachment 69999 [details] [review]
Bug 19599: Speed anonymise_issue_history up

The query generated by anonymise_issue_history is unecessary complex and
can be very slow on large DB.
With this patch we will execute one query per patron instead.

See patch "Bug 19599: Simplify the query" on the bug report for an
alternative.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Jonathan Druart 2017-12-21 16:24:05 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 11 Nick Clemens 2018-01-15 13:20:38 UTC
Awesome work all! Pushed to stable for 17.11.02
Comment 12 Fridolin SOMERS 2018-01-15 15:50:33 UTC
Depends on Bug 19830 that is not in 17.05.x
Comment 13 Fridolin SOMERS 2018-01-16 13:28:13 UTC
(In reply to Fridolin SOMERS from comment #12)
> Depends on Bug 19830 that is not in 17.05.x

I've changed my mind and push to 17.05.x for v17.05.08
Comment 14 Fridolin SOMERS 2018-01-16 13:30:24 UTC
I think not needed for 16.11.x because does not contain Bug 16966