This is potentially a large list used in staff and opac to show a user's circulation history. The routine itself makes an unnecessary iteration over the list to do an inefficient copy and the consumers of the list also loop over and copy the list before the final loop perdormed in instantiating the template. Move unnecessary processing out of the loops (e.g. format dates using the tt custom plugin dont format dates we are not displaying). Don't increase the performabce hit of the routine
Created attachment 9317 [details] [review] Proposed patch
Created attachment 9536 [details] [review] Bug 8017 reduce manipulation of GetAllIssues return GetAllIssues can produce large lists For performance purposes: Dont loop over the list without cause Dont do expensive processing in the loop Dont needlessly copy the array Do display formatting in the template Dont extract the barcode list unless we are producing it Reduce db calls by using the data to hand Make the table in the template a bit more readable where everything was stuffed into one line Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
QA Comments: 1/ You remove the FIXME in getAllIssues (sanity-check order and limit) but $order is not sanitized. Please call $dbh->quote with the $order variable. 2/ Why do you remove the parenthesis for the IF statement (template) ? It seems mandatory by the koha guidelines. But I agree with you, it is useless :) Failed QA for 1. Replacement of fetchrow_hashref with a more adapted fetchall_arrayref is good. I think there is others occurrences of this into the koha code.
Created attachment 10192 [details] [review] Proposed Amended Patch Patch rebased against master. Have added sanitizing of order and limit parameters (although order is currently always passed a constant string from the caller) Note on issue 2 adding parens around the variable forces the tt parser to evaluate as an expression. i.e. potentially adding processing where a simple variable is used. While it was used defensively in converting templates I'd suggest following normal tt style in new code
This patch seems already signed off. But it does not apply. Could you resubmit a rebased patch please ?
Created attachment 11330 [details] [review] Patch rebased against current head Rebase of patch against HEAD
Thanks Colin ! I did not find any regression. So I mark it as Passed QA.
I realize that this patch modify a SELECT * that is LARGE as it's a JOIN on issues / biblio / items / biblioitems I agree it was like this before the patch. But Colin, could you improve it to return only needed columns, because that's probably also a source of the perfs problem. Side note : there is a call to +my $branches = GetBranches(); +foreach my $issue ( @{$issues} ) { + $issue->{issuingbranch} = $branches->{ $issue->{branchcode} }->{branchname}; } That could be replaced by another JOIN (not sure it's a good idea, just sharing) (not failing QA, the mistake was here before your patch. If you can't provide a follow-up, i'll push like this. But if you agree for a follow-up, it will be cool ;-) )
I'll take a look at Paul's suggestions The main performance hit was the unnecessary repetitions of loops and in production the patch as is does enough to fix large sites that were having problems displaying circ history.
Colin = pinging my comment 8
Paul, Id be happy to see this pushed as is. I think I'll look at what can be done to improve the query as a separate piece of work
Sorry, but the patch does not apply anymore, because of bug 6494. Could you rebase & resubmit (conflict not trivial to solve: I feel most of it is tidying code, but to avoid any mistake, I let you do)
Created attachment 12177 [details] [review] Patch rebased against current head Rebased to incorporate changes introduced by the opachtmlsummary code
switching back to passed QA
Patch pushed to master. Could be ported to 3.8 imo
I would like this in 3.8.x but unfortunately does not apply cleanly # Changes to be committed: # # modified: C4/Members.pm # modified: koha-tmpl/intranet-tmpl/prog/en/modules/members/readingrec.tt # modified: members/readingrec.pl # # Unmerged paths: # (use "git add/rm <file>..." as appropriate to mark resolution) # # both modified: koha-tmpl/opac-tmpl/prog/en/modules/opac-readingrecord.tt # both modified: opac/opac-readingrecord.pl
Created attachment 12207 [details] [review] Patch - Version for 3.8 This is a version of patch that should apply to 3.8 - The last rebase was to account for some new functionality in master. This omits updating that non existent code
Patch contains no string changes, makes some template changes which I think are safe, pushed to 3.8.x, will be in 3.8.5