Bug 13740 - Timeout on deleting patrons if too many patrons
Summary: Timeout on deleting patrons if too many patrons
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-02-20 15:31 UTC by Jonathan Druart
Modified: 2015-12-03 22:02 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 13740: Remove the NOT NULL clause in GetBorrowersToExpunge (2.04 KB, patch)
2015-02-20 15:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13740: Remove the NOT NULL clause in GetBorrowersToExpunge (2.11 KB, patch)
2015-02-25 04:37 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 13740: Fix null vs not null in the query (800 bytes, patch)
2015-02-25 09:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13740: Remove the NOT NULL clause in GetBorrowersToExpunge (2.16 KB, patch)
2015-03-03 14:57 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 13740: Fix null vs not null in the query (866 bytes, patch)
2015-03-03 14:57 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
[PASSED QA] Bug 13740: Remove the NOT NULL clause in GetBorrowersToExpunge (2.23 KB, patch)
2015-03-13 18:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13740: Fix null vs not null in the query (936 bytes, patch)
2015-03-13 18:07 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 Jonathan Druart 2015-02-20 15:31:17 UTC
C4::Borrowers::GetBorrowersToExpunge should not use a "NOT IN", it is not efficient at all.

With only 1 guarantor and more than 136k patrons, the not in clause in this subroutine takes ages:
mysql> select count(*) FROM   borrowers where  borrowernumber NOT IN (SELECT guarantorid FROM borrowers WHERE guarantorid IS NOT NULL AND guarantorid <> 0) ;
[...]

not ended after 5min
Comment 1 Jonathan Druart 2015-02-20 15:34:26 UTC Comment hidden (obsolete)
Comment 2 Brendan Gallagher 2015-02-25 04:37:15 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2015-02-25 09:54:39 UTC
(In reply to Brendan Gallagher from comment #2)
> Created attachment 36150 [details] [review] [review]
> Bug 13740: Remove the NOT NULL clause in GetBorrowersToExpunge
> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
> I had 2 guarantors/guarentees - worked ok.

Arg, the partner has found a problem with this patch.
Actually it catches the patrons *with* a guarantor, and it is not what we want here.
I will submit a follow-up and switch the status back to needs signoff.
Comment 4 Jonathan Druart 2015-02-25 09:55:49 UTC Comment hidden (obsolete)
Comment 5 Fridolin Somers 2015-02-26 13:38:54 UTC
In this particular case, I think we should squash the 2 patches.
It will be more safe for inclusion into stable branches.
Comment 6 Jonathan Druart 2015-02-26 14:00:38 UTC
(In reply to Fridolin SOMERS from comment #5)
> In this particular case, I think we should squash the 2 patches.
> It will be more safe for inclusion into stable branches.

Why is it more safe?
The first patch has already been signed off, and the second patch highlights the sql error.
Comment 7 Koha Team AMU 2015-03-02 15:00:50 UTC
I try on a sandbox :
Updatedatabase:
Upgrade to 3.19.00.000 done (there's life after 3.18)\nUpgrade to 3.19.00.001 done (Bug 13436: Add more options to UsageStatsLibraryType)\nupgrade to 3.19.00.002 done (Bug 10753: replace __ANY__ with empty string in suggestions.branchcode)\nUpgrade to 3.19.00.003 done (Bug 1861 - Unique patrons logins not (totally) enforced)\nUpgrade to 3.19.00.004 done (Bug 13346: OpacExportOptions is now multiple)\nUpgrade to 3.19.00.005 done (Bug 13379 - Modify authorised_values.category to varchar(32))\nUpgrade to 3.19.00.006 done (Bug 11944 - Convert DB tables to utf8_unicode_ci)\nUpgrade to 3.19.00.007 done (Bug 12905: Check budget integrity: OK)\nUpgrade to 3.19.00.008 done (Bug 12601 - Add new foreign key aqorders.budget_id WARNING: temporary budget and fund have been created (search for "BACKUP_TMP"). At least one of your order was not linked to a budget)\nUpgrade to 3.19.00.009 done (Bug 13007 - Add new foreign key suggestions.budgetid)\nUpgrade to 3.19.00.010 done (Bug 5511 - SessionRestrictionByIP)\nUpgrade to 3.19.00.011 done (Bug 13417: Add permission to delete public lists)\nUpgrade to 3.19.00.012 done (Bug 13523 - Remove NOT NULL restriction on field marcxml due to mysql STRICT_TRANS_TABLES)
Comment 8 Jonathan Druart 2015-03-02 15:56:38 UTC
(In reply to Koha Team AMU from comment #7)
> I try on a sandbox :

And, what is the problem?
Is not an error.
Comment 9 Biblibre Sandboxes 2015-03-03 14:57:14 UTC
Patch tested with a sandbox, by Koha Team AMU <koha.aixmarseille@gmail.com>
Comment 10 Biblibre Sandboxes 2015-03-03 14:57:42 UTC Comment hidden (obsolete)
Comment 11 Biblibre Sandboxes 2015-03-03 14:57:45 UTC Comment hidden (obsolete)
Comment 12 Koha Team AMU 2015-03-03 15:08:21 UTC
Sorry, no problem to apply the patch. Patrons are not deleted if they are guarantor or if there is a fee. So, ok for the deletion.
About the massive deletion, we tried it on a sandbox, but the import tool was not efficient. We tried on our server : ok. Even if a message tell us that time is out, the deletion will end.
Comment 13 Kyle M Hall 2015-03-13 18:07:22 UTC
Created attachment 36873 [details] [review]
[PASSED QA] Bug 13740: Remove the NOT NULL clause in GetBorrowersToExpunge

C4::Borrowers::GetBorrowersToExpunge should not use a "NOT IN", it is
not efficient at all.

With only 1 guarantor and more than 136k patrons, the not in clause in
this subroutine takes ages:
mysql> select count(*) FROM   borrowers where  borrowernumber NOT IN
(SELECT guarantorid FROM borrowers WHERE guarantorid IS NOT NULL AND
guarantorid <> 0) ;
[...]

not ended after 5min

With the query modified by this patch, the results come after 1 sec :)

Test plan:
Verify the delete_patrons.pl cronjob or the cleanborrowers tools work as
before.
Especially with guarantors.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Signed-off-by: Koha Team AMU <koha.aixmarseille@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Kyle M Hall 2015-03-13 18:07:32 UTC
Created attachment 36874 [details] [review]
[PASSED QA] Bug 13740: Fix null vs not null in the query

The previous patch did not work, only patrons *with* guanrantees were
deleted!

Signed-off-by: Koha Team AMU <koha.aixmarseille@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Tomás Cohen Arazi 2015-03-31 14:04:31 UTC
Patches pushed to master.

Thanks Jonathan!
Comment 16 Chris Cormack 2015-04-03 03:37:58 UTC
Pushed to 3.18.x will be in 3.18.6