Bug 33554 - searching borrowers is a lot slower if there's searchable extended attributes.
Summary: searching borrowers is a lot slower if there's searchable extended attributes.
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 30055
Blocks:
  Show dependency treegraph
 
Reported: 2023-04-18 11:07 UTC by Didier Gautheron
Modified: 2024-02-08 01:39 UTC (History)
14 users (show)

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


Attachments
Bug 33554: [WIP] Add a new lookup endpoint for patron searching (13.14 KB, patch)
2023-08-09 13:59 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 Didier Gautheron 2023-04-18 11:07:01 UTC
Hi,

Since BZ 30055 search is using the REST API.

For a library with 80,000 borrowers and  800,000 borrower_attributes, (one searchable per borrower), searching a borrower is twice as slow and is noticeable in checkin/checkout.

before 30055 the request was something like :
SELECT borrowernumber FROM borrowers 
WHERE firstname` LIKE '%foo%' 
OR middle_name LIKE '%foo%' 
OR surname LIKE '%foo%' 
OR othernames LIKE '%foo%' 
OR cardnumber LIKE '%foo%' 
OR userid LIKE '%foo%'
OR borrowernumber in ( 
  select borrowernumber from borrower_attributes where attribute LIKE '%foo%' AND code = 'BAR' ) 
GROUP BY borrowernumber;
runtime 0.069 sec

in master
SELECT borrowernumber FROM borrowers 
LEFT JOIN borrower_attributes USING(borrowernumber) 
WHERE firstname` LIKE '%foo%' 
OR middle_name LIKE '%foo%' 
OR surname LIKE '%foo%' 
OR othernames LIKE '%foo%' 
OR cardnumber LIKE '%foo%' 
OR userid LIKE '%foo%'
OR (attribute LIKE '%foo%' AND code = 'BAR' ) 
GROUP BY borrowernumber;

runtime 1.324 sec
Comment 1 Nick Clemens 2023-04-18 11:24:17 UTC
An index on those tables might help - we saw similar issues on a site with many borrowers.
Comment 2 Didier Gautheron 2023-05-02 11:07:27 UTC
(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.
Comment 3 Nick Clemens 2023-05-24 16:41:37 UTC
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
Comment 4 Jonathan Druart 2023-05-25 13:20:47 UTC
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...)
Comment 5 Kyle M Hall 2023-06-07 16:14:35 UTC
(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.
Comment 6 Julian Maurice 2023-06-19 09:12:19 UTC
(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 ?
Comment 7 Jonathan Druart 2023-07-27 12:38:33 UTC
Tomas, any ideas how to fix this correctly?
Comment 8 Tomás Cohen Arazi 2023-08-08 12:56:25 UTC
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.
Comment 9 Tomás Cohen Arazi 2023-08-08 13:00:10 UTC
(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?
Comment 10 Nick Clemens 2023-08-08 13:21:03 UTC
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
Comment 11 Nick Clemens 2023-08-09 13:59:16 UTC
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
Comment 12 Kyle M Hall 2023-08-09 14:44:47 UTC
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?
Comment 13 Jonathan Druart 2023-08-09 16:19:40 UTC
We can also resurrect bug 17500...
Comment 14 Kyle M Hall 2023-08-10 16:05:05 UTC
(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.
Comment 15 Nick Clemens 2023-08-10 16:30:06 UTC
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
Comment 16 Liz Rea 2023-08-21 21:34:02 UTC
(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.
Comment 17 Jonathan Druart 2023-08-30 13:15:41 UTC
Didier, Julian, can you have a look at this patch and test it?
Comment 18 Didier Gautheron 2023-09-16 11:28:01 UTC
(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.
Comment 19 Julian Maurice 2023-10-10 09:21:07 UTC
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 20 Julian Maurice 2023-10-10 09:25:37 UTC
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)
Comment 21 Martin Renvoize 2023-11-08 12:13:14 UTC
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.