There are two queries without any WHERE which return more than one row, but use just first one (since they don't have order which one will be returned is database dependent). SELECT authorised_value FROM marc_subfield_structure WHERE kohafield="items.notforloan" SELECT authorised_value FROM marc_subfield_structure WHERE kohafield="items.restricted"
Created attachment 8774 [details] [review] Bug 7872 - C4::Items::GetItemsInfo marc_subfield_structure SQL queries fetch all results and use first row. Adding frameworkcode to SQL queries will help returning just one (correct) row, improve performance and allow caching of values in future.
Interesting patch, not only for performances but also functionally. let's assume we have several bibliographic frameworks using different authorized value categories for notforloan field (A librarian told me that functionally it doesn't make sense. But technically it's possible). In this case, some items get the notforloan description from an other authorized value than its framework is configured for. I think it depends on the order rows are returned. So, changing importance to normal bug. And signing off
Created attachment 8904 [details] [review] [Signed-off] Bug 7872 - C4::Items::GetItemsInfo marc_subfield_structure SQL queries fetch all results and use first row. Adding frameworkcode to SQL queries will help returning just one (correct) row, improve performance and allow caching of values in future. Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
QA Comment: Hi Dobrica, Why don't you replace these queries with a call to the C4::Koha::GetAuthValCode function (it does the same query) ? However, isn't it better to use the LIMIT 1 clause if we just want one result ?
Jonathan, great point about C4::Koha::GetAuthValCode. I will rewrite patch and as side-benefit we can memoize it later for bug 7177. LIMIT 1 is not needed since adding framework to WHERE always returns single row and it's more correct.
(In reply to comment #5) > LIMIT 1 is not needed since adding framework to WHERE always returns single > row and it's more correct. Why more correct ? I don't understand. For me, LIMIT 1 should be used if we want only a unique row. I think it can increase performance (I agree with you, it is negligible).
Jonathan, we need to get the right row, not just 1 row. The right one depends on the framework code of the record (see comment 2). But C4::Koha::GetAuthValCode seems to be the best way to do that.
Created attachment 8917 [details] [review] Bug 7872 - C4::Items::GetItemsInfo should use C4::Koha instead of SQL queries directly This simplifies code, and has nice side-effect that memoize of C4::Koha functions will be more effective.
Created attachment 8923 [details] [review] Bug 7872 - C4::Items::GetItemsInfo should use C4::Koha instead of SQL queries directly This simplifies code, and has nice side-effect that memoize of C4::Koha functions will be more effective.
(In reply to comment #6) > (In reply to comment #5) > > > LIMIT 1 is not needed since adding framework to WHERE always returns single > > row and it's more correct. > > Why more correct ? I don't understand. > For me, LIMIT 1 should be used if we want only a unique row. I think it can > increase performance (I agree with you, it is negligible). Actually a where on an indexed column is a lot faster than using limit, explain is a good tool for looking at the query plan to see how mysql is optimising the query. There is a whole project of sql optimisation we could do by checking some of our more used queries.
Created attachment 9993 [details] [review] Bug 7872 - C4::Items::GetItemsInfo should use C4::Koha instead of SQL queries directly This simplifies code, and has nice side-effect that memoize of C4::Koha functions will be more effective. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
QA comment: this patch replaces 30 lines by 3, using an existing function to avoid duplicaing code dobrica++ !
(In reply to comment #10) > (In reply to comment #6) > > (In reply to comment #5) > > > > > LIMIT 1 is not needed since adding framework to WHERE always returns single > > > row and it's more correct. > > > > Why more correct ? I don't understand. > > For me, LIMIT 1 should be used if we want only a unique row. I think it can > > increase performance (I agree with you, it is negligible). > > Actually a where on an indexed column is a lot faster than using limit, > explain is a good tool for looking at the query plan to see how mysql is > optimising the query. There is a whole project of sql optimisation we could > do by checking some of our more used queries. Site comment: investigating DBI, we've found that all handlers are automatically closed when a query has returned all results. If plan not to fetch all results, then you must explicitely close $sth, or you may have some useless memory consumption
There have been no further reports of problems so I am marking this bug resolved.