We have a partner processing thousands of holds per day, per branch. At this number of holds, the script loads very slow if it manages to load at all. The primary slowdown is the individual fetch of the patron for each hold to be displayed. If we prefetch those patrons, the entire script should load much faster.
Created attachment 124626 [details] [review] Bug 28966: Prefetch patron data for holds queue viewer We have a partner processing thousands of holds per day, per branch. At this number of holds, the script loads very slow if it manages to load at all. The primary slowdown is the individual fetch of the patron for each hold to be displayed. If we prefetch those patrons, the entire script should load much faster. Test Plan: 1) Create as many holds as you can ( up to a few thousand if you can ) 2) Run the holds queue viewer, note the load time 3) Apply this patch 4) Restart all the things! 5) Reload the holds queue viewer, load time should be improved 6) Note the HTML generated looks the same
Created attachment 124627 [details] [review] Bug 28966: Prefetch patron data for holds queue viewer We have a partner processing thousands of holds per day, per branch. At this number of holds, the script loads very slow if it manages to load at all. The primary slowdown is the individual fetch of the patron for each hold to be displayed. If we prefetch those patrons, the entire script should load much faster. Test Plan: 1) Create as many holds as you can ( up to a few thousand if you can ) 2) Run the holds queue viewer, note the load time 3) Apply this patch 4) Restart all the things! 5) Reload the holds queue viewer, load time should be improved 6) Note the HTML generated looks the same
Created attachment 124628 [details] [review] Bug 28966: Prefetch patron data for holds queue viewer We have a partner processing thousands of holds per day, per branch. At this number of holds, the script loads very slow if it manages to load at all. The primary slowdown is the individual fetch of the patron for each hold to be displayed. If we prefetch those patrons, the entire script should load much faster. Test Plan: 1) Create as many holds as you can ( up to a few thousand if you can ) 2) Run the holds queue viewer, note the load time 3) Apply this patch 4) Restart all the things! 5) Reload the holds queue viewer, load time should be improved 6) Note the HTML generated looks the same
Created attachment 124629 [details] [review] Bug 28966: Prefetch patron data for holds queue viewer We have a partner processing thousands of holds per day, per branch. At this number of holds, the script loads very slow if it manages to load at all. The primary slowdown is the individual fetch of the patron for each hold to be displayed. If we prefetch those patrons, the entire script should load much faster. Test Plan: 1) Create as many holds as you can ( up to a few thousand if you can ) 2) Run the holds queue viewer, note the load time 3) Apply this patch 4) Restart all the things! 5) Reload the holds queue viewer, load time should be improved 6) Note the HTML generated looks the same
Created attachment 124639 [details] [review] Bug 28966: Prefetch patron data for holds queue viewer We have a partner processing thousands of holds per day, per branch. At this number of holds, the script loads very slow if it manages to load at all. The primary slowdown is the individual fetch of the patron for each hold to be displayed. If we prefetch those patrons, the entire script should load much faster. Test Plan: 1) Create as many holds as you can ( up to a few thousand if you can ) 2) Run the holds queue viewer, note the load time 3) Apply this patch 4) Restart all the things! 5) Reload the holds queue viewer, load time should be improved 6) Note the HTML generated looks the same
Created attachment 124652 [details] [review] Bug 28966: Prefetch patron data for holds queue viewer We have a partner processing thousands of holds per day, per branch. At this number of holds, the script loads very slow if it manages to load at all. The primary slowdown is the individual fetch of the patron for each hold to be displayed. If we prefetch those patrons, the entire script should load much faster. Test Plan: 1) Create as many holds as you can ( up to a few thousand if you can ) 2) Run the holds queue viewer, note the load time 3) Apply this patch 4) Restart all the things! 5) Reload the holds queue viewer, load time should be improved 6) Note the HTML generated looks the same
Created attachment 124854 [details] [review] Bug 28966: Prefetch patron data for holds queue viewer We have a partner processing thousands of holds per day, per branch. At this number of holds, the script loads very slow if it manages to load at all. The primary slowdown is the individual fetch of the patron for each hold to be displayed. If we prefetch those patrons, the entire script should load much faster. Test Plan: 1) Create as many holds as you can ( up to a few thousand if you can ) 2) Run the holds queue viewer, note the load time 3) Apply this patch 4) Restart all the things! 5) Reload the holds queue viewer, load time should be improved 6) Note the HTML generated looks the same Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
hmm, this disables the hide_patron_infos_if_needed variable which is probably not wanted, though the feature is quite broken anyways but this makes it more broken.
Created attachment 125377 [details] [review] Bug 28966: Prefetch patron data for holds queue viewer We have a partner processing thousands of holds per day, per branch. At this number of holds, the script loads very slow if it manages to load at all. The primary slowdown is the individual fetch of the patron for each hold to be displayed. If we prefetch those patrons, the entire script should load much faster. Test Plan: 1) Create as many holds as you can ( up to a few thousand if you can ) 2) Run the holds queue viewer, note the load time 3) Apply this patch 4) Restart all the things! 5) Reload the holds queue viewer, load time should be improved 6) Note the HTML generated looks the same Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
(In reply to Joonas Kylmälä from comment #8) > hmm, this disables the hide_patron_infos_if_needed variable which is > probably not wanted, though the feature is quite broken anyways but this > makes it more broken. Just to make it clear and try to get this moving here, I'm holding off from signing-off on this until we have some clarity to this.
This is going backward IMO, why not move to DBIC/Koha::Object instead?
(In reply to Jonathan Druart from comment #11) > This is going backward IMO, why not move to DBIC/Koha::Object instead? Indeed we should be go back to direct SQL even if faster. Se should use DBIC for all tables, with prefetch. But we could start with adding pagination Bug 28974 while working on rewriting to use DBIC.
(In reply to Fridolin Somers from comment #12) > (In reply to Jonathan Druart from comment #11) > > This is going backward IMO, why not move to DBIC/Koha::Object instead? > > Indeed we should be go back to direct SQL even if faster. Is there a "not" missing here?
Oupsy : > Indeed we should be go back to direct SQL even if faster. Indeed we should not go back to direct SQL even if faster.
I gather from the discussion that this should be changed to use DBIC/Koha::Object instead of SQL. Moving to Failed QA for now to trigger action.
Created attachment 143583 [details] [review] Bug 28966: Reimplement query using objects Test Plan: 1) Generate the holds queue 2) Load the holds queue viewer page 3) Apply this patch 4) Restart all the things! 5) Reload the page 6) Note nothing has changed
Created attachment 143584 [details] [review] Bug 28966: Prefetch patron data for holds queue viewer Test Plan: 1) Generate the holds queue 2) Load the holds queue viewer page 3) Apply this patch 4) Restart all the things! 5) Reload the page 6) Note nothing has changed
Tried to apply patches on current master and received following error: Applying: Bug 28966: Prefetch patron data for holds queue viewer error: sha1 information is lacking or useless (C4/HoldsQueue.pm). error: could not build fake ancestor Patch failed at 0001 Bug 28966: Prefetch patron data for holds queue viewer hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-28966-Prefetch-patron-data-for-holds-queue-vie-FQNEIE.patch
Created attachment 143857 [details] [review] Bug 28966: Add Koha::Object(s) for tmp_holdsqueue
Created attachment 143859 [details] [review] Bug 28966: Add Koha::Object(s) for tmp_holdsqueue
Created attachment 143860 [details] [review] Bug 28966: Prefetch patron data for holds queue viewer Test Plan: 1) Generate the holds queue 2) Load the holds queue viewer page 3) Apply this patch 4) Restart all the things! 5) Reload the page 6) Note nothing has changed
This patch breaks the patch for bug 29760 since it removes the patron from the itemloop.
(In reply to Andreas Jonsson from comment #22) > This patch breaks the patch for bug 29760 since it removes the patron from > the itemloop. I'm still seeing the patron category displayed in the patron category column on the holds queue viewer after applying this patch set. Am I looking in the wrong place?
The column "Patron category" on the page /cgi-bin/koha/circ/view_holdsqueue.pl will contain empty parentheses. This patch removes the patron attribute from each item: - for my $item ( @$items ) { - $item->{patron} = Koha::Patrons->find( $item->{borrowernumber} ); - } But the displaying of category uses this attribute: <td class="hq-patroncategory">[% itemsloo.patron.category.description | html %] ([% itemsloo.patron.categorycode | html %])</td> Also, the phone number should be affected: [% UNLESS Koha.Preference('HidePatronName') %] <p>[% itemsloo.patron.phone | html %]</p> [% END %]
Marking "Failed QA" based on comment 24
Can you provide a screenshot? Did you test the patch? It's working fine for me! (In reply to Andreas Jonsson from comment #24) > The column "Patron category" on the page > /cgi-bin/koha/circ/view_holdsqueue.pl > will contain empty parentheses. > > This patch removes the patron attribute from each item: > > - for my $item ( @$items ) { > - $item->{patron} = Koha::Patrons->find( $item->{borrowernumber} ); > - } > > But the displaying of category uses this attribute: > > <td class="hq-patroncategory">[% > itemsloo.patron.category.description | html %] ([% > itemsloo.patron.categorycode | html %])</td> > > Also, the phone number should be affected: > > [% UNLESS Koha.Preference('HidePatronName') %] > <p>[% itemsloo.patron.phone | html %]</p> > [% END %]
Created attachment 145395 [details] [review] Bug 28966: Prefetch patron data for holds queue viewer Test Plan: 1) Generate the holds queue 2) Load the holds queue viewer page 3) Apply this patch 4) Restart all the things! 5) Reload the page 6) Note nothing has changed
During my testing I noticed the item's biblioitem's size wasn't displaying correctly. This new edition should fix that!
Created attachment 151733 [details] [review] Bug 28966: Add Koha::Object(s) for tmp_holdsqueue Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 151734 [details] [review] Bug 28966: Prefetch patron data for holds queue viewer Test Plan: 1) Generate the holds queue 2) Load the holds queue viewer page 3) Apply this patch 4) Restart all the things! 5) Reload the page 6) Note nothing has changed Signed-off-by: Owen Leonard <oleonard@myacpl.org>
+Koha::Hold::HoldsQueueItem - Koha hold cancellation request Object class Copy/paste ?
(In reply to Kyle M Hall from comment #0) > We have a partner processing thousands of holds per day, per branch. At this > number of holds, the script loads very slow if it manages to load at all. > The primary slowdown is the individual fetch of the patron for each hold to > be displayed. If we prefetch those patrons, the entire script should load > much faster. Could you provide some figures that we are faster now ?
join => [ 'borrower', # 'biblio', # 'biblioitem', # { # 'item' => { # 'item_group_item' => 'item_group' # } # }, ], prefetch => [ 'biblio', 'biblioitem', { 'item' => { 'item_group_item' => 'item_group' } } ], Do we need to both join and prefetch here ?
Changing status for feedback.
Created attachment 153120 [details] [review] Bug 28966: (QA follow-up) Changes for Koha objects Trivial edits.
(In reply to Marcel de Rooy from comment #33) > join => [ > 'borrower', > # 'biblio', > # 'biblioitem', > # { > # 'item' => { > # 'item_group_item' => 'item_group' > # } > # }, > ], > prefetch => [ > 'biblio', > 'biblioitem', > { > 'item' => { > 'item_group_item' => 'item_group' > } > } > ], > > Do we need to both join and prefetch here ? The DBIC docs indicate we can drop the join as the prefetch implies it. The syntax of 'prefetch' is the same as 'join' and implies the joining, so there is no need to use both together. I'll add a followup!
Created attachment 153875 [details] [review] Bug 28966: Add Koha::Object(s) for tmp_holdsqueue Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 153876 [details] [review] Bug 28966: Prefetch patron data for holds queue viewer Test Plan: 1) Generate the holds queue 2) Load the holds queue viewer page 3) Apply this patch 4) Restart all the things! 5) Reload the page 6) Note nothing has changed Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 153877 [details] [review] Bug 28966: (QA follow-up) Changes for Koha objects Trivial edits.
Created attachment 153878 [details] [review] Bug 28966: (QA follow-up) Remove superfluous joins
Created attachment 153879 [details] [review] Bug 28966: Add Koha::Object(s) for tmp_holdsqueue Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153880 [details] [review] Bug 28966: Prefetch patron data for holds queue viewer Test Plan: 1) Generate the holds queue 2) Load the holds queue viewer page 3) Apply this patch 4) Restart all the things! 5) Reload the page 6) Note nothing has changed Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153881 [details] [review] Bug 28966: (QA follow-up) Changes for Koha objects Trivial edits. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153882 [details] [review] Bug 28966: (QA follow-up) Remove superfluous joins Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Pushed to master for 23.11. Nice work everyone, thanks!
Created attachment 153928 [details] [review] Bug 28966: (QA Follow-up) Fix failing unit tests Test Plan: 1) prove t/db_dependent/TestBuilder.t 2) Note tests fail 3) Apply this patch 4) Run updatedatabase.pl 5) Update the schema files ( alias 'dbic' can be used in koha-testing-docker ) 6) prove t/db_dependent/TestBuilder.t 7) Tests now pass!
Created attachment 153929 [details] [review] Bug 28966: (QA Follow-up) Fix failing unit tests Test Plan: 1) prove t/db_dependent/TestBuilder.t 2) Note tests fail 3) Apply this patch 4) Run updatedatabase.pl 5) Update the schema files ( alias 'dbic' can be used in koha-testing-docker ) 6) prove t/db_dependent/TestBuilder.t 7) Tests now pass!
Thanks Kyle, for promptly fixing the issue.
Created attachment 153939 [details] [review] Bug 28966: (QA follow-up) tmp_holdsqueue no longer an iterator Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Kyle M Hall from comment #47) > Created attachment 153929 [details] [review] [review] > Bug 28966: (QA Follow-up) Fix failing unit tests > > Test Plan: > 1) prove t/db_dependent/TestBuilder.t > 2) Note tests fail > 3) Apply this patch > 4) Run updatedatabase.pl > 5) Update the schema files ( alias 'dbic' can be used in > koha-testing-docker ) > 6) prove t/db_dependent/TestBuilder.t > 7) Tests now pass! This is breaking Koha_Master_My8 12:06:40 koha_1 | DBD::mysql::st execute failed: All parts of a PRIMARY KEY must be NOT NULL; if you need NULL in a key, use UNIQUE instead at /usr/share/perl5/DBIx/RunSQL.pm line 279, <$args{...}> line 1. 12:06:40 koha_1 | Something went wrong loading file /kohadevbox/koha/installer/data/mysql/kohastructure.sql ([SQL ERROR]: CREATE TABLE `tmp_holdsqueue` ( 12:06:40 koha_1 | `biblionumber` int(11) DEFAULT NULL, 12:06:40 koha_1 | `itemnumber` int(11) DEFAULT NULL, 12:06:40 koha_1 | `barcode` varchar(20) DEFAULT NULL, 12:06:40 koha_1 | `surname` longtext NOT NULL, 12:06:40 koha_1 | `firstname` mediumtext DEFAULT NULL, 12:06:40 koha_1 | `phone` mediumtext DEFAULT NULL, 12:06:40 koha_1 | `borrowernumber` int(11) NOT NULL, 12:06:40 koha_1 | `cardnumber` varchar(32) DEFAULT NULL, 12:06:40 koha_1 | `reservedate` date DEFAULT NULL, 12:06:40 koha_1 | `title` longtext DEFAULT NULL, 12:06:40 koha_1 | `itemcallnumber` varchar(255) DEFAULT NULL, 12:06:40 koha_1 | `holdingbranch` varchar(10) DEFAULT NULL, 12:06:40 koha_1 | `pickbranch` varchar(10) DEFAULT NULL, 12:06:40 koha_1 | `notes` mediumtext DEFAULT NULL, 12:06:40 koha_1 | `item_level_request` tinyint(4) NOT NULL DEFAULT 0, 12:06:40 koha_1 | `timestamp` timestamp NOT NULL DEFAULT current_timestamp() ON UPDATE current_timestamp() COMMENT 'date and time this entry as added/last updated', 12:06:40 koha_1 | PRIMARY KEY (`itemnumber`), 12:06:40 koha_1 | KEY `tmp_holdsqueue_ibfk_1` (`itemnumber`), 12:06:40 koha_1 | KEY `tmp_holdsqueue_ibfk_2` (`biblionumber`), 12:06:40 koha_1 | KEY `tmp_holdsqueue_ibfk_3` (`borrowernumber`), 12:06:40 koha_1 | CONSTRAINT `tmp_holdsqueue_ibfk_1` FOREIGN KEY (`itemnumber`) REFERENCES `items` (`itemnumber`) ON DELETE CASCADE ON UPDATE CASCADE, 12:06:40 koha_1 | CONSTRAINT `tmp_holdsqueue_ibfk_2` FOREIGN KEY (`biblionumber`) REFERENCES `biblio` (`biblionumber`) ON DELETE CASCADE ON UPDATE CASCADE, 12:06:40 koha_1 | CONSTRAINT `tmp_holdsqueue_ibfk_3` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE 12:06:40 koha_1 | ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci 12:06:40 koha_1 | ) at /kohadevbox/koha/C4/Installer.pm line 585. https://jenkins.koha-community.org/job/Koha_Master_My8/1236/console
Thanks Kyle for taking care on the separate bug. Please QA y'all.
Upgrade from Koha 23.05 to 23.11 and get this error Upgrade to 23.06.00.013 [01:38:28]: Bug 28966 - Holds queue view too slow to load for large numbers of holds ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Cannot change column 'itemnumber': used in a foreign key constraint 'tmp_holdsqueue_ibfk_1' at /usr/share/koha/lib/C4/Installer.pm line 741
(In reply to Mohd Hafiz Yusoff from comment #52) > Upgrade from Koha 23.05 to 23.11 and get this error > > Upgrade to 23.06.00.013 [01:38:28]: Bug 28966 - Holds queue view too slow > to load for large numbers of holds > ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Cannot change > column 'itemnumber': used in a foreign key constraint > 'tmp_holdsqueue_ibfk_1' at /usr/share/koha/lib/C4/Installer.pm line 741 Also reported on the French mailing list: https://groupes.renater.fr/sympa/arc/koha.infos/2024-01/msg00000.html
(In reply to Jonathan Druart from comment #53) > (In reply to Mohd Hafiz Yusoff from comment #52) > > Upgrade from Koha 23.05 to 23.11 and get this error > > > > Upgrade to 23.06.00.013 [01:38:28]: Bug 28966 - Holds queue view too slow > > to load for large numbers of holds > > ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Cannot change > > column 'itemnumber': used in a foreign key constraint > > 'tmp_holdsqueue_ibfk_1' at /usr/share/koha/lib/C4/Installer.pm line 741 > > Also reported on the French mailing list: > https://groupes.renater.fr/sympa/arc/koha.infos/2024-01/msg00000.html I wonder if this is mysql or mariadb version specific. Not getting that error on 10.5.22-MariaDB
(In reply to Kyle M Hall from comment #54) > (In reply to Jonathan Druart from comment #53) > > (In reply to Mohd Hafiz Yusoff from comment #52) > > > Upgrade from Koha 23.05 to 23.11 and get this error > > > > > > Upgrade to 23.06.00.013 [01:38:28]: Bug 28966 - Holds queue view too slow > > > to load for large numbers of holds > > > ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Cannot change > > > column 'itemnumber': used in a foreign key constraint > > > 'tmp_holdsqueue_ibfk_1' at /usr/share/koha/lib/C4/Installer.pm line 741 > > > > Also reported on the French mailing list: > > https://groupes.renater.fr/sympa/arc/koha.infos/2024-01/msg00000.html > > I wonder if this is mysql or mariadb version specific. Not getting that > error on 10.5.22-MariaDB Filed a new bug report at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35687