C4::Reserves::CanBookBeReserved uses GetItemsInfo($biblionumber) to get the itemnumbers for all the items attached to the biblio in question. However, GetItemsInfo is a very heavy-weight subroutine, pulling in data from all over the database. This data includes a run of C4::Reserves::CheckReserves(), as well as pulling in the last three borrowers from old_issues. Using get_itemnumbers_of(($biblionumber)) would greatly reduce the cost of CanBookBeReserved.
Created attachment 5861 [details] [review] Proposed Patch Replaces CanBookBeReserved()'s call to GetItemsInfo with a call to get_itemnumbers_of, to save on system resources.
nengard@debian:~/kohaclone$ git bz apply 7016 Bug 7016 - CanBookBeReserved uses GetItemsInfo unnecessarily Proposed Patch Apply? [yn] y Applying: Bug 7016: CanBookBeReserved uses GetItemsInfo unnecessarily error: patch failed: C4/Reserves.pm:382 error: C4/Reserves.pm: patch does not apply Patch failed at 0001 Bug 7016: CanBookBeReserved uses GetItemsInfo unnecessarily When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". Patch left in /tmp/Proposed-Patch-Jv873P.patch nengard@debian:~/kohaclone$
Created attachment 5998 [details] [review] Proposed patch (rebased for analytics code) With the addition of Host Items, having a more efficient subroutine here is even more important. Thankfully, a get_hostitemnumbers_of subroutine was provided in parallel to the get_itemnumbers_of, so no compromises needed to be made.
Created attachment 5999 [details] [review] [SIGNED-OFF] Bug 7016: CanBookBeReserved uses GetItemsInfo unnecessarily CanBookBeReserved uses the very heavy-weight function GetItemsInfo to simply retrieve the itemnumbers attached to a given biblio. The function get_itemnumbers_of() is much lighter-weight and returns the required information; switching to it will reduce the overall system resource cost of placing a hold. Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>
Marking Passed QA to catch RM's attention ;). Chris, if you also think this is a sound change, please push, otherwise it can wait.
I have no issues with this, works and is a lot lighter, pushing
This patch broke the function. CanItemBeReserved needs an itemnumber and it seems that it now gets an array of hash (for exemple you would get itemnumber with $item->{$biblionumber}->[0]). I would suggest either reverting this patch or using/adding subs that just return an array of itemnumber correctly.
Created attachment 7071 [details] [review] Proposed Followup Adds a new subroutine to just get an array of itemnumbers for any given biblionumber, and replaces the use of get_itemnumbers_of in CanBookBeReserved.
Followup patch needs signoff still, I believe.
Created attachment 9127 [details] [review] Bug 7016 Followup: Add new GetItemnumberForBiblio subroutine Adds a new subroutine in C4::Items, GetItemnumbersForBiblio, which takes a single biblionumber, and returns an array of all the corresponding itemnumbers. This patch also replaces the usage of get_itemnumbers_of in C4::Reserves::CanBookBeReserved with this new subroutine, as the output is more consistent with what we were lookng for (this is what fixes the bug issue). Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Created attachment 9128 [details] [review] bug 7016 further followup: clarify return of GetItemnumbersForBiblio New function was actually returning an arrayref, so made perldoc and function usage consistent. Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Comment on attachment 7071 [details] [review] Proposed Followup obsoleted by signed-off attachments
Now signed off - note that it's a two patch series: Bug 7016 Followup: Add new GetItemnumberForBiblio subroutine bug 7016 further followup: clarify return of GetItemnumbersForBiblio
Marking as blocker for 3.8; without the two follow-up patches, holdability checking is effectively broken in 3.6 and 3.8.
QA Comment: Two followups look good to me. Marking as Passed QA. No translation issues involved.
comment 7 describe the problem, however i'm surprised that noone has spotted it before. I've pushed the patch, but could not see the bugguy behaviour and the fixed behaviour. Maybe that's because of my setup, or a missing test plan. Anyway, patch pushed
Hmm have you not merged yet? Maybe could you not set pushed to Master until its in master ... it confuses me everytime, maybe just set that when you merge and push to master?