Summary: | Searching borrowers is a lot slower if there's searchable extended attributes | ||
---|---|---|---|
Product: | Koha | Reporter: | Didier Gautheron <didier.gautheron> |
Component: | Patrons | Assignee: | Bugs List <koha-bugs> |
Status: | Failed QA --- | QA Contact: | Testopia <testopia> |
Severity: | normal | ||
Priority: | P5 - low | CC: | andreas.jonsson, bugzilla, david, dcook, gmcharlt, jonathan.druart, julian.maurice, kyle.m.hall, kyle, martin.renvoize, mengu, nick, rcoert, sally.healey, sbarter, tomascohen, wizzyrea |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: |
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33489 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30645 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33428 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34517 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36025 |
||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Bug Depends on: | 30055 | ||
Bug Blocks: | |||
Attachments: | Bug 33554: [WIP] Add a new lookup endpoint for patron searching |
Description
Didier Gautheron
2023-04-18 11:07:01 UTC
An index on those tables might help - we saw similar issues on a site with many borrowers. (In reply to Nick Clemens from comment #1) > An index on those tables might help - we saw similar issues on a site with > many borrowers. In my understanding these tables are already fully indexed. I did play with indexes' statistics but got no improvement. Would it be reasonable if making an attribute searchable didn't mean it was searched by default? i.e. we could add the attribute to the search dropdowns, or have an option on the attribute to include in searches by default I was going to suggest to have a separate "extended_attributes" parameter we could pass to the endpoint, but that's not trivial to implement (we need the search_type, and to know if we need a OR or AND with q...) (In reply to Jonathan Druart from comment #4) > I was going to suggest to have a separate "extended_attributes" parameter we > could pass to the endpoint, but that's not trivial to implement (we need the > search_type, and to know if we need a OR or AND with q...) what about storing searchable attribute values in a new borrowers column? Any time a searchable attribute is modified, we would just regenerate the text for that column. Then we would no longer need to join on the attributes tables. (In reply to Jonathan Druart from comment #4) > I was going to suggest to have a separate "extended_attributes" parameter we > could pass to the endpoint, but that's not trivial to implement (we need the > search_type, and to know if we need a OR or AND with q...) I think you'd need to pass all three parameters (the search text, the search field, and the search type) to make it work. Would that be acceptable to have this special case inside Koha::REST::V1::Patrons::list ? Tomas, any ideas how to fix this correctly? I've asked my DBA to analyze it. Nick and I have talked about making a specialized endpoint that restores the subquery, but this might be solvable with some DB optimizations. (In reply to Julian Maurice from comment #6) > (In reply to Jonathan Druart from comment #4) > > I was going to suggest to have a separate "extended_attributes" parameter we > > could pass to the endpoint, but that's not trivial to implement (we need the > > search_type, and to know if we need a OR or AND with q...) > > I think you'd need to pass all three parameters (the search text, the search > field, and the search type) to make it work. Would that be acceptable to > have this special case inside Koha::REST::V1::Patrons::list ? Should we do it on a specialized endpoint? I am working on a 'patrons/lookup' endpoint to see about building the query in a more performant way. For testing, I imported 100k borrowers with several attributes each and was able to confirm the slowdown when attributes are enabled I dumped the tables and made them available on github: https://github.com/kidclamp/sample_files/blob/main/many_attributes.gz https://github.com/kidclamp/sample_files/blob/main/many_borrowers.gz Created attachment 154331 [details] [review] Bug 33554: [WIP] Add a new lookup endpoint for patron searching This patch adds a new API lookup optoin for patrons. This endpoint takes three params: search_type - contains or starts with search_term - the string passed to search search_field - a comma separated string of fields to search The patron-search code is altered to pass these additional params, and the datatables code is updated to pass these params through The lookup code parses the attributes search directly, then adds it as a subquery to the results This should only affect patron/checkout searches - other patron searches are not changed at this time NOTE: In testing I noted that a search for a patron field plus an attribute returns nothing, but this is the same as existing code. i.e. patron Kenneth ABRAMS with SCHOOL attribute 'Oxford' is not returned for 'ken oxf' search WIP: Needs test coverage TO test: 1 - Add the sample borrowers and attributes to your DB 2 - Perform some patron searches 'ken' 'ken abr' 'oxford' 'ken oxford' 3 - Note the response times, most are slow 4 - Apply patch, restart all 5 - Repeat searches 6 - Note faster response times, but same results I feel like we could greatly simplify and speed up patron searching by simply have a new table column or columns that exist solely to contain searchable content. We could even store this in JSON format and make use of JSON functions at a later date if possible. Here are the default patron search fields: firstname,middle_name,surname,othernames,cardnumber,userid Let's say I have a patron: Joe Gerald Patronson, esquire ( 123456789 ), jpatronson Upon editing, we could store the following: { firstname: "Joe", middle_name: "Gerald", surname: "Patronson", othernames: "esquire", cardnumber: "123456789", userid: "jpatronson", } Now, if we need to search the entire field we can do a LIKE ': "%<VALUE>%\n' for contains and LIKE ': "<VALUE>%\n' for begins with If we want to search for patrons with a firstname starting with J and surname containing son it would like like WHERE newcolumn LIKE 'firstname: "J%"\n' AND newcolumn LIKE 'surname: "%son%"\n' If we do this, we can have a single column to search and index. We can also add extended attributes to this by attribute code, for example: { firstname: "Joe", middle_name: "Gerald", surname: "Patronson", othernames: "esquire", cardnumber: "123456789", userid: "jpatronson", ATTR: "value1", ATTR: "value2", } Right now we cannot search on specific attributes, but that could easily be added using this system. This allows us to search all needed patron data without the need for complex joins. I think TINYTEXT might be a bit too small, but we could definitely be fine with TEXT and we can specify the length of the index as a reasonable number ( say 500 characters or so ). Thoughts? We can also resurrect bug 17500... (In reply to Jonathan Druart from comment #13) > We can also resurrect bug 17500... I think that's the best idea. It's gonna take time though, assuming you are interesting in picking it back up. If you are I'd be happy to help how I can! In the mean time we can try to get a quick fix that gets us to acceptable. If this approach looks reasonable then I will write tests, and update to fix the attribute search. Just let me know if I should put this time in (In reply to Nick Clemens from comment #15) > If this approach looks reasonable then I will write tests, and update to fix > the attribute search. Just let me know if I should put this time in To my mind, we need a fix here sooner than later. Didier, Julian, can you have a look at this patch and test it? (In reply to Jonathan Druart from comment #17) > Didier, Julian, can you have a look at this patch and test it? For real life test we only have < 22.11.06 customers and patch doesn't apply. In my understanding it applies on to be released 22.11.10, a customer will upgrade next month and I can test it then. The patch breaks patrons search for me. If I use the top search bar it seems to work, but after that, using the form on the left doesn't do anything. If I go to patrons search by using the navigation link, the letters don't work. Comment on attachment 154331 [details] [review] Bug 33554: [WIP] Add a new lookup endpoint for patron searching Review of attachment 154331 [details] [review]: ----------------------------------------------------------------- ::: Koha/Patrons.pm @@ +93,5 @@ > + } > + $attributes_rs = Koha::Patron::Attributes->search( { -and => \@attributes_query } ); > + } > + > + push @{ $query->{-or} }, { 'me.borrowernumber' => { -in => [ $attributes_rs->get_column('borrowernumber') ] } }; This will break if ExtendedPatronAttributes is disabled ($attributes_rs will be undef) Also, if no patron attributes are searchable, `$attributes_rs->get_column('borrowernumber')` will return borrowernumbers of all borrowers that have at least one attribute. It's probably a good idea to use `$attributes_rs->_resultset->get_column('borrowernumber')->as_query` instead in order to not have to fetch all borrowernumbers first (not tested) We should generalise this into Koha::REST::Plugin::Query.. that way we could get the performance boost for illattributes, extended_attributes and borrower_attributes who all follow the same patturn. I use a searchable extended attribute as part of a workflow for moving patrons into different patron lists. Having the attribute available as a choice in the Search field drop-down would work well. We have over 277,000 patrons so this does have quite an impact. |