Bug 28966

Summary: Holds queue viewer too slow to load for large numbers of holds
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Hold requestsAssignee: Kyle M Hall <kyle>
Status: RESOLVED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: minor    
Priority: P5 - low CC: andreas.jonsson, dcook, emmi.takkinen, fridolin.somers, gmcharlt, jonathan.druart, joonas.kylmala, m.de.rooy, mhby87, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28972
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33184
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00
Bug Depends on: 29015    
Bug Blocks: 28974, 34494, 35687    
Attachments: Bug 28966: Prefetch patron data for holds queue viewer
Bug 28966: Prefetch patron data for holds queue viewer
Bug 28966: Prefetch patron data for holds queue viewer
Bug 28966: Prefetch patron data for holds queue viewer
Bug 28966: Prefetch patron data for holds queue viewer
Bug 28966: Prefetch patron data for holds queue viewer
Bug 28966: Prefetch patron data for holds queue viewer
Bug 28966: Prefetch patron data for holds queue viewer
Bug 28966: Reimplement query using objects
Bug 28966: Prefetch patron data for holds queue viewer
Bug 28966: Add Koha::Object(s) for tmp_holdsqueue
Bug 28966: Add Koha::Object(s) for tmp_holdsqueue
Bug 28966: Prefetch patron data for holds queue viewer
Bug 28966: Prefetch patron data for holds queue viewer
Bug 28966: Add Koha::Object(s) for tmp_holdsqueue
Bug 28966: Prefetch patron data for holds queue viewer
Bug 28966: (QA follow-up) Changes for Koha objects
Bug 28966: Add Koha::Object(s) for tmp_holdsqueue
Bug 28966: Prefetch patron data for holds queue viewer
Bug 28966: (QA follow-up) Changes for Koha objects
Bug 28966: (QA follow-up) Remove superfluous joins
Bug 28966: Add Koha::Object(s) for tmp_holdsqueue
Bug 28966: Prefetch patron data for holds queue viewer
Bug 28966: (QA follow-up) Changes for Koha objects
Bug 28966: (QA follow-up) Remove superfluous joins
Bug 28966: (QA Follow-up) Fix failing unit tests
Bug 28966: (QA Follow-up) Fix failing unit tests
Bug 28966: (QA follow-up) tmp_holdsqueue no longer an iterator

