[Bug 29134] New: Patron search has poor performance when ExtendedAttributes enabled and many attributes match
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 Bug ID: 29134 Summary: Patron search has poor performance when ExtendedAttributes enabled and many attributes match Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: nick@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org When system preference ExtendedPatronAttributes is enabled the patron search finds all patrons with a matching attribute and builds a where string with all the borrowernumbers concatted into an 'IN' 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; } When there are too many matches on a system with many patrons/attributes this generates a very large query which takes a very long time to return -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 --- Comment #1 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 125451 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=125451&action=edit 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=15812 Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 --- Comment #2 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 125453 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=125453&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com --- Comment #3 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Jonathan Druart from comment #2)
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.
~600-700ms with Nick's patch 22s on master ~600-700ms with this alternative patch. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 --- Comment #4 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #3)
(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.
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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #5 from David Nind <david@davidnind.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 --- Comment #6 from Nick Clemens <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 --- Comment #8 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #125451|0 |1 is obsolete| | --- Comment #9 from David Nind <david@davidnind.com> --- Created attachment 125568 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=125568&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 --- Comment #10 from 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 Joonas Kylmälä <joonas.kylmala@iki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 Joonas Kylmälä <joonas.kylmala@iki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #125453|0 |1 is obsolete| | Attachment #125568|0 |1 is obsolete| | --- Comment #11 from Joonas Kylmälä <joonas.kylmala@iki.fi> --- Created attachment 125649 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=125649&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 Joonas Kylmälä <joonas.kylmala@iki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joonas.kylmala@iki.fi QA Contact|testopia@bugs.koha-communit |joonas.kylmala@iki.fi |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |21.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 --- Comment #12 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Pushed to master for 21.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|21.11.00 |21.11.00,21.05.05 released in| | Status|Pushed to master |Pushed to stable CC| |kyle@bywatersolutions.com --- Comment #13 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to 21.05.x for 21.05.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 --- Comment #14 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 125978 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=125978&action=edit 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 --- Comment #15 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Which means the original change was missing test coverage.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | --- Comment #16 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Nick Clemens from comment #14)
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!
Pushed to master. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|21.11.00,21.05.05 |21.11.00,21.05.05,20.11.11 released in| | Status|Pushed to stable |Pushed to oldstable CC| |fridolin.somers@biblibre.co | |m --- Comment #17 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 20.11.x for 20.11.11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |victor@tuxayo.net --- Comment #18 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Followup missing from 21.05.x and 20.11.x, RMaints notified. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 --- Comment #19 from Fridolin Somers <fridolin.somers@biblibre.com> --- Followup pushed to 20.11.x for 20.11.11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 --- Comment #20 from Kyle M Hall <kyle@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 --- Comment #21 from Victor Grousset/tuxayo <victor@tuxayo.net> --- 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)
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 --- Comment #22 from Victor Grousset/tuxayo <victor@tuxayo.net> --- comment 21 invalid, I was on the wrong branch... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29134 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to oldstable |Pushed to oldoldstable Version(s)|21.11.00,21.05.05,20.11.11 |21.11.00,21.05.05,20.11.11, released in| |20.05.17 --- Comment #23 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Backported: Pushed to 20.05.x branch for 20.05.17 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org