Since introduction of http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529#add_comment "Overdue notices respect holidays" feature, there is a big problem of performance running overdue_notices.pl For one of our client, where it used to take less than one hour, it now takes more than 7 hours. And this should be rune every day... This is due to date criteria that were removed from SQL queries, so all current issues are parsed 3 times (once for each level of overdue) rather than just overdue issues concerning one given level. Dealing with holidays should be done before quering issues, so that date criteria could be used.
Could you give information about the holidays? How many entries do you have in the special_holidays and repeatable_holidays tables?
We have a system that's really struggling with the same issue. 262 rows in special_holidays 9 rows in repeatable_holidays We also have a system where the preferences are set to ignore the holidays and this runs much more quickly (though still isn't super quick). It has 921 special_holidays and 307 repeatable_holidays. Hope that helps a little. I'm happy to run a profile at the next run of the scripts unless someone else beats me to it.
Created attachment 45524 [details] [review] Bug 15240: Do not process issues with a date due later than today There is no need to do this job in Perl, MySQL could do it instead. The idea is to only retrieve the issues info which could be overdued.
Sophie, Martin, If you are not using OverdueNoticeCalendar, the number of holidays should not change anything for you actually. It would be great if you could have a look at this patch and compare the execution time, with and without. Do not do it in production, I have not tested this patch enough to be 100% confident :)
That's what I meant by 'system where the preferences are set to ignore the holidays' in comment 2, I just couldn't remember what the pref was called ;)
So it could be related to the number of issues in the DB and this patch could do the job :)
Created attachment 45554 [details] [review] Bug 15344: Remove some other calls of GetMemberDetails from pl scripts Same as previously. For these files it's a bit less obvious. To make sure these changes won't introduce any regression, check that the variable returned by GetMember is never used to get something else than a borrower fields. The 'flags' should not be get neither. For opac-user.tt it's different, other keys are got but there are defined in the pl script. On the way: - 'showname' is removed (never used) - fix scope var issue in opac-user.tt (BORROWER_INF.OPACPatronDetails vs OPACPatronDetails)
Comment on attachment 45524 [details] [review] Bug 15240: Do not process issues with a date due later than today Review of attachment 45524 [details] [review]: ----------------------------------------------------------------- This eyeballs well. ::: misc/cronjobs/overdue_notices.pl @@ +454,4 @@ > AND b.branchcode = items.homebranch > AND biblio.biblionumber = biblioitems.biblionumber > AND issues.borrowernumber = ? > + AND TO_DAYS($date)-TO_DAYS(issues.date_due) >= 0 This makes sure all the filtering based on date is done by MySQL. @@ -500,5 @@ > # <date> <itemcount> <firstname> <lastname> <address1> <address2> <address3> <city> <postcode> <country> > > - my $borrower_sql = <<'END_SQL'; > -SELECT issues.borrowernumber, firstname, surname, address, address2, city, zipcode, country, email, emailpro, B_email, smsalertnumber, phone, cardnumber, > -TO_DAYS(?)-TO_DAYS(date_due) as difference, date_due 'difference' field removed because it is moved to part of the WHERE condition. @@ +505,4 @@ > FROM issues,borrowers,categories > WHERE issues.borrowernumber=borrowers.borrowernumber > AND borrowers.categorycode=categories.categorycode > +AND TO_DAYS($date)-TO_DAYS(issues.date_due) >= 0 This was the difference field, which strangely was never really used, and by limiting the SQL query here (make SQL do the work), this speeds the script up. Also, the ? in the TO_DAYS was changed to a hard coded $date. Gut feeling: meh, not a big deal. @@ -508,3 @@ > END_SQL > my @borrower_parameters; > - push @borrower_parameters, $date_to_run->datetime(); Removed, because the ? was changed to a hard coded $date. @@ -526,5 @@ > my $borrowernumber; > while ( my $data = $sth->fetchrow_hashref ) { > > - next unless ( DateTime->compare( $date_to_run, dt_from_string($data->{date_due})) ) == 1; > - Perl filtering on a larger data set is slower. Hence the removal of this, when replaced by the SQL grunt work. @@ -616,5 @@ > my $j = 0; > my $exceededPrintNoticesMaxLines = 0; > while ( my $item_info = $sth2->fetchrow_hashref() ) { > - next unless ( DateTime->compare( $date_to_run, dt_from_string($item_info->{date_due})) ) == 1; > - Perl filtering on a larger data set is slower. Hence the removal of this, when replaced by the SQL grunt work.
It's nearing midnight, and there is no express test plan to follow or steps to take to generate the data necessary to sign off. :(
Test plan: Launch the overdue_notices.pl on a large DB and compare the execution times with and without patch applied. No behavior difference should be noticed.
Jonathan, I'm checking with ByWater to see if our rather large test system (running 3.22.1) database could somehow be used to help test the patch as you suggested you needed a large db to do it. I'll let you know what I find out.
Created attachment 46406 [details] [review] Bug 15240: Do not process issues with a date due later than today There is no need to do this job in Perl, MySQL could do it instead. The idea is to only retrieve the issues info which could be overdued. To test: 1/ Run the script 2/ Apply the patch 3/ Run the script again, notice the exact same results but execution time is faster Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Hi Chris, Thanks for testing! Did you compare the execution times before and after the patches?
Pre patch: time misc/cronjobs/overdue_notices.pl -t -itemscontent title,author,itemcallnumber,barcode,replacementprice real 2m9.252s user 2m5.492s sys 0m0.108s Post Patch: time misc/cronjobs/overdue_notices.pl -t -itemscontent title,author,itemcallnumber,barcode,replacementprice real 1m19.965s user 1m12.437s sys 0m0.192s mysql> select COUNT(*) from issues where date_due > now(); +----------+ | COUNT(*) | +----------+ | 480 | +----------+ 1 row in set (0.01 sec) mysql> select COUNT(*) from issues where date_due <= now(); +----------+ | COUNT(*) | +----------+ | 65218 | +----------+ 1 row in set (0.01 sec) I found it quite striking how much of a time difference there was even though the number of checkouts not due yet is quite low so I ran the tests again with very similar results!
Kyle, Heather, does this changed script yield the same results? It passes my QA, as it just moves the date comparison into MySQL and preserves the --date functionality.
(In reply to Jesse Weaver from comment #15) > Kyle, Heather, does this changed script yield the same results? It passes my > QA, as it just moves the date comparison into MySQL and preserves the --date > functionality. I did not think to check that, I was focused on performance testing.
For us it's terribly important that the functionality remains unchanged and it's not quickly testable in Koha. Please don't rush this.
(In reply to Katrin Fischer from comment #17) > For us it's terribly important that the functionality remains unchanged and > it's not quickly testable in Koha. Please don't rush this. Katrin, you are more than welcome to take point on QA for this bug. I wasn't QA'ing it, I was only doing performance testing.
I hope to get around to it soon - but the more testing the better. Overdues are a bit of a complicated matter. We only actively use it with the overdue and notice triggers - if someone else could verify it works the same as before without -t that would be helpful.
QA: Looking at this one..
(In reply to Marcel de Rooy from comment #20) > QA: Looking at this one.. Will continue later (if no one else does).
Created attachment 47335 [details] [review] Bug 15240: Do not process issues with a date due later than today There is no need to do this job in Perl, MySQL could do it instead. The idea is to only retrieve the issues info which could be overdued. To test: 1/ Run the script 2/ Apply the patch 3/ Run the script again, notice the exact same results but execution time is faster Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Only tested git-bz; please wait..
(In reply to Katrin Fischer from comment #19) > I hope to get around to it soon - but the more testing the better. Overdues > are a bit of a complicated matter. We only actively use it with the overdue > and notice triggers - if someone else could verify it works the same as > before without -t that would be helpful. No reasons to worry. Actually, the code part where the triggered condition is executed (if passed), was not touched.
Created attachment 47353 [details] [review] Bug 15240: Do not process issues with a date due later than today There is no need to do this job in Perl, MySQL could do it instead. The idea is to only retrieve the issues info which could be overdued. To test: 1/ Run the script 2/ Apply the patch 3/ Run the script again, notice the exact same results but execution time is faster Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 47354 [details] [review] Bug 15240: [QA Follow-up] Minor adjustments This patch does: [1] It removes some unused modules. [2] It adds some options not listed in the synopsis. [3] It removes an unused sql expression from one query. Note: In fines related code the third parameter of CalcFine sometimes is named as days_overdue too. [4] Corrects a few typos in comments or pod. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
QA Comment: Your change generally looks good to me. But I will be very precise here :) Note that you use TO_DAYS in the query. So you select the current date with all hours too. Formerly, records of today were compared with date_to_run (including time) eliminating the records in hours still to come. As long as $mindays is not zero (from overduerules), these records will be discarded later on (L 544-555) since mindays may not be zero in the code of this script (line 490). Another point is the -date parameter. I am not sure what it actually should do btw, since it is documented rather poorly. Looking through code, it seems to mimic running on a different date (e.g. say -date <yesterday> or -date <tomorrow> e.g.). The same argument as above could be repeated for the records of the specific date given via this parameter. This actually brings me to the conclusion that you made a large first step, but the second step would be moving the checks for mindays and maxdays into the SQL statement too. Note that it may run three times with different values for the three triggers. Another report? Related: While testing PREDUE and ODUE, I found that using -n on the command line will generate the error that the print notice for PREDUE and ODUE are not found. Copying them from email, revealed that PREDUE should use a block <item>...</item> in order to have the title displayed, or should use <items.content>. In a new installation this should be corrected. If you confirm that, we could address it on a new report too? I used the opportunity to add a few minor adjustments in a follow-up. Passed QA
(In reply to Marcel de Rooy from comment #27) > Another point is the -date parameter. I am not sure what it actually should > do btw, since it is documented rather poorly. Looking through code, it seems > to mimic running on a different date (e.g. say -date <yesterday> or -date > <tomorrow> e.g.). The same argument as above could be repeated for the > records of the specific date given via this parameter. That's exactly what it does. > This actually brings me to the conclusion that you made a large first step, > but the second step would be moving the checks for mindays and maxdays into > the SQL statement too. Note that it may run three times with different > values for the three triggers. Another report? I had in mind it was not possible, because of the calendar checks. But looking at the code again, it seems possible. > Related: While testing PREDUE and ODUE, I found that using -n on the command > line will generate the error that the print notice for PREDUE and ODUE are > not found. Copying them from email, revealed that PREDUE should use a block > <item>...</item> in order to have the title displayed, or should use > <items.content>. In a new installation this should be corrected. If you > confirm that, we could address it on a new report too? Indeed, ODUE.print should exist, we could add the print template if it does not exist. PREDUE is used by advance_notices.pl > I used the opportunity to add a few minor adjustments in a follow-up. They looks good to me. > Passed QA Thanks!
Pushed to Master - Should be in the May 2016 release. Thanks!
Patch pushed to 3.22.x, will be in 3.22.3
This patch has been pushed to 3.20.x, will be in 3.20.9.
I have tested these patches in 3.22.3 now and all works well!