Bug 29134 - Patron search has poor performance when ExtendedAttributes enabled and many attributes match
Summary: Patron search has poor performance when ExtendedAttributes enabled and many a...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Joonas Kylmälä
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-09-29 12:26 UTC by Nick Clemens
Modified: 2022-06-06 20:27 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.05,20.11.11,20.05.17


Attachments
Bug 29134: Use a subquery to increase performance of patron attributes search (2.52 KB, patch)
2021-09-29 12:33 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29134: [ALTERNATIVE patch] Keep filter_by_attribute_value (1.31 KB, patch)
2021-09-29 13:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29134: Use a subquery to increase performance of patron attributes search (2.57 KB, patch)
2021-09-30 18:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 29134: Use a subquery to increase performance of patron attributes search (2.71 KB, patch)
2021-10-02 13:47 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29134: (follow-up) Specify which borrowernumber (1.63 KB, patch)
2021-10-08 14:46 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-09-29 12:26:48 UTC
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
Comment 1 Nick Clemens 2021-09-29 12:33:20 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
Comment 2 Jonathan Druart 2021-09-29 13:07:54 UTC
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.
Comment 3 Jonathan Druart 2021-09-29 13:09:55 UTC
(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.
Comment 4 Nick Clemens 2021-09-29 16:09:37 UTC
(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
Comment 5 David Nind 2021-09-29 23:31:30 UTC
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.
Comment 6 Nick Clemens 2021-09-30 11:09:43 UTC
(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
Comment 7 Martin Renvoize 2021-09-30 12:35:53 UTC
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?
Comment 8 Jonathan Druart 2021-09-30 12:54:28 UTC
(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.
Comment 9 David Nind 2021-09-30 18:37:53 UTC
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>
Comment 10 David Nind 2021-09-30 18:42:56 UTC
(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
Comment 11 Joonas Kylmälä 2021-10-02 13:47:03 UTC
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>
Comment 12 Jonathan Druart 2021-10-06 08:38:48 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 13 Kyle M Hall 2021-10-08 13:58:25 UTC
Pushed to 21.05.x for 21.05.05
Comment 14 Nick Clemens 2021-10-08 14:46:10 UTC
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!
Comment 15 Jonathan Druart 2021-10-08 14:51:23 UTC
Which means the original change was missing test coverage..
Comment 16 Jonathan Druart 2021-10-08 15:02:08 UTC
(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.
Comment 17 Fridolin Somers 2021-10-21 04:11:02 UTC
Pushed to 20.11.x for 20.11.11
Comment 18 Victor Grousset/tuxayo 2021-10-26 00:58:31 UTC
Followup missing from 21.05.x and 20.11.x, RMaints notified.
Comment 19 Fridolin Somers 2021-10-26 06:11:17 UTC
Followup pushed to 20.11.x for 20.11.11
Comment 20 Kyle M Hall 2021-10-26 12:43:52 UTC
(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
Comment 21 Victor Grousset/tuxayo 2021-10-26 21:19:33 UTC Comment hidden (obsolete)
Comment 22 Victor Grousset/tuxayo 2021-10-26 21:20:42 UTC
comment 21 invalid, I was on the wrong branch...
Comment 23 Victor Grousset/tuxayo 2021-10-26 21:41:20 UTC
Backported: Pushed to 20.05.x branch for 20.05.17