---- Reported by drdrulm@yahoo.com 2007-12-26 14:08:47 ---- Intranet: If the request.pl?biblionumber= hold reserve request screen is given a patron of the character % this pulls in all patrons and crashes mozilla, this may be a problem elsewhere where patrons can be searched or in other conditions where too many patrons are pulled into the selection widget ---- Additional Comments From oleonard@myacpl.org 2007-12-31 17:28:16 ---- I guess the crux of the problem is that searching for patrons by inputting '%' brings up all results. ---- Additional Comments From jmf@liblime.com 2007-12-31 18:42:28 ---- The solution here, as with the solution to an identical problem that happens with the circulation search, is to allow paged results of patrons on the request.pl and circulation.pl patron searches. This has been done with the standard patron search (member.pl), and that search should be ported to circ and reserve. Assigning to Chris. ---- Additional Comments From oleonard@myacpl.org 2008-01-01 05:35:34 ---- Is it really necessary to allow the user to search using '%' in these contexts? Someone looking for a list of all patrons could use reports. ---- Additional Comments From jmf@liblime.com 2008-03-17 16:12:52 ---- I propose that we use this opportunity to create a generic patron search API that both circulation and patron searches use, and it should feature paged search results, like in the patron search. The UI for patron and circulation searches should likewise be merged, for consistency sake. Re-assigning to Galen for now as this touches circulation and patrons. ---- Additional Comments From nengard@gmail.com 2009-12-23 17:58:42 ---- This bug hasn't been touched in nearly a year, please revisit and test and close if appropriate. ---- Additional Comments From oleonard@myacpl.org 2009-12-25 19:14:43 ---- This problem is mitigated now that patron searches from members-home.pl return paged results, but still exists when searching via the circulation search form. --- Bug imported by chris@bigballofwax.co.nz 2010-05-21 00:33 UTC --- This bug was previously known as _bug_ 1707 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=1707 Actual time not defined. Setting to 0.0 The original reporter of this bug does not have an account here. Reassigning to the person who moved it here: chris@bigballofwax.co.nz. Previous reporter was drdrulm@yahoo.com.
This is still a valid bug.
Is this still a problem with the new server side processing of the patron search table?
This is still a problem. I'd say the member search is dangerous and generates some horrible queries which takes many minutes to run if you have a large dataset. We have over 300,000 patrons. If someone search for something stupid like %29% - this is (part of) the generated query: https://gist.githubusercontent.com/boutros/db14414591ea77638b2a3a183a18ec00/raw/d59f7ef33890b7a034f24a0d350b5af3b6257e4e/bigquery.sql
(In reply to Petter Goksøyr Åsen from comment #3) > This is still a problem. I'd say the member search is dangerous and > generates some horrible queries which takes many minutes to run if you have > a large dataset. > > We have over 300,000 patrons. > > If someone search for something stupid like %29% - this is (part of) the > generated query: > https://gist.githubusercontent.com/boutros/db14414591ea77638b2a3a183a18ec00/ > raw/d59f7ef33890b7a034f24a0d350b5af3b6257e4e/bigquery.sql How d(In reply to Petter Goksøyr Åsen from comment #3) > This is still a problem. I'd say the member search is dangerous and > generates some horrible queries which takes many minutes to run if you have > a large dataset. > > We have over 300,000 patrons. > > If someone search for something stupid like %29% - this is (part of) the > generated query: > https://gist.githubusercontent.com/boutros/db14414591ea77638b2a3a183a18ec00/ > raw/d59f7ef33890b7a034f24a0d350b5af3b6257e4e/bigquery.sql For the records: http://irc.koha-community.org/koha/2016-10-13#i_1866950 Petter, how did you finally fix it? I have several ways to fix it but I don't figure out which one is the best.
I have put a limit on the query as you suggested: http://paste.koha-community.org/135 It's not in production yet, but testing in staging so far seems good. The result is probably not accurate, as you said, but I think it's only potentially wrong if you have a very wide search (i.e few numbers which matches lots of borrowernumbers). If the result list is long, say tens of thousands, it does not matter that its not accurate. If you search for full fields (in our case its social security number as extended patron attribute), the returned result is always correct In my mind, it's quite dangerous that Koha allows to generate SQL-queries which are several MB of text, which generated huge loads on our server, so I'd prefer safety to accuracy here.
Created attachment 57068 [details] [review] Bug 1707: Do not return more than 100 patrons matching the attributes search As you can feel, this patch is not perfect as we are hardcodind the number of results to return. But this number should be correct in the most cases. Test plan: With a large number of patrons, using attributes, you can search for something very generic, like '%'. Without this patch, the MySQL server could be overloaded because of the generated query (built from borrowernumbers).
Comment on attachment 57068 [details] [review] Bug 1707: Do not return more than 100 patrons matching the attributes search Review of attachment 57068 [details] [review]: ----------------------------------------------------------------- ::: C4/Members/Attributes.pm @@ +152,4 @@ > JOIN borrower_attribute_types USING (code) > WHERE staff_searchable = 1 > AND (} . join (" OR ", map "attribute like ?", @$filter) .qq{)}; > + $query .= q{ LIMIT ?} if $limit; LIMIT is a MySQLism. Perhaps DBIx would be a good way to get around this? Not that I know how to do it, but this looks like a good place to start: http://search.cpan.org/dist/DBIx-Class/lib/DBIx/Class/ResultSet.pm#rows
(In reply to M. Tompsett from comment #7) > LIMIT is a MySQLism. Hum? Ref needed please. https://www.postgresql.org/docs/current/static/queries-limit.html
(In reply to M. Tompsett from comment #7) > LIMIT is a MySQLism. If this page is accurate, LIMIT is not standard but is not unique to MySQL: http://troels.arvin.dk/db/rdbms/#select-limit IMO there is a LIMIT to how much database-agnosticism we should worry about.
Created attachment 62301 [details] [review] Bug 1707: Do not return more than 100 patrons matching the attributes search As you can feel, this patch is not perfect as we are hardcodind the number of results to return. But this number should be correct in the most cases. Test plan: With a large number of patrons, using attributes, you can search for something very generic, like '%'. Without this patch, the MySQL server could be overloaded because of the generated query (built from borrowernumbers).
Created attachment 62324 [details] [review] Bug 1707: Do not return more than 100 patrons matching the attributes search As you can feel, this patch is not perfect as we are hardcodind the number of results to return. But this number should be correct in the most cases. Test plan: With a large number of patrons, using attributes, you can search for something very generic, like '%'. Without this patch, the MySQL server could be overloaded because of the generated query (built from borrowernumbers). Signed-off-by: Marc Véron <veron@veron.ch>
I think this fixes a problem, but I'd really like to see a notification/error pushed back to the user letting them know the search was truncated and suggesting a more exact search This feels similar to bug 15812
We cannot really know the search has been truncated.
Created attachment 63335 [details] [review] Bug 1707 (QA Followup) Add a warning that attribute searches will be truncated
(In reply to Jonathan Druart from comment #13) > We cannot really know the search has been truncated. True, but in that case we can just always warn - if we are going to have a hardcoded limit it just needs to be announced
(In reply to Nick Clemens from comment #15) > (In reply to Jonathan Druart from comment #13) > > We cannot really know the search has been truncated. > > True, but in that case we can just always warn - if we are going to have a > hardcoded limit it just needs to be announced Yes but the note is wrong, and will be hard to make it correct :) Note you added says "Note: Patron attribute searching is enabled, to avoid memory issues a limit of 100 patrons will be returned during "standard" searches" If you are searching for a pattern that is used 250x in patron attribute, and 100x in other standard fields, we will certainly get less 100 results even if the whole set would have been 100.
Could we close this with the "new" API based patron search?
(In reply to Katrin Fischer from comment #17) > Could we close this with the "new" API based patron search? +1