|
Description
Jonathan Druart
2015-02-20 15:31:17 UTC
Created attachment 36096 [details] [review] 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. Created attachment 36150 [details] [review] 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> I had 2 guarantors/guarentees - worked ok. (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. Created attachment 36157 [details] [review] Bug 13740: Fix null vs not null in the query The previous patch did not work, only patrons *with* guanrantees were deleted! In this particular case, I think we should squash the 2 patches. It will be more safe for inclusion into stable branches. (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. 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) (In reply to Koha Team AMU from comment #7) > I try on a sandbox : And, what is the problem? Is not an error. Patch tested with a sandbox, by Koha Team AMU <koha.aixmarseille@gmail.com> Created attachment 36410 [details] [review] 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> Created attachment 36411 [details] [review] 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> 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. 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> 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> Patches pushed to master. Thanks Jonathan! Pushed to 3.18.x will be in 3.18.6 |