Summary: | Search filter on receive orders page crashes the system | ||
---|---|---|---|
Product: | Koha | Reporter: | Andreas Jonsson <andreas.jonsson> |
Component: | REST API | Assignee: | Bugs List <koha-bugs> |
Status: | CONFIRMED --- | QA Contact: | |
Severity: | critical | ||
Priority: | P5 - low | CC: | dcook, jonathan.druart, martin.renvoize, nick, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: |
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36025 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38130 |
||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Attachments: | Bug 35361: Replace prefetch with join in REST API. |
Description
Andreas Jonsson
2023-11-17 16:01:50 UTC
This is just an instance of a much bigger problem with DBIx. For instance the SQL code of an ordinary borrower search looks like this, once again, the LIMIT-clause is applied on the subquery: SELECT `me`.`borrowernumber`, `me`.`cardnumber`, `me`.`surname`, `me`.`firstname`, `me`.`middle_name`, `me`.`title`, `me`.`othernames`, `me`.`initials`, `me`.`pronouns`, `me`.`streetnumber`, `me`.`streettype`, `me`.`address`, `me`.`address2`, `me`.`city`, `me`.`state`, `me`.`zipcode`, `me`.`country`, `me`.`email`, `me`.`phone`, `me`.`mobile`, `me`.`fax`, `me`.`emailpro`, `me`.`phonepro`, `me`.`B_streetnumber`, `me`.`B_streettype`, `me`.`B_address`, `me`.`B_address2`, `me`.`B_city`, `me`.`B_state`, `me`.`B_zipcode`, `me`.`B_country`, `me`.`B_email`, `me`.`B_phone`, `me`.`dateofbirth`, `me`.`branchcode`, `me`.`categorycode`, `me`.`dateenrolled`, `me`.`dateexpiry`, `me`.`password_expiration_date`, `me`.`date_renewed`, `me`.`gonenoaddress`, `me`.`lost`, `me`.`debarred`, `me`.`debarredcomment`, `me`.`contactname`, `me`.`contactfirstname`, `me`.`contacttitle`, `me`.`borrowernotes`, `me`.`relationship`, `me`.`sex`, `me`.`password`, `me`.`secret`, `me`.`auth_method`, `me`.`flags`, `me`.`userid`, `me`.`opacnote`, `me`.`contactnote`, `me`.`sort1`, `me`.`sort2`, `me`.`altcontactfirstname`, `me`.`altcontactsurname`, `me`.`altcontactaddress1`, `me`.`altcontactaddress2`, `me`.`altcontactaddress3`, `me`.`altcontactstate`, `me`.`altcontactzipcode`, `me`.`altcontactcountry`, `me`.`altcontactphone`, `me`.`smsalertnumber`, `me`.`sms_provider_id`, `me`.`privacy`, `me`.`privacy_guarantor_fines`, `me`.`privacy_guarantor_checkouts`, `me`.`checkprevcheckout`, `me`.`updated_on`, `me`.`lastseen`, `me`.`lang`, `me`.`login_attempts`, `me`.`overdrive_auth_token`, `me`.`anonymized`, `me`.`autorenew_checkouts`, `me`.`primary_contact_method`, `extended_attributes`.`id`, `extended_attributes`.`borrowernumber`, `extended_attributes`.`code`, `extended_attributes`.`attribute` FROM (SELECT `me`.`borrowernumber`, `me`.`cardnumber`, `me`.`surname`, `me`.`firstname`, `me`.`middle_name`, `me`.`title`, `me`.`othernames`, `me`.`initials`, `me`.`pronouns`, `me`.`streetnumber`, `me`.`streettype`, `me`.`address`, `me`.`address2`, `me`.`city`, `me`.`state`, `me`.`zipcode`, `me`.`country`, `me`.`email`, `me`.`phone`, `me`.`mobile`, `me`.`fax`, `me`.`emailpro`, `me`.`phonepro`, `me`.`B_streetnumber`, `me`.`B_streettype`, `me`.`B_address`, `me`.`B_address2`, `me`.`B_city`, `me`.`B_state`, `me`.`B_zipcode`, `me`.`B_country`, `me`.`B_email`, `me`.`B_phone`, `me`.`dateofbirth`, `me`.`branchcode`, `me`.`categorycode`, `me`.`dateenrolled`, `me`.`dateexpiry`, `me`.`password_expiration_date`, `me`.`date_renewed`, `me`.`gonenoaddress`, `me`.`lost`, `me`.`debarred`, `me`.`debarredcomment`, `me`.`contactname`, `me`.`contactfirstname`, `me`.`contacttitle`, `me`.`borrowernotes`, `me`.`relationship`, `me`.`sex`, `me`.`password`, `me`.`secret`, `me`.`auth_method`, `me`.`flags`, `me`.`userid`, `me`.`opacnote`, `me`.`contactnote`, `me`.`sort1`, `me`.`sort2`, `me`.`altcontactfirstname`, `me`.`altcontactsurname`, `me`.`altcontactaddress1`, `me`.`altcontactaddress2`, `me`.`altcontactaddress3`, `me`.`altcontactstate`, `me`.`altcontactzipcode`, `me`.`altcontactcountry`, `me`.`altcontactphone`, `me`.`smsalertnumber`, `me`.`sms_provider_id`, `me`.`privacy`, `me`.`privacy_guarantor_fines`, `me`.`privacy_guarantor_checkouts`, `me`.`checkprevcheckout`, `me`.`updated_on`, `me`.`lastseen`, `me`.`lang`, `me`.`login_attempts`, `me`.`overdrive_auth_token`, `me`.`anonymized`, `me`.`autorenew_checkouts`, `me`.`primary_contact_method` FROM `borrowers` `me` LEFT JOIN `borrower_attributes` `extended_attributes` ON `extended_attributes`.`borrowernumber` = `me`.`borrowernumber` WHERE ( ( ( `me`.`borrowernumber` LIKE '%nil%' OR `me`.`email` LIKE '%nil%' OR `me`.`surname` LIKE '%nil%' OR `me`.`firstname` LIKE '%nil%' OR `me`.`othernames` LIKE '%nil%' OR `me`.`cardnumber` LIKE '%nil%' OR `me`.`userid` LIKE '%nil%' OR `me`.`B_address` LIKE '%nil%' OR `me`.`middle_name` LIKE '%nil%' ) OR ( `me`.`borrowernumber` LIKE '%nil%' OR `me`.`email` LIKE '%nil%' OR `me`.`surname` LIKE '%nil%' OR `me`.`firstname` LIKE '%nil%' OR `me`.`othernames` LIKE '%nil%' OR `me`.`cardnumber` LIKE '%nil%' OR `me`.`userid` LIKE '%nil%' OR `me`.`B_address` LIKE '%nil%' OR `me`.`middle_name` LIKE '%nil%' ) OR ( `extended_attributes`.`attribute` LIKE '%nil%' AND ( `extended_attributes`.`code` = 'BUSS' OR `extended_attributes`.`code` = 'MEROPPET' ) ) ) ) GROUP BY `me`.`borrowernumber`, `me`.`cardnumber`, `me`.`surname`, `me`.`firstname`, `me`.`middle_name`, `me`.`title`, `me`.`othernames`, `me`.`initials`, `me`.`pronouns`, `me`.`streetnumber`, `me`.`streettype`, `me`.`address`, `me`.`address2`, `me`.`city`, `me`.`state`, `me`.`zipcode`, `me`.`country`, `me`.`email`, `me`.`phone`, `me`.`mobile`, `me`.`fax`, `me`.`emailpro`, `me`.`phonepro`, `me`.`B_streetnumber`, `me`.`B_streettype`, `me`.`B_address`, `me`.`B_address2`, `me`.`B_city`, `me`.`B_state`, `me`.`B_zipcode`, `me`.`B_country`, `me`.`B_email`, `me`.`B_phone`, `me`.`dateofbirth`, `me`.`branchcode`, `me`.`categorycode`, `me`.`dateenrolled`, `me`.`dateexpiry`, `me`.`password_expiration_date`, `me`.`date_renewed`, `me`.`gonenoaddress`, `me`.`lost`, `me`.`debarred`, `me`.`debarredcomment`, `me`.`contactname`, `me`.`contactfirstname`, `me`.`contacttitle`, `me`.`borrowernotes`, `me`.`relationship`, `me`.`sex`, `me`.`password`, `me`.`secret`, `me`.`auth_method`, `me`.`flags`, `me`.`userid`, `me`.`opacnote`, `me`.`contactnote`, `me`.`sort1`, `me`.`sort2`, `me`.`altcontactfirstname`, `me`.`altcontactsurname`, `me`.`altcontactaddress1`, `me`.`altcontactaddress2`, `me`.`altcontactaddress3`, `me`.`altcontactstate`, `me`.`altcontactzipcode`, `me`.`altcontactcountry`, `me`.`altcontactphone`, `me`.`smsalertnumber`, `me`.`sms_provider_id`, `me`.`privacy`, `me`.`privacy_guarantor_fines`, `me`.`privacy_guarantor_checkouts`, `me`.`checkprevcheckout`, `me`.`updated_on`, `me`.`lastseen`, `me`.`lang`, `me`.`login_attempts`, `me`.`overdrive_auth_token`, `me`.`anonymized`, `me`.`autorenew_checkouts`, `me`.`primary_contact_method` ORDER BY `me`.`surname` ASC, `me`.`firstname` ASC, `me`.`middle_name` ASC, `me`.`othernames` ASC, `me`.`streetnumber` ASC, `me`.`address` ASC, `me`.`address2` ASC, `me`.`city` ASC, `me`.`state` ASC, `me`.`zipcode` ASC, `me`.`country` ASC LIMIT 20) `me` LEFT JOIN `borrower_attributes` `extended_attributes` ON `extended_attributes`.`borrowernumber` = `me`.`borrowernumber` WHERE ( ( ( `me`.`borrowernumber` LIKE '%nil%' OR `me`.`email` LIKE '%nil%' OR `me`.`surname` LIKE '%nil%' OR `me`.`firstname` LIKE '%nil%' OR `me`.`othernames` LIKE '%nil%' OR `me`.`cardnumber` LIKE '%nil%' OR `me`.`userid` LIKE '%nil%' OR `me`.`B_address` LIKE '%nil%' OR `me`.`middle_name` LIKE '%nil%' ) OR ( `me`.`borrowernumber` LIKE '%nil%' OR `me`.`email` LIKE '%nil%' OR `me`.`surname` LIKE '%nil%' OR `me`.`firstname` LIKE '%nil%' OR `me`.`othernames` LIKE '%nil%' OR `me`.`cardnumber` LIKE '%nil%' OR `me`.`userid` LIKE '%nil%' OR `me`.`B_address` LIKE '%nil%' OR `me`.`middle_name` LIKE '%nil%' ) OR ( `extended_attributes`.`attribute` LIKE '%nil%' AND ( `extended_attributes`.`code` = 'BUSS' OR `extended_attributes`.`code` = 'MEROPPET' ) ) ) ) ORDER BY `me`.`surname` ASC, `me`.`firstname` ASC, `me`.`middle_name` ASC, `me`.`othernames` ASC, `me`.`streetnumber` ASC, `me`.`address` ASC, `me`.`address2` ASC, `me`.`city` ASC, `me`.`state` ASC, `me`.`zipcode` ASC, `me`.`country` ASC; This is apparently a known issue with DBIx: https://rt.cpan.org/Public/Bug/Display.html?id=59764 Open since 2010 with priority "low". Do we have a plan to move away from DBIx? Created attachment 159213 [details] [review] Bug 35361: Replace prefetch with join in REST API. Replacing "prefetch" with "join" actually solves this problem. Why are we using "prefetch"? This is what the documentation has to say: "Prefetch does a lot of deep magic. As such, it may not behave exactly as you might expect." On the other hand, the documentation also says: "..., only the prefetch technique will deal sanely with fetching related objects over a has_many relation. " (In reply to Andreas Jonsson from comment #4) > Replacing "prefetch" with "join" actually solves this problem. Why are we > using "prefetch"? This is what the documentation has to say: > > "Prefetch does a lot of deep magic. As such, it may not behave exactly as > you might expect." Tomas, can you maybe help with that question? (In reply to Andreas Jonsson from comment #0) > The search filter for the table on the page acqui/parcel.pl generates the > below api-request, which results in the below sql query. Note how the LIMIT > clause is applied to the sub-query, not the whole query. For us, this > specific query actually yielded a search result of in total over 171 000 > rows. This particular query here does seem hypothetical rather than practical. The size of the query does make it difficult to boil down. It looks like your vendor must have a large number of orders. In theory, that LIMIT 20 should limit the results, but there are a lot of JOINs across different tables, so you could end up with some unexpected results I suppose... (In reply to Andreas Jonsson from comment #2) > Do we have a plan to move away from DBIx? No, Koha is in deep with DBIx. (In reply to Andreas Jonsson from comment #4) > Replacing "prefetch" with "join" actually solves this problem. Why are we > using "prefetch"? This is what the documentation has to say: > > "Prefetch does a lot of deep magic. As such, it may not behave exactly as > you might expect." > > On the other hand, the documentation also says: > > "..., only the prefetch technique will deal sanely with fetching related > objects over a has_many relation. " Koha does some interesting generic handling of search queries that get translated into DBIx queries. It might be that Koha is doing something wrong as well. Also, can you define "crashes the system"? At the moment, I don't know that this one is "critical", because I haven't heard of any systems crashing because of this. Does the server hang because of the large result set? Is a 500 error thrown? It is not hypothetical. If you find an invoice under Acquisitions -> Invoices and then "Go to receipt page" (i.e., acqui/parcel.pl) you will find a table with open orders. If you type anything into the search filter, the above will be the sql-query generated. Since there are LEFT JOIN clauses applied after the limit have been applied, and several of those joins have the potential to multiply the number of result lines that are produced, the number of matched lines can explode. The problem is that all lines are loaded into a perl-process that filters out duplicates. This perl-filtering consumes heaps of memory and takes several minutes to perform. Since it is an automatic search performed by typing in the box there will typically be multiple such search requests issued. This makes the system unresponsive and likely triggers the oom killer. The Libraries in question is using a work flow for their acquisitions where orders are often left opened. Over time there will be many open orders. It starts to become problematic at about 20,000 open orders. Of course, this should not be a problem, because the request should have been limited to return at most 20 rows. DBIx is fundamentally flawed, and I think it would be a good idea to write our own sql-queries and our own code for generating sql queries. We are currently working around the problem by regularly creating new vendor objects and registering new orders on the new vendor object to keep the number of opened orders per vendor limited. (In reply to Andreas Jonsson from comment #8) > It is not hypothetical. If you find an invoice under Acquisitions -> > Invoices and then "Go to receipt page" (i.e., acqui/parcel.pl) you will find > a table with open orders. If you type anything into the search filter, the > above will be the sql-query generated. In retrospect, I think that I was testing something different. I was looking just at the column level search filters. I think you must've been table level search filter. My mistake. > Since there are LEFT JOIN clauses applied after the limit have been applied, > and several of those joins have the potential to multiply the number of > result lines that are produced, the number of matched lines can explode. Agreed. > DBIx is fundamentally flawed, and I think it would be a good idea to write > our own sql-queries and our own code for generating sql queries. Take a look back in the git log maybe 10 years ago ;). The DBIx ORM is a relatively new addition in the life of Koha. It replaced a lot of bespoke SQL queries. I don't think we'll be going backwards. However, for complex cases like this one, maybe we do need the controller to have a more specific handler with custom SQL queries. I can understand why we want to use something like DBIx::Class. But have we properly considered the quality and the viability of DBIx::Class as an open source project? My impression is that it is not a good library: - The query semantics is vague and ambigous - The documentation cannot be trusted - It is slow - It is unmaintainable Although hundreds of people are listed as contributors, judging by the commit history, it is in practice a one-man project. Is there any Koha developer who has read the source code (e.g., https://metacpan.org/release/RIBASUSHI/DBIx-Class-0.082843/source/lib/DBIx/Class/Storage/DBI.pm) and can say that they feel confident about the situation? |