Description
Nick Clemens (kidclamp)
2021-09-29 12:26:48 UTC
Created attachment 125451 [details] [review] Bug 29134: Use a subquery to increase performance of patron attributes search This patch generates a subquery and checks if a borrowrnumber is in the results to add patrons to search results To test: 1 - Generate a bunch of patrons: SELECT surname, firstname, branchcode, categorycode FROM ( SELECT surname FROM borrowers ORDER BY rand() ) a,( SELECT firstname FROM borrowers ORDER BY rand() ) b,( SELECT branchcode FROM borrowers ORDER BY rand() ) c,( SELECT categorycode FROM borrowers ORDER BY rand() ) d LIMIT 50000 2 - Add a patron attribute to the system and make it searchable - I used code 'TEST' 3 - Add a value for this attribute to many patrons: INSERT INTO borrower_attributes (borrowernumber,code,attribute) SELECT borrowernumber, 'TEST','alphabet' FROM borrowers LIMIT 10000; 4 - In staff client got 'Patrons' 5 - Open the browser console (F12) and view the netwrok tab 6 - Perform a patron search for 'a' 7 - Note the time it takes for 'search' to complete in console 8 - Apply patch, restart_all 9 - Repeat search 10 - Note it is much faster 11 - prove -v t/db_dependent/Utils/Datatables_Members.t NOTE: I tested with 500k patrons and 100k attributes - search returned in ~2 seconds with patch and did not return before I got impatient without patch Created attachment 125453 [details] [review] Bug 29134: [ALTERNATIVE patch] Keep filter_by_attribute_value And build the IN using the existing @matching_borrowernumbers. We have a method for that, we should not use raw SQL. (In reply to Jonathan Druart from comment #2) > Created attachment 125453 [details] [review] [review] > Bug 29134: [ALTERNATIVE patch] Keep filter_by_attribute_value > > And build the IN using the existing @matching_borrowernumbers. > We have a method for that, we should not use raw SQL. ~600-700ms with Nick's patch 22s on master ~600-700ms with this alternative patch. (In reply to Jonathan Druart from comment #3) > (In reply to Jonathan Druart from comment #2) > > Created attachment 125453 [details] [review] [review] [review] > > Bug 29134: [ALTERNATIVE patch] Keep filter_by_attribute_value > > > > And build the IN using the existing @matching_borrowernumbers. > > We have a method for that, we should not use raw SQL. > > ~600-700ms with Nick's patch > 22s on master > ~600-700ms with this alternative patch. At 500k patrons with 100k attributes the alternate is a bit slower The query can still be large (caharacter count) with the IN, and requires 2 db calls, one for filter_borrowers_by_attribute, another to search the borrower DV Moving to Objects is nice, but I think pure SQL here makes more sense than mixing the two - I opened bug 29125 to remove all the SQL here I attempted to test (using koha-testing-docker - started with ku). For whatever reason, the SQL in step 1 didn't add the new patrons to the database - I still just have the standard 53 patrons you normally get with KTD. (In reply to David Nind from comment #5) > I attempted to test (using koha-testing-docker - started with ku). > > For whatever reason, the SQL in step 1 didn't add the new patrons to the > database - I still just have the standard 53 patrons you normally get with > KTD. That was a typo, it should be:" INSERT into borrowers(surname, firstname, branchcode, categorycode) SELECT surname, firstname, branchcode, categorycode FROM ( SELECT surname FROM borrowers ORDER BY rand() ) a,( SELECT firstname FROM borrowers ORDER BY rand() ) b,( SELECT branchcode FROM borrowers ORDER BY rand() ) c,( SELECT categorycode FROM borrowers ORDER BY rand() ) d LIMIT 50000 I'm not all that familiar with C4::Utils::DataTables::Members, but I having had a quick read I'm not averse to Nick approach of having it 'all sql'. With that said, I'd really love to wrap my head around what that module is used for at all.. it feels like we should be able to move to using the patron API and our datatables wrapper for this now in most cases? (In reply to Martin Renvoize from comment #7) > I'm not all that familiar with C4::Utils::DataTables::Members, but I having > had a quick read I'm not averse to Nick approach of having it 'all sql'. > > With that said, I'd really love to wrap my head around what that module is > used for at all.. it feels like we should be able to move to using the > patron API and our datatables wrapper for this now in most cases? It's the first module that provided a JSON for DataTables in AJAX (so with filter, ordering, etc.) It does a lot of things, and using the REST API (with the DT wrapper) won't be easy. It's used for the different patron searches Nick wrote something on bug 29125, to remove the raw SQL. Created attachment 125568 [details] [review] Bug 29134: Use a subquery to increase performance of patron attributes search This patch generates a subquery and checks if a borrowrnumber is in the results to add patrons to search results To test: 1 - Generate a bunch of patrons: SELECT surname, firstname, branchcode, categorycode FROM ( SELECT surname FROM borrowers ORDER BY rand() ) a,( SELECT firstname FROM borrowers ORDER BY rand() ) b,( SELECT branchcode FROM borrowers ORDER BY rand() ) c,( SELECT categorycode FROM borrowers ORDER BY rand() ) d LIMIT 50000 2 - Add a patron attribute to the system and make it searchable - I used code 'TEST' 3 - Add a value for this attribute to many patrons: INSERT INTO borrower_attributes (borrowernumber,code,attribute) SELECT borrowernumber, 'TEST','alphabet' FROM borrowers LIMIT 10000; 4 - In staff client got 'Patrons' 5 - Open the browser console (F12) and view the netwrok tab 6 - Perform a patron search for 'a' 7 - Note the time it takes for 'search' to complete in console 8 - Apply patch, restart_all 9 - Repeat search 10 - Note it is much faster 11 - prove -v t/db_dependent/Utils/Datatables_Members.t NOTE: I tested with 500k patrons and 100k attributes - search returned in ~2 seconds with patch and did not return before I got impatient without patch Signed-off-by: David Nind <david@davidnind.com> (In reply to Nick Clemens from comment #6) > (In reply to David Nind from comment #5) .. > That was a typo, it should be:" > INSERT into borrowers(surname, firstname, branchcode, categorycode) SELECT > surname, firstname, branchcode, categorycode FROM ( SELECT surname FROM > borrowers ORDER BY rand() ) a,( SELECT firstname FROM borrowers ORDER BY > rand() ) b,( SELECT branchcode FROM borrowers ORDER BY rand() ) c,( SELECT > categorycode FROM borrowers ORDER BY rand() ) d LIMIT 50000 Thanks Nick - now works as expected, and was able to test. Before patch applied (koha-testing-docker): 19.71 s After: 1.81 s Created attachment 125649 [details] [review] Bug 29134: Use a subquery to increase performance of patron attributes search This patch generates a subquery and checks if a borrowrnumber is in the results to add patrons to search results To test: 1 - Generate a bunch of patrons: SELECT surname, firstname, branchcode, categorycode FROM ( SELECT surname FROM borrowers ORDER BY rand() ) a,( SELECT firstname FROM borrowers ORDER BY rand() ) b,( SELECT branchcode FROM borrowers ORDER BY rand() ) c,( SELECT categorycode FROM borrowers ORDER BY rand() ) d LIMIT 50000 2 - Add a patron attribute to the system and make it searchable - I used code 'TEST' 3 - Add a value for this attribute to many patrons: INSERT INTO borrower_attributes (borrowernumber,code,attribute) SELECT borrowernumber, 'TEST','alphabet' FROM borrowers LIMIT 10000; 4 - In staff client got 'Patrons' 5 - Open the browser console (F12) and view the netwrok tab 6 - Perform a patron search for 'a' 7 - Note the time it takes for 'search' to complete in console 8 - Apply patch, restart_all 9 - Repeat search 10 - Note it is much faster 11 - prove -v t/db_dependent/Utils/Datatables_Members.t NOTE: I tested with 500k patrons and 100k attributes - search returned in ~2 seconds with patch and did not return before I got impatient without patch Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Pushed to master for 21.11, thanks to everybody involved! Pushed to 21.05.x for 21.05.05 Created attachment 125978 [details] [review] Bug 29134: (follow-up) Specify which borrowernumber When performing a search that includes permissions we need to specify we are comparing the borrowers tabnle borrowernumber To test: 1 - Browse to Admin->Funds 2 - Edit a fund 3 - Click '+ Add users' 4 - Attempt a search, no results 5 - Check plack-error.log C4::Utils::DataTables::Members::search(): DBI Exception: DBD::mysql::st execute failed: Column 'borrowernumber' in IN/ALL/ANY subquery is ambiguous at /kohadevbox/koha/svc/members/search line 80 6 - Apply patch 7 - Repeat 1-3 8 - search for 'a' 9 - Result! Which means the original change was missing test coverage.. (In reply to Nick Clemens from comment #14) > Created attachment 125978 [details] [review] [review] > Bug 29134: (follow-up) Specify which borrowernumber > > When performing a search that includes permissions we need to specify we are > comparing > the borrowers tabnle borrowernumber > > To test: > 1 - Browse to Admin->Funds > 2 - Edit a fund > 3 - Click '+ Add users' > 4 - Attempt a search, no results > 5 - Check plack-error.log > C4::Utils::DataTables::Members::search(): DBI Exception: DBD::mysql::st > execute failed: Column 'borrowernumber' in IN/ALL/ANY subquery is ambiguous > at /kohadevbox/koha/svc/members/search line 80 > 6 - Apply patch > 7 - Repeat 1-3 > 8 - search for 'a' > 9 - Result! Pushed to master. Pushed to 20.11.x for 20.11.11 Followup missing from 21.05.x and 20.11.x, RMaints notified. Followup pushed to 20.11.x for 20.11.11 (In reply to Fridolin Somers from comment #19) > Followup pushed to 20.11.x for 20.11.11 Followup pushed to 21.05.x for 20.05.05 I can't backport to 20.05.x , help needed if someone needs this patch in 20.05.x and 19.11.x The conflict is on a few lines but they changes a lot between 20.05 and 20.11. Clarification of the conflict format display <<<<<<< HEAD # code from current 20.05.x ======= # code from upstream 20.11.x before the patch ||||||| parent of 123abc123abc # code from upstream 20.11.x after the patch >>>>>>> 123abc123abc Actuall conflict: <<<<<<< HEAD push @where_strs_or, " borrowers.borrowernumber IN ( SELECT DISTINCT borrowernumber FROM borrower_attributes JOIN borrower_attribute_types USING (code) WHERE staff_searchable = 1 AND attribute LIKE ? ) "; push @where_args, $term; ||||||| parent of 3c4c2ebe9d (Bug 29134: Use a subquery to increase performance of patron attributes search) my @matching_borrowernumbers = Koha::Patrons->filter_by_attribute_value($searchmember)->get_column('borrowernumber'); for my $borrowernumber ( @matching_borrowernumbers ) { push @where_strs_or, "borrowers.borrowernumber = ?"; push @where_args, $borrowernumber; } ======= push @where_strs_or, " borrowernumber IN ( SELECT DISTINCT borrowernumber FROM borrower_attributes JOIN borrower_attribute_types USING (code) WHERE staff_searchable = 1 AND attribute LIKE ? ) "; push @where_args, $term; >>>>>>> 3c4c2ebe9d (Bug 29134: Use a subquery to increase performance of patron attributes search) comment 21 invalid, I was on the wrong branch... Backported: Pushed to 20.05.x branch for 20.05.17 |