Bug 7872

Summary: C4::Items should use C4::Koha methods instead of duplicating SQL queries
Product: Koha Reporter: Dobrica Pavlinusic <dpavlin>
Component: MARC Authority data supportAssignee: Dobrica Pavlinusic <dpavlin>
Status: CLOSED FIXED QA Contact: Ian Walls <koha.sekjal>
Severity: normal    
Priority: P5 - low CC: alex.arnaud, chris, jonathan.druart, paul.poulain
Version: 3.10   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 7119, 7172, 7177    
Attachments: Bug 7872 - C4::Items::GetItemsInfo marc_subfield_structure
[Signed-off] Bug 7872 - C4::Items::GetItemsInfo marc_subfield_structure
Bug 7872 - C4::Items::GetItemsInfo should use C4::Koha instead of SQL queries directly
Bug 7872 - C4::Items::GetItemsInfo should use C4::Koha instead of SQL queries directly
Bug 7872 - C4::Items::GetItemsInfo should use C4::Koha instead of SQL queries directly

Description Dobrica Pavlinusic 2012-04-02 13:57:17 UTC
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"
Comment 1 Dobrica Pavlinusic 2012-04-02 14:05:51 UTC Comment hidden (obsolete)
Comment 2 Alex Arnaud 2012-04-05 08:14:43 UTC
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
Comment 3 Alex Arnaud 2012-04-05 08:25:07 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2012-04-05 12:19:50 UTC
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 ?
Comment 5 Dobrica Pavlinusic 2012-04-05 13:07:37 UTC
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.
Comment 6 Jonathan Druart 2012-04-05 13:24:02 UTC
(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).
Comment 7 Alex Arnaud 2012-04-05 13:38:30 UTC
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.
Comment 8 Dobrica Pavlinusic 2012-04-05 13:48:18 UTC Comment hidden (obsolete)
Comment 9 Dobrica Pavlinusic 2012-04-05 14:15:08 UTC Comment hidden (obsolete)
Comment 10 Chris Cormack 2012-04-05 20:29:39 UTC
(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.
Comment 11 Chris Cormack 2012-06-09 09:50:44 UTC
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>
Comment 12 Paul Poulain 2012-06-10 09:07:41 UTC
QA comment: this patch replaces 30 lines by 3, using an existing function to avoid duplicaing code

dobrica++ !
Comment 13 Paul Poulain 2012-06-10 09:11:37 UTC
(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
Comment 14 Jared Camins-Esakov 2012-12-31 01:04:52 UTC
There have been no further reports of problems so I am marking this bug resolved.