Bug 15107 - Batch Patron Deletion (Trash) Performance Improvement
Summary: Batch Patron Deletion (Trash) Performance Improvement
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: cnorthcott.work
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 7923
  Show dependency treegraph
 
Reported: 2015-11-02 00:57 UTC by cnorthcott.work
Modified: 2016-08-24 10:07 UTC (History)
6 users (show)

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


Attachments
Bug 15107: Batch Patron Deletion (Trash) Performance Improvement (4.59 KB, patch)
2015-11-05 01:36 UTC, cnorthcott.work
Details | Diff | Splinter Review
Patron data to import for testing (222.49 KB, text/plain)
2015-11-05 01:37 UTC, cnorthcott.work
Details

Note You need to log in before you can comment on or make changes to this bug.
Description cnorthcott.work 2015-11-02 00:57:54 UTC
Batch Patron Deletion (Trash) has a fairly long run/load time compared to Batch Patron Deletion (Permanent). I have attempted to improve the runtime of the Batch Patron Deletion (Trash) tool and my enhancement reduces the time to modify a batch of patrons by an average of 78.21%

I discovered the extended runtime was mostly due to the overhead DBIx::Class::ResultSet which I switched to basic SQL calls to the $dbh which reduced the trashing of 1000 patrons from 24.84 seconds to 5.41 seconds, saving 19.43 seconds of resultset calls.
Comment 1 cnorthcott.work 2015-11-05 01:36:24 UTC
Created attachment 44447 [details] [review]
Bug 15107: Batch Patron Deletion (Trash) Performance Improvement

Batch Patron Modification has a fairly long run/load time. I have attempted
to improve the runtime of the Batch Patron Modification tool by switching
out more expensive resultset calls for faster more direct SQL queries.

Test Plan:
1) Prepare NTYprof (http://wiki.koha-community.org/wiki/Profiling_with_Devel::NYTProf)
   or procure a stopwatch (stopwatch will be simpler but less accurate).
2) Ensure you have a decent number of patrons in your Koha system (>500) if you are
   lacking patrons to modify in your database you can import 1000 using the
   PatronDataCSV.csv attachement and the Import Patrons tool before moving to
   step 3.
4) Navigate to Home > Tools > Batch Patrons Deletion in the Koha Intranet
5) Check the box beside the "Verify you want to delete patrons" box
6) Under "who have not borrowed since" enter todays date
7) You should get a message saying:
   "1000 patrons will be deleted
    0 checkout history will be anonymized"
    If not please go back to step 2 and import patrons using the PatronDataCSV.csv
8) Select the radio button option beside "Move these patrons to the trash"
9) If you are using a stopwatch, prepare your stopwatch so that you will
   start counting seconds from the point you click the button in the next
   step.
10) Click the "Finsh" button (simultaneously start your stopwatch if using one)
11) When the page appears showing completion of the change, stop your stopwatch
    and check the time or navigate to the folder you have set NYTProf to output.
12) Record the runtime. This is the pre-optimisation time.
13) Apply this patch.
14) Repeat steps 2-12 of this testplan with the patch applied. This will yield
    the post-optimisation time.
15) Compare the pre-optimisation time and post optimisation time. The second
    post-optimisation time should be faster.
Comment 2 cnorthcott.work 2015-11-05 01:37:19 UTC
Created attachment 44448 [details]
Patron data to import for testing
Comment 3 Martin Renvoize 2015-11-16 11:49:20 UTC
Comment on attachment 44447 [details] [review]
Bug 15107: Batch Patron Deletion (Trash) Performance Improvement

Review of attachment 44447 [details] [review]:
-----------------------------------------------------------------

Great to see someone working on performance!

I realise that dropping DBIC, especially in background scripts where a startup cost is clearly a bad thing, does lead to a performance increase but I am a little apprehensive to do an about turn on dbic's use as it's only jsut picking up momentum in the project as a whole.  i wonder if there is a dbic friendly way to improve performance rather than just ditching it?

::: C4/Members.pm
@@ +1772,3 @@
>  =cut
>  
>  sub MoveMemberToDeleted {

Seems you've highlighted a bad API choice here (not your fault). This sub does not 'Move' at all.. it 'Copies'!

I would prefer to see it actually do a 'Move', i.e. do the insert from select, followed by a delete wrapped in a transaction.

Would you be willing to look into this as part of this piece?

@@ +1773,5 @@
>  
>  sub MoveMemberToDeleted {
> +    my ($borrowernumber) = shift or return;
> +    my $dbh = C4::Context->dbh;
> +    my $borrower_columns = q{

I'm not especially happy with adding a list of fields here, it means increased maintenance cost going forward as would require this script to be altered upon any db change on this table.
Comment 4 Martin Renvoize 2015-11-16 11:54:33 UTC
Comment on attachment 44447 [details] [review]
Bug 15107: Batch Patron Deletion (Trash) Performance Improvement

Review of attachment 44447 [details] [review]:
-----------------------------------------------------------------

::: C4/Members.pm
@@ +1772,4 @@
>  =cut
>  
>  sub MoveMemberToDeleted {
> +    my ($borrowernumber) = shift or return;

I missed this one the first time around.. Have you actually checked that all calls to MoveMoverToDeleted are actually passing a borrowernumber int as a posed to a member hashref?
Comment 5 Martin Renvoize 2015-11-16 12:14:37 UTC
Hi Catherine, 

What sort of appetite do you have for expanding the scope of this as hinted above?

I feel koha is in a somewhat transitional stage at the moment regarding performance/coding standards.. I certainly don't wan't to put you off this important work but would like to see some alternatives that actually attempt to fix up the api's rather than make 'fixes' on top of old design decisions.

For instance, the 'Move' routine not being a Move at all, but rather a Copy.. this is leading to bugs elsewhere. Also, the routine not being clear as to whether it requires a borrowernumber or a member hashref is likely to lead to bugs.  I also think there could be an improvement to allow for passing lists of borrowernumber as a posed to single borrowers which could lead to performance gains too.  This is all skirting the issue of data integrity and the lack of good db constraints meaning that objects could well become orphans after running this routine :(

Any thoughts?
Comment 6 Martin Renvoize 2015-11-16 12:34:38 UTC
I seem to be on a comment mission today, sorry ;)

The routine you've optimised is called in three places:

misc/cronjobs/delete_patrons.pl
tools/cleanborrowers.pl
members/deletemem.pl

Could you verify there are no adverse effects resulting from these changes for those scripts.. and perhaps profile profiles for running with and without plack as this will effect the performance metrics greatly.

Pretty please :)
Comment 7 Kyle M Hall 2015-11-16 13:56:45 UTC
I agree with Martin. I would prefer we don't move backwards. If we can find a way to improve the speed while retaining DBIC that would be best.

Maybe we could cache the result set as a package variable ( use vars )? Or maybe re-engineer the subroutine to accept a list of patrons to copy?

I also think Martin is correct in that it would be great if you would be willing to fix the semantic issues with the code.
Comment 8 Mark Tompsett 2016-02-23 02:31:47 UTC
Comment on attachment 44447 [details] [review]
Bug 15107: Batch Patron Deletion (Trash) Performance Improvement

Review of attachment 44447 [details] [review]:
-----------------------------------------------------------------

While DBIx assures DB agnosticism, all this code is really ANSI stuff. I'm personally torn between speed and eventual consistency (keeping DBIx across the board). As for Martin's comment, perhaps DBIx::CopyRecord? I have no idea.

::: C4/Members.pm
@@ +1788,5 @@
> +        cardnumber, branchcode, categorycode, sort1, sort2, 
> +        dateenrolled, dateexpiry, opacnote, borrowernotes, userid,
> +        password, flags, gonenoaddress, lost, debarred, 
> +        debarredcomment, smsalertnumber, privacy
> +    };

This would be better built with C4::Member::columns()
Comment 9 Martin Renvoize 2016-05-26 21:46:40 UTC
Shame, this one seems to have lost its head of steam :-(
Comment 10 Jonathan Druart 2016-08-24 10:07:03 UTC
We are moving our code to the Koha namespace using DBIx::Class objects, I am not sure it's a good idea to go back and use DBI directly.

See bug 16891 and bug 16907.