Description Kyle M Hall 2021-09-07 16:54:50 UTC
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.
Comment 1 Kyle M Hall 2021-09-07 16:58:01 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2021-09-07 17:24:54 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2021-09-07 17:47:14 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2021-09-07 18:08:24 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2021-09-08 11:06:07 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2021-09-08 12:17:36 UTC Comment hidden (obsolete)
Comment 7 Emmi Takkinen 2021-09-14 07:53:48 UTC Comment hidden (obsolete)
Comment 8 Joonas Kylmälä 2021-09-19 11:27:26 UTC
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.
Comment 9 Kyle M Hall 2021-09-27 19:04:31 UTC Comment hidden (obsolete)
Comment 10 Joonas Kylmälä 2021-10-03 08:39:35 UTC
(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.
Comment 11 Jonathan Druart 2021-12-02 09:30:18 UTC
This is going backward IMO, why not move to DBIC/Koha::Object instead?
Comment 12 Fridolin Somers 2022-01-31 23:54:13 UTC
(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.
Comment 13 Jonathan Druart 2022-02-01 08:35:34 UTC
(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?
Comment 14 Fridolin Somers 2022-02-02 06:03:54 UTC
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.
Comment 15 Katrin Fischer 2022-02-13 13:39:56 UTC
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.
Comment 16 Kyle M Hall 2022-11-09 19:11:15 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2022-11-09 19:12:13 UTC Comment hidden (obsolete)
Comment 18 Emmi Takkinen 2022-11-11 06:47:01 UTC
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
Comment 19 Kyle M Hall 2022-11-14 18:27:07 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2022-11-14 18:28:52 UTC
Created attachment 143859 [details] [review]
Bug 28966: Add Koha::Object(s) for tmp_holdsqueue
Comment 21 Kyle M Hall 2022-11-14 18:29:04 UTC
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
Comment 22 Andreas Jonsson 2022-12-21 15:58:00 UTC
This patch breaks the patch for bug 29760 since it removes the patron from the itemloop.
Comment 23 Kyle M Hall 2022-12-21 16:06:01 UTC
(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?
Comment 24 Andreas Jonsson 2022-12-21 17:02:40 UTC
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 %]
Comment 25 Owen Leonard 2023-01-18 12:41:20 UTC
Marking "Failed QA" based on comment 24
Comment 26 Kyle M Hall 2023-01-18 14:58:19 UTC
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 %]
Comment 27 Kyle M Hall 2023-01-18 14:59:27 UTC
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
Comment 28 Kyle M Hall 2023-01-18 15:00:28 UTC
During my testing I noticed the item's biblioitem's size wasn't displaying correctly. This new edition should fix that!
Comment 29 Owen Leonard 2023-05-26 12:13:21 UTC
Created attachment 151733 [details] [review]
Bug 28966: Add Koha::Object(s) for tmp_holdsqueue

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 30 Owen Leonard 2023-05-26 12:13:24 UTC
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>
Comment 31 Marcel de Rooy 2023-07-07 07:27:32 UTC
+Koha::Hold::HoldsQueueItem - Koha hold cancellation request Object class

Copy/paste ?
Comment 32 Marcel de Rooy 2023-07-07 07:38:29 UTC
(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 ?
Comment 33 Marcel de Rooy 2023-07-07 07:44:02 UTC
            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 ?
Comment 34 Marcel de Rooy 2023-07-07 07:44:27 UTC
Changing status for feedback.
Comment 35 Marcel de Rooy 2023-07-07 07:44:55 UTC
Created attachment 153120 [details] [review]
Bug 28966: (QA follow-up) Changes for Koha objects

Trivial edits.
Comment 36 Kyle M Hall 2023-07-25 13:28:41 UTC
(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!
Comment 37 Kyle M Hall 2023-07-25 13:44:14 UTC
Created attachment 153875 [details] [review]
Bug 28966: Add Koha::Object(s) for tmp_holdsqueue

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 38 Kyle M Hall 2023-07-25 13:44:20 UTC
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>
Comment 39 Kyle M Hall 2023-07-25 13:44:23 UTC
Created attachment 153877 [details] [review]
Bug 28966: (QA follow-up) Changes for Koha objects

Trivial edits.
Comment 40 Kyle M Hall 2023-07-25 13:44:25 UTC
Created attachment 153878 [details] [review]
Bug 28966: (QA follow-up) Remove superfluous joins
Comment 41 Marcel de Rooy 2023-07-25 14:06:42 UTC
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>
Comment 42 Marcel de Rooy 2023-07-25 14:06:45 UTC
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>
Comment 43 Marcel de Rooy 2023-07-25 14:06:48 UTC
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>
Comment 44 Marcel de Rooy 2023-07-25 14:06:51 UTC
Created attachment 153882 [details] [review]
Bug 28966: (QA follow-up) Remove superfluous joins

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 45 Tomás Cohen Arazi 2023-07-25 19:26:18 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 46 Kyle M Hall 2023-07-26 15:56:14 UTC
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!
Comment 47 Kyle M Hall 2023-07-26 15:57:47 UTC
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!
Comment 48 Tomás Cohen Arazi 2023-07-26 18:43:16 UTC
Thanks Kyle, for promptly fixing the issue.
Comment 49 Tomás Cohen Arazi 2023-07-26 20:48:46 UTC
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>
Comment 50 Jonathan Druart 2023-08-03 10:21:28 UTC
(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
Comment 51 Tomás Cohen Arazi 2023-08-08 12:56:59 UTC
Thanks Kyle for taking care on the separate bug.

Please QA y'all.
Comment 52 Mohd Hafiz Yusoff 2023-12-31 17:44:53 UTC
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
Comment 53 Jonathan Druart 2024-01-03 13:46:42 UTC
(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
Comment 54 Kyle M Hall 2024-01-03 13:57:51 UTC
(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
Comment 55 Kyle M Hall 2024-01-03 14:09:53 UTC
(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