Bug 24488

Summary: Holds to Pull sometimes shows the wrong 'first patron' details
Product: Koha Reporter: Aleisha Amohia <aleisha>
Component: CirculationAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: normal    
Priority: P5 - low CC: black23, caroline.cyr-la-rose, fridolin.somers, gmcharlt, jonathan.druart, joonas.kylmala, josef.moravec, katrin.fischer, kelly, kyle.m.hall, lucy.vaux-harvey, m.de.rooy, martin.renvoize, nick
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22431
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23360
Change sponsored?: Sponsored Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00
Bug Depends on: 24301    
Bug Blocks: 27131, 27706, 27718, 27803, 29148    
Attachments: Bug 24488: Show correct first patron details on Holds to pull
Bug 24488: Show correct first patron details on Holds to pull
Bug 24488: Show correct first patron details on Holds to pull
Bug 24488: Show correct first patron details on Holds to pull
Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY
Bug 24488: Show correct first patron details on Holds to pull
Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY
Bug 24488: Display patron with the highest priority hold
Bug 24488: (follow-up) Reduce repeated code
Bug 24488: Simplify searches
Bug 24488: [DISCUSSION] For comparaison
Bug 24488: [DISCUSSION] For comparaison
Bug 24488: Show correct first patron details on Holds to pull
Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY
Bug 24488: Display patron with the highest priority hold
Bug 24488: Simplify searches
Bug 24488: [DISCUSSION] For comparaison
Bug 24488: Show correct first patron details on Holds to pull
Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY
Bug 24488: Display patron with the highest priority hold
Bug 24488: Simplify searches
Bug 24488: [DISCUSSION] For comparaison
Bug 24488: Show correct first patron details on Holds to pull
Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY
Bug 24488: Display patron with the highest priority hold
Bug 24488: Simplify searches
Bug 24488: [DISCUSSION] For comparaison
Bug 24488: (QA follow-up): Fix small bugs, add checkk for waiting holds
Bug 24488: (QA follow-up) Make pending reserves faster
Bug 24488: (QA follow-up) Remove two queries out of foreach cycle
Bug 24488: (QA follow-up) Remove two queries out of foreach cycle
Bug 24488: (QA follow-up) Move getting items out of cycle
Bug 24488: Fix count distinct patron on a given hold
Bug 24488: perf - Group by at DBMS level
Bug 24488: perf - move patrons_count out of the loop
Bug 24488: Simplify structure passed to the template
Bug 24488: perf - Move holds search out of the loop
Bug 24488: perf - Move holds search out of the loop
Bug 24488: Show correct first patron details on Holds to pull
Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY
Bug 24488: Display patron with the highest priority hold
Bug 24488: Simplify searches
Bug 24488: [DISCUSSION] For comparaison
Bug 24488: (QA follow-up): Fix small bugs, add checkk for waiting holds
Bug 24488: (QA follow-up) Make pending reserves faster
Bug 24488: (QA follow-up) Remove two queries out of foreach cycle
Bug 24488: (QA follow-up) Move getting items out of cycle
Bug 24488: Fix count distinct patron on a given hold
Bug 24488: perf - Group by at DBMS level
Bug 24488: perf - move patrons_count out of the loop
Bug 24488: Simplify structure passed to the template
Bug 24488: perf - Move holds search out of the loop
Bug 24488: Show correct first patron details on Holds to pull
Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY
Bug 24488: Display patron with the highest priority hold
Bug 24488: Simplify searches
Bug 24488: [DISCUSSION] For comparaison
Bug 24488: (QA follow-up): Fix small bugs, add checkk for waiting holds
Bug 24488: (QA follow-up) Make pending reserves faster
Bug 24488: (QA follow-up) Remove two queries out of foreach cycle
Bug 24488: (QA follow-up) Move getting items out of cycle
Bug 24488: Fix count distinct patron on a given hold
Bug 24488: perf - Group by at DBMS level
Bug 24488: perf - move patrons_count out of the loop
Bug 24488: Simplify structure passed to the template
Bug 24488: perf - Move holds search out of the loop
Bug 24488: (QA follow-up) Go to next loop as early as possible
Bug 24488: Show correct first patron details on Holds to pull
Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY
Bug 24488: Display patron with the highest priority hold
Bug 24488: Simplify searches
Bug 24488: [DISCUSSION] For comparaison
Bug 24488: (QA follow-up): Fix small bugs, add checkk for waiting holds
Bug 24488: (QA follow-up) Make pending reserves faster
Bug 24488: (QA follow-up) Remove two queries out of foreach cycle
Bug 24488: (QA follow-up) Move getting items out of cycle
Bug 24488: Fix count distinct patron on a given hold
Bug 24488: perf - Group by at DBMS level
Bug 24488: perf - move patrons_count out of the loop
Bug 24488: Simplify structure passed to the template
Bug 24488: perf - Move holds search out of the loop
Bug 24488: (QA follow-up) Go to next loop as early as possible
Bug 24488: create a loop to avoid copy paste
Bug 24488: Show correct first patron details on Holds to pull
Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY
Bug 24488: Display patron with the highest priority hold
Bug 24488: Simplify searches
Bug 24488: [DISCUSSION] For comparaison
Bug 24488: (QA follow-up): Fix small bugs, add checkk for waiting holds
Bug 24488: (QA follow-up) Make pending reserves faster
Bug 24488: (QA follow-up) Remove two queries out of foreach cycle
Bug 24488: (QA follow-up) Move getting items out of cycle
Bug 24488: Fix count distinct patron on a given hold
Bug 24488: perf - Group by at DBMS level
Bug 24488: perf - move patrons_count out of the loop
Bug 24488: Simplify structure passed to the template
Bug 24488: perf - Move holds search out of the loop
Bug 24488: (QA follow-up) Go to next loop as early as possible
Bug 24488: create a loop to avoid copy paste
Bug 24488: create a loop to avoid copy paste

Description Aleisha Amohia 2020-01-23 03:58:29 UTC
The query that pulls the information for the Circ -> Holds to pull table is incredibly complex and doesn't always pull the correct information. It has a bunch of commands in it that repeatedly reorders the results (group_concats and counts). This means the order of the results cannot be guaranteed to be the same with every execution, and sometimes we see the wrong 'first patron' for the given date selection.

It's hard to test because it only happens sometimes, but I have been able to reproduce it this way:
- Place two holds for two different patrons on the same biblio
- Check the Holds to pull, it'll show the details of the first patron
- Change the reservedate for the second hold in the database to be yesterday
- Check the Holds to pull, it shows the details of the wrong hold under 'first patron' but shows the correct details 'under earliest hold date' 
- Change the date selection, it doesn't fix itself
Comment 1 Aleisha Amohia 2020-01-23 04:03:01 UTC
Created attachment 97773 [details] [review]
Bug 24488: Show correct first patron details on Holds to pull

This patch rewrites the complex search query to reduce the risk of
reordering the results multiple times.

This patch includes a change to schema files so may need to upgrade
schema and/or restart memcached

To test:
1) Reproduce problem following test plan in Description
2) Apply patch and refresh page
3) Notice the correct patron is now shown
4) Play with date selection, confirm correct results are still shown
5) Test cancelling holds
6) Test filtering table results

Sponsored by: IHC New Zealand
Comment 2 Kelly McElligott 2020-01-24 14:34:25 UTC
Aleisha- something odd- nothing is showing in the Holds To Pull report.  I have tried this multiple times and unable to get the holds to populate. Both biblio and Item level holds.
Comment 3 Aleisha Amohia 2020-01-26 20:42:37 UTC
(In reply to Kelly McElligott from comment #2)
> Aleisha- something odd- nothing is showing in the Holds To Pull report.  I
> have tried this multiple times and unable to get the holds to populate. Both
> biblio and Item level holds.

Hi Kelly, is this before or after you apply the patch? If after, did you try upgrading the schema files and restarting memcached?
Comment 4 Kelly McElligott 2020-01-27 15:20:12 UTC
Hi Aleisha,
I have done what you have recommended.  I am now getting an error when I try to go to the Holds to Pull report 

DBIx::Class::Storage::DBI::_dbh_execute(): 'koha_hold.reserve.reserve_id' isn't in GROUP BY at /kohadevbox/koha/Koha/Objects.pm line 130
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77
Comment 5 Aleisha Amohia 2020-01-27 22:06:34 UTC
(In reply to Kelly McElligott from comment #4)
> Hi Aleisha,
> I have done what you have recommended.  I am now getting an error when I try
> to go to the Holds to Pull report 
> 
> DBIx::Class::Storage::DBI::_dbh_execute(): 'koha_hold.reserve.reserve_id'
> isn't in GROUP BY at /kohadevbox/koha/Koha/Objects.pm line 130
>  at /usr/share/perl5/DBIx/Class/Exception.pm line 77

Hi Kelly, I can't reproduce this error. There might be a problem in your database? Sorry!
Comment 6 Katrin Fischer 2020-01-27 22:34:21 UTC
Hi Aleisha, I think this is related to bug 21941/bug 17258 - a DBMS version problem. Can you please check?
Comment 7 Aleisha Amohia 2020-02-13 01:03:44 UTC
(In reply to Katrin Fischer from comment #6)
> Hi Aleisha, I think this is related to bug 21941/bug 17258 - a DBMS version
> problem. Can you please check?

Hi Katrin, sorry I'm not really sure how I'm meant to check? It could be related, but also that query is pretty horrible and I think this fix is nicer code to work with in the long term.

Setting back to Needs signoff because I'm not getting the aforementioned error, and wonder if it happened when the reservedate was manually changed in the database? Regardless, patch still works for me.
Comment 8 Katrin Fischer 2020-02-14 15:11:08 UTC
> DBIx::Class::Storage::DBI::_dbh_execute(): 'koha_hold.reserve.reserve_id'
> isn't in GROUP BY at /kohadevbox/koha/Koha/Objects.pm line 130
>  at /usr/share/perl5/DBIx/Class/Exception.pm line 77

Issues with GROUP BY usually point to problems with stricter DB settings - that's what I meant. It's possible the tester had a different DBMS version / different settings which made it explode - trying to help because you said you cannot reproduce.
Comment 9 Martin Renvoize 2020-02-14 15:21:51 UTC
To summarise.. try setting `<strict_sql_modes>1</strict_sql_modes>` in your koha-conf.xml Aleisha.. it's on by default in sandboxes and dev environments to weed out queries that would fail we strict features enabled at the SQL level.

Looking at the error Kelly is reporting it sounds like strict SQL is enabled and she's catching an error in your reworked SQL query that you're not seeing with sql in 'forgiving' mode ;).

I've not looked at the code yet, but this usually arrises from a group by on a query with a join where you're trying to group by an id field in on table.. the fix, as horrible as it is, is to add all fields in that table to the group by.
Comment 10 Aleisha Amohia 2020-02-16 20:07:50 UTC
I've misunderstood here - I thought that Katrin's comment was talking about the potential cause of the original bug reported, not in relation to reproducing Kelly's error.

Also I'm more than happy to write tests. I had simply forgotten in the first instance.

Will take another look when I get a chance.
Comment 11 Jonathan Druart 2020-05-14 09:08:28 UTC
Kelly's error exists with and without the patch (if strict_sql_modes is set).
Comment 12 Jonathan Druart 2020-05-14 09:09:32 UTC
(In reply to Jonathan Druart from comment #11)
> Kelly's error exists with and without the patch (if strict_sql_modes is set).

Well, at least without the patch, I did not try with the patch applied.
Comment 13 Jonathan Druart 2020-05-21 09:06:55 UTC
*** Bug 24301 has been marked as a duplicate of this bug. ***
Comment 14 Michal Denar 2020-06-23 21:01:52 UTC
Hello Aleisha,
is this patch ready to test? I applied patch, but name of first patron was same after change of holds order as before. Tested on Kohadevbox after restart_all.
Comment 15 Aleisha Amohia 2020-06-23 21:21:47 UTC
Hi Michal, not ready to test yet - needs some work on master and some tests.
Comment 16 Michal Denar 2020-06-23 21:29:11 UTC
OK, I'll wait on "Needs signoff" status :-)

But I think that similar bug has Holds queue page.
Comment 17 Aleisha Amohia 2020-06-29 05:21:22 UTC
Created attachment 106379 [details] [review]
Bug 24488: Show correct first patron details on Holds to pull

This patch rewrites the complex search query to reduce the risk of
reordering the results multiple times.

This patch includes a change to schema files so may need to upgrade
schema and/or restart memcached

To test:
1) Reproduce problem following test plan in Description
2) Apply patch and refresh page
3) Notice the correct patron is now shown
4) Play with date selection, confirm correct results are still shown
5) Test cancelling holds
6) Test filtering table results
7) Test with biblios with multiple items
8) Test with making items unavailable (i.e. not for loan, checked out)

Sponsored by: IHC New Zealand
Comment 18 Aleisha Amohia 2020-06-29 05:22:23 UTC
Just rebasing - still need to try with strict_sql_mode. Will come back to this tomorrow.
Comment 19 Jonathan Druart 2020-06-29 09:37:26 UTC
Yes it's failing with strict mode:
  'koha_kohadev.reserve.reserve_id' isn't in GROUP BY 

Aleisa, if you could compare the performance on a production DB that would be really useful.
Comment 20 Aleisha Amohia 2020-07-14 23:23:18 UTC
Created attachment 106882 [details] [review]
Bug 24488: Show correct first patron details on Holds to pull

This patch rewrites the complex search query to reduce the risk of
reordering the results multiple times.

This patch includes a change to schema files so may need to upgrade
schema and/or restart memcached

To test:
1) Reproduce problem following test plan in Description
2) Apply patch and refresh page
3) Notice the correct patron is now shown
4) Play with date selection, confirm correct results are still shown
5) Test cancelling holds
6) Test filtering table results
7) Test with biblios with multiple items
8) Test with making items unavailable (i.e. not for loan, checked out)

Sponsored by: IHC New Zealand
Comment 21 Aleisha Amohia 2020-07-14 23:24:03 UTC
I enabled strict_sql_modes but unfortunately I still can't reproduce the errors. Can someone give me some instructions for this? I'm happy to keep working on it.
Comment 22 Katrin Fischer 2020-07-18 16:59:46 UTC
Actually I think we have a bigger problem here: I was confusing holds to pull and holds queue earlier. Now testing another patch... 

Holds to pull fails for me with and without this patch in strict mode (error on bug 26020)
Comment 23 Michal Denar 2020-07-18 19:00:10 UTC
Hi Aleisha,
I'm so sorry but this patch didn't solve problem with first patron on my kohadevbox. If I change order of borrower with hold, list without change. I've problems with 'dbic' on kohadevbox, so I tried make schema change manually by !sudo koha-upgrade-schema kohadev'.
Comment 24 Katrin Fischer 2020-07-18 19:07:55 UTC
The patch applies for me - did you mean to set Failed QA?
Comment 25 Michal Denar 2020-07-18 19:34:39 UTC
Hi Katrin,
yes,QA faild for me. Thank you. I saw on IRC, that you fight with 'dbic' too. Did you solve it?
Comment 26 Katrin Fischer 2020-07-18 19:36:01 UTC
(In reply to Michal Denar from comment #25)
> Hi Katrin,
> yes,QA faild for me. Thank you. I saw on IRC, that you fight with 'dbic'
> too. Did you solve it?

No, I ended up running the normal command in koha-shell - so doing it manually instead of taking the shortcut.
Comment 27 Aleisha Amohia 2020-07-19 20:48:07 UTC
(In reply to Katrin Fischer from comment #22)
> Actually I think we have a bigger problem here: I was confusing holds to
> pull and holds queue earlier. Now testing another patch... 
> 
> Holds to pull fails for me with and without this patch in strict mode (error
> on bug 26020)

Thanks for pointing this out and reporting it, Katrin. I still can't seem to enable strict mode, can you please tell me how to do this?

I suppose we want to fix the bug on Bug 26020 before getting this patch through (in fact fixing Bug 26020 might fix this problem) so I'll set 26020 as blocking this bug.
Comment 28 Katrin Fischer 2020-07-19 21:03:54 UTC
Hi Aleiha, all I do is changing the setting in the koha-conf.xml file and restarted all the things including the mysql server. Not sure if that's needed. I later turend it off again so I coudl continue testing the other patch and get around the bug.
Comment 29 Katrin Fischer 2020-07-19 21:05:02 UTC
(In reply to Katrin Fischer from comment #28)
> Hi Aleiha, all I do is changing the setting in the koha-conf.xml file and
> restarted all the things including the mysql server. Not sure if that's
> needed. I later turend it off again so I coudl continue testing the other
> patch and get around the bug.

Oh, and your patch didn't fix it for me - still showing the same problem with the group by. (it's not changed, I think)
Comment 30 Jonathan Druart 2020-07-20 10:04:20 UTC
+              itemnumber => { -not_in => \'SELECT itemnumber FROM branchtransfers WHERE datearrived IS NULL', -not_in => \'SELECT itemnumber FROM issues' },

There are twice the same key in the hash "-not_in", only the second one will be used.
As you are reusing this set in different ->search I think you should retrieve them once to have the itemnumber list in a perl array.
Comment 31 Aleisha Amohia 2020-07-23 00:01:28 UTC
Created attachment 107206 [details] [review]
Bug 24488: Show correct first patron details on Holds to pull

This patch rewrites the complex search query to reduce the risk of
reordering the results multiple times.

This patch includes a change to schema files so may need to upgrade
schema and/or restart memcached

To test:
1) Reproduce problem following test plan in Description
2) Apply patch and refresh page
3) Notice the correct patron is now shown
4) Play with date selection, confirm correct results are still shown
5) Test cancelling holds
6) Test filtering table results
7) Test with biblios with multiple items
8) Test with making items unavailable (i.e. not for loan, checked out)

Sponsored by: IHC New Zealand
Comment 32 Aleisha Amohia 2020-07-23 00:02:52 UTC
Sorry team I really cannot seem to enable strict_sql_modes and reproduce that error, I tried a bunch of different things including MariaDB specific things but I couldn't reproduce.

I've reworked the patch a bit, and really curious if it stops that error from showing. Please test and let me know how it goes.
Comment 33 Jonathan Druart 2020-07-23 06:43:46 UTC
(In reply to Aleisha Amohia from comment #27)
> (In reply to Katrin Fischer from comment #22)
> > Actually I think we have a bigger problem here: I was confusing holds to
> > pull and holds queue earlier. Now testing another patch... 
> > 
> > Holds to pull fails for me with and without this patch in strict mode (error
> > on bug 26020)
> 
> Thanks for pointing this out and reporting it, Katrin. I still can't seem to
> enable strict mode, can you please tell me how to do this?
> 
> I suppose we want to fix the bug on Bug 26020 before getting this patch
> through (in fact fixing Bug 26020 might fix this problem) so I'll set 26020
> as blocking this bug.

I have the hope to fix the problematic group by with your patches, Aleisha. We already tried to fix it on bug 22431 but failed, so I don't think we should depend on another bug report.
Comment 34 Jonathan Druart 2020-07-23 06:58:22 UTC
Created attachment 107213 [details] [review]
Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY
Comment 35 Michal Denar 2020-07-23 21:27:27 UTC
Hi,
sorry Aleisha and Jonathan. But patch don't work for me. If you change priority of holds, name on List to Pull is still same as befere change. It's not on test plan, I know. But I should work too. Maybe someone test it too. Katrin?
https://recordit.co/RA8bCjXekD
Comment 36 Aleisha Amohia 2020-07-23 22:25:58 UTC
(In reply to Michal Denar from comment #35)
> Hi,
> sorry Aleisha and Jonathan. But patch don't work for me. If you change
> priority of holds, name on List to Pull is still same as befere change. It's
> not on test plan, I know. But I should work too. Maybe someone test it too.
> Katrin?
> https://recordit.co/RA8bCjXekD

Hi Michal, I don't think it's about priority, it's about the earliest place hold. As far as I know the Holds to Pull shows the 'first patron' as based on date, I don't think it ever considered priority. But I may have that wrong, perhaps Jonathan or Katrin can confirm?
Comment 37 Jonathan Druart 2020-07-24 14:29:51 UTC
I am expecting the first patron in the queue to be displayed.

The query did not deal with the priority indeed, but this query is plain wrong :D
Comment 38 Michal Denar 2020-07-24 19:52:30 UTC
Hi,
we'll change status on Failed QA? For me is best solution make 'first patron' based on wating queue (smallest non suspended priority number)?
Comment 39 Aleisha Amohia 2020-08-05 21:24:47 UTC
So to confirm, this patch will not only fix the group_by error but we also want to fix the Holds to Pull to show the highest priority hold instead of the earliest placed hold? Because currently it shows the earliest placed hold.

Happy to make this change.
Comment 40 Michal Denar 2020-08-10 21:32:13 UTC
Hi Aleisha,
from my point of view focus on priority solve both scenarios. If borrower make hold, Koha set priority to "1". Next hold "2", etc. If case that priority will be changed, Hold too pull list show correct borrower too. Win-win for me.
Comment 41 Michal Denar 2020-09-28 19:24:10 UTC
Hi Aleisha,
any updates?

Thank you.
Comment 42 Aleisha Amohia 2020-09-28 20:40:24 UTC
Unfortunately, not yet sorry. It's on my list to get to when I have time.
Comment 43 Michal Denar 2020-09-28 20:55:17 UTC
OK
Comment 44 Jonathan Druart 2020-10-07 15:23:42 UTC
Created attachment 111341 [details] [review]
Bug 24488: Show correct first patron details on Holds to pull

This patch rewrites the complex search query to reduce the risk of
reordering the results multiple times.

This patch includes a change to schema files so may need to upgrade
schema and/or restart memcached

To test:
1) Reproduce problem following test plan in Description
2) Apply patch and refresh page
3) Notice the correct patron is now shown
4) Play with date selection, confirm correct results are still shown
5) Test cancelling holds
6) Test filtering table results
7) Test with biblios with multiple items
8) Test with making items unavailable (i.e. not for loan, checked out)

Sponsored by: IHC New Zealand
Comment 45 Jonathan Druart 2020-10-07 15:23:50 UTC
Created attachment 111342 [details] [review]
Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY
Comment 46 Jonathan Druart 2020-10-07 15:23:56 UTC
Created attachment 111343 [details] [review]
Bug 24488: Display patron with the highest priority hold
Comment 47 Jonathan Druart 2020-10-07 15:25:48 UTC
Patches have been rebased (and I've added some modifications to the first patch, especially related to bug 23485 and bug 25534).

I have also added a patch to display the patron with the highest priority hold.

Reading for testing again.
Comment 48 Nick Clemens 2020-10-07 15:59:57 UTC
Created attachment 111347 [details] [review]
Bug 24488: (follow-up) Reduce repeated code
Comment 49 Jonathan Druart 2020-10-07 16:06:42 UTC
Created attachment 111348 [details] [review]
Bug 24488: Simplify searches

It also:
Use dtf for date handling
add missing td
Comment 50 Jonathan Druart 2020-10-07 16:20:44 UTC
Aleisha, what were you trying to achieve with the following code?

196 foreach my $bibnum ( @biblionumbers ){
197 
198     my @items = Koha::Items->search({ biblionumber => $bibnum });
199     foreach my $i ( @items ){
200         if ( $i->checkout ){
201             next;
202         }
203     }
Comment 51 Jonathan Druart 2020-10-07 16:35:18 UTC
Created attachment 111349 [details] [review]
Bug 24488: [DISCUSSION] For comparaison

I'd like someone to run a benchmark with and without this patch on a
quite big database and see which approach is the best
Comment 52 Jonathan Druart 2020-10-07 16:40:21 UTC
Created attachment 111350 [details] [review]
Bug 24488: [DISCUSSION] For comparaison

I'd like someone to run a benchmark with and without this patch on a
quite big database and see which approach is the best
Comment 53 Michal Denar 2020-10-07 19:33:14 UTC
Created attachment 111358 [details] [review]
Bug 24488: Show correct first patron details on Holds to pull

This patch rewrites the complex search query to reduce the risk of
reordering the results multiple times.

This patch includes a change to schema files so may need to upgrade
schema and/or restart memcached

To test:
1) Reproduce problem following test plan in Description
2) Apply patch and refresh page
3) Notice the correct patron is now shown
4) Play with date selection, confirm correct results are still shown
5) Test cancelling holds
6) Test filtering table results
7) Test with biblios with multiple items
8) Test with making items unavailable (i.e. not for loan, checked out)

Sponsored by: IHC New Zealand

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 54 Michal Denar 2020-10-07 19:33:24 UTC
Created attachment 111359 [details] [review]
Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 55 Michal Denar 2020-10-07 19:33:35 UTC
Created attachment 111360 [details] [review]
Bug 24488: Display patron with the highest priority hold

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 56 Michal Denar 2020-10-07 19:33:45 UTC
Created attachment 111361 [details] [review]
Bug 24488: Simplify searches

It also:
Use dtf for date handling
add missing td

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 57 Michal Denar 2020-10-07 19:33:51 UTC
Created attachment 111362 [details] [review]
Bug 24488: [DISCUSSION] For comparaison

I'd like someone to run a benchmark with and without this patch on a
quite big database and see which approach is the best

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 58 Katrin Fischer 2020-10-22 23:51:29 UTC
Neither the original patches nor Joubu's alternative(?) apply at the moment. I feel like we got a bit stuck here. 

Aleisha, could you take a look at comment#50 too when rebasing and maybe Joubu's patch?
Comment 59 Jonathan Druart 2020-11-02 08:22:37 UTC
Created attachment 112815 [details] [review]
Bug 24488: Show correct first patron details on Holds to pull

This patch rewrites the complex search query to reduce the risk of
reordering the results multiple times.

This patch includes a change to schema files so may need to upgrade
schema and/or restart memcached

To test:
1) Reproduce problem following test plan in Description
2) Apply patch and refresh page
3) Notice the correct patron is now shown
4) Play with date selection, confirm correct results are still shown
5) Test cancelling holds
6) Test filtering table results
7) Test with biblios with multiple items
8) Test with making items unavailable (i.e. not for loan, checked out)

Sponsored by: IHC New Zealand

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 60 Jonathan Druart 2020-11-02 08:22:41 UTC
Created attachment 112816 [details] [review]
Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 61 Jonathan Druart 2020-11-02 08:22:45 UTC
Created attachment 112817 [details] [review]
Bug 24488: Display patron with the highest priority hold

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 62 Jonathan Druart 2020-11-02 08:22:49 UTC
Created attachment 112818 [details] [review]
Bug 24488: Simplify searches

It also:
Use dtf for date handling
add missing td

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 63 Jonathan Druart 2020-11-02 08:22:53 UTC
Created attachment 112819 [details] [review]
Bug 24488: [DISCUSSION] For comparaison

I'd like someone to run a benchmark with and without this patch on a
quite big database and see which approach is the best

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 64 Jonathan Druart 2020-11-02 08:23:19 UTC
(In reply to Katrin Fischer from comment #58)
> Neither the original patches nor Joubu's alternative(?) apply at the moment.
> I feel like we got a bit stuck here. 

Easy conflict solved.
Comment 65 Josef Moravec 2020-11-17 10:47:42 UTC
Created attachment 113695 [details] [review]
Bug 24488: Show correct first patron details on Holds to pull

This patch rewrites the complex search query to reduce the risk of
reordering the results multiple times.

This patch includes a change to schema files so may need to upgrade
schema and/or restart memcached

To test:
1) Reproduce problem following test plan in Description
2) Apply patch and refresh page
3) Notice the correct patron is now shown
4) Play with date selection, confirm correct results are still shown
5) Test cancelling holds
6) Test filtering table results
7) Test with biblios with multiple items
8) Test with making items unavailable (i.e. not for loan, checked out)

Sponsored by: IHC New Zealand

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 66 Josef Moravec 2020-11-17 10:47:50 UTC
Created attachment 113696 [details] [review]
Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 67 Josef Moravec 2020-11-17 10:47:56 UTC
Created attachment 113697 [details] [review]
Bug 24488: Display patron with the highest priority hold

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 68 Josef Moravec 2020-11-17 10:48:01 UTC
Created attachment 113698 [details] [review]
Bug 24488: Simplify searches

It also:
Use dtf for date handling
add missing td

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 69 Josef Moravec 2020-11-17 10:48:07 UTC
Created attachment 113699 [details] [review]
Bug 24488: [DISCUSSION] For comparaison

I'd like someone to run a benchmark with and without this patch on a
quite big database and see which approach is the best

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 70 Josef Moravec 2020-11-17 10:48:13 UTC
Created attachment 113700 [details] [review]
Bug 24488: (QA follow-up): Fix small bugs, add checkk for waiting holds

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 71 Josef Moravec 2020-11-17 16:01:14 UTC
I have done a little benchmark

Testing DB - copy from production
  85k biblios
  125k items
  15k reserves

I ran twice a batch of 10 runs of pendingreserves.pl and measure clean time of this script running (from begin to end of whole script)

Original version from Aleisha:
Run 1: 7.57 s avg
Run 1: 8.1 s avg

Joubu's alternative:
Run 1: 5,96 s
Run 2: 6,04 s

That is between 20 and 25 percent gain for Joubu's alternative patch
Comment 72 Nick Clemens 2020-11-17 17:34:46 UTC
(In reply to Jonathan Druart from comment #63)
> I'd like someone to run a benchmark with and without this patch on a
> quite big database and see which approach is the best

I placed 1000 holds in my system:
INSERT INTO reserves (borrowernumber,reservedate,biblionumber,branchcode,priority,itemnumber) SELECT CEIL(rand()*50),CURDATE(),biblionumber,'CPL',1,NULL FROM biblio;

On master (strict mode disabled):
< 10s

Without discusson patch and followup:
> 1 min 20s

With discussion and followup:
< 50 s

While the follow-up offers great improvement, I am not impressed with the slowdown overall
Comment 73 Nick Clemens 2020-11-17 17:35:22 UTC
(In reply to Josef Moravec from comment #71)
> I have done a little benchmark
> 
> Testing DB - copy from production
>   85k biblios
>   125k items
>   15k reserves
> 
> I ran twice a batch of 10 runs of pendingreserves.pl and measure clean time
> of this script running (from begin to end of whole script)
> 
> Original version from Aleisha:
> Run 1: 7.57 s avg
> Run 1: 8.1 s avg
> 
> Joubu's alternative:
> Run 1: 5,96 s
> Run 2: 6,04 s
> 
> That is between 20 and 25 percent gain for Joubu's alternative patch

How many holds were reported on the report for you?
Comment 74 Josef Moravec 2020-11-17 18:35:55 UTC
Created attachment 113756 [details] [review]
Bug 24488: (QA follow-up) Make pending reserves faster

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 75 Josef Moravec 2020-11-17 18:40:52 UTC
(In reply to Nick Clemens from comment #72)
> (In reply to Jonathan Druart from comment #63)
> > I'd like someone to run a benchmark with and without this patch on a
> > quite big database and see which approach is the best
> 
> I placed 1000 holds in my system:
> INSERT INTO reserves
> (borrowernumber,reservedate,biblionumber,branchcode,priority,itemnumber)
> SELECT CEIL(rand()*50),CURDATE(),biblionumber,'CPL',1,NULL FROM biblio;
> 
> On master (strict mode disabled):
> < 10s
> 
> Without discusson patch and followup:
> > 1 min 20s
> 
> With discussion and followup:
> < 50 s
> 
> While the follow-up offers great improvement, I am not impressed with the
> slowdown overall

There is in fact no need to select all itemnumbers from checkouts...

I added a follow-up, for me it is now significantly faster, even something like 10 times faster then before, could you test on your data?
Comment 76 Josef Moravec 2020-11-17 19:03:00 UTC
Created attachment 113757 [details] [review]
Bug 24488: (QA follow-up) Remove two queries out of foreach cycle

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 77 Josef Moravec 2020-11-17 20:40:33 UTC
Created attachment 113763 [details] [review]
Bug 24488: (QA follow-up) Remove two queries out of foreach cycle

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 78 Josef Moravec 2020-11-17 20:40:38 UTC
Created attachment 113764 [details] [review]
Bug 24488: (QA follow-up) Move getting items out of cycle

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 79 Jonathan Druart 2020-11-18 07:56:45 UTC
Josef, I am expecting this last patch to improve perf only in the case where all items are coming from the same biblio. Which don't happen on a real-life situation :)
Comment 80 Jonathan Druart 2020-11-18 07:59:21 UTC
And the memory usage will be worst (maybe negligible however).
Comment 81 Jonathan Druart 2020-11-18 09:08:06 UTC
17-18s (josef)

18-19 (mine)

5s (master)

Working on this.
Comment 82 Martin Renvoize 2020-11-18 09:25:57 UTC
Comment on attachment 113696 [details] [review]
Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY

Review of attachment 113696 [details] [review]:
-----------------------------------------------------------------

::: circ/pendingreserves.pl
@@ +190,2 @@
>  );
> +my @biblionumbers = uniq $distinct_holds->get_column('biblionumber');

We could do this at the DB level: `my @biblionumbers = $distinct_holds->_resultset->get_column('biblionumber')->func('DISTINCT');`
Comment 83 Jonathan Druart 2020-11-18 10:54:05 UTC
Created attachment 113774 [details] [review]
Bug 24488: Fix count distinct patron on a given hold
Comment 84 Jonathan Druart 2020-11-18 10:54:09 UTC
Created attachment 113775 [details] [review]
Bug 24488: perf - Group by at DBMS level
Comment 85 Jonathan Druart 2020-11-18 10:54:13 UTC
Created attachment 113776 [details] [review]
Bug 24488: perf - move patrons_count out of the loop
Comment 86 Jonathan Druart 2020-11-18 10:54:17 UTC
Created attachment 113777 [details] [review]
Bug 24488: Simplify structure passed to the template
Comment 87 Jonathan Druart 2020-11-18 10:54:21 UTC
Created attachment 113778 [details] [review]
Bug 24488: perf - Move holds search out of the loop

This will need attention from QA. Are we sure it's doing the same thing
(especially the order by!)
Comment 88 Jonathan Druart 2020-11-18 10:55:59 UTC
With the last patches I am at ~9sec (the last patch is doing the best part)

I am afraid this one won't hit 20.11.

Setting back to NSO as I'd like a full retest of the whole patch stack.
Comment 90 Josef Moravec 2020-11-19 07:04:45 UTC
This does not work correctly - in fact the behaviour is same as before patch (only slower)

When we get all holds at once, it will be the same as before... we need to do one of this things:

- get hold by biblionumber in each loop - revert last patch - bad performance
- add 'priority' => 1, to where clause - best performance, but can we 
- add subquery and get only the holds with priority = min(priority) for given biblionumber - probably the optimal solution?
Comment 91 Josef Moravec 2020-11-19 07:06:47 UTC
(In reply to Josef Moravec from comment #90)
> This does not work correctly - in fact the behaviour is same as before patch
> (only slower)
> 
> When we get all holds at once, it will be the same as before... we need to
> do one of this things:
> 
> - get hold by biblionumber in each loop - revert last patch - bad performance
> - add 'priority' => 1, to where clause - best performance, but can we 
> - add subquery and get only the holds with priority = min(priority) for
> given biblionumber - probably the optimal solution?

Sorry, commited comment too early ;)

ad second point:
- add 'priority' => 1, to where clause - best performance, but can we assume the lowest priority is always 1?
Comment 92 Jonathan Druart 2020-11-19 09:06:07 UTC
Created attachment 113798 [details] [review]
Bug 24488: perf - Move holds search out of the loop

This will need attention from QA.
We are assuming that the hold with the highest priority has priority=1
Comment 93 Jonathan Druart 2020-11-19 09:08:41 UTC
Sorry, the last patch was silly indeed. I hadn't turned on the strict mode.

Your suggestion, Josef, of assuming that the highest priority hold has priority=1 is great actually. I think we never wanted to assume that (in case the data is corrupted?) in this script, but we should go for it IMO, that simplifies the code, the queries, and it makes sense to me!
Comment 94 Katrin Fischer 2020-11-19 09:31:12 UTC
(In reply to Jonathan Druart from comment #93)
> Sorry, the last patch was silly indeed. I hadn't turned on the strict mode.
> 
> Your suggestion, Josef, of assuming that the highest priority hold has
> priority=1 is great actually. I think we never wanted to assume that (in
> case the data is corrupted?) in this script, but we should go for it IMO,
> that simplifies the code, the queries, and it makes sense to me!

I believe in the past we had issues with the priorities not being correct, but believe it was fixed (hopefully). Nick or Kyle might be able to tell.

Wondering what happens with the local holds priority settings - do they change the priority in the db or do they check highest priority for local?
Comment 95 Nick Clemens 2020-11-19 13:06:31 UTC
(In reply to Katrin Fischer from comment #94)
> (In reply to Jonathan Druart from comment #93)
> > Sorry, the last patch was silly indeed. I hadn't turned on the strict mode.
> > 
> > Your suggestion, Josef, of assuming that the highest priority hold has
> > priority=1 is great actually. I think we never wanted to assume that (in
> > case the data is corrupted?) in this script, but we should go for it IMO,
> > that simplifies the code, the queries, and it makes sense to me!
> 
> I believe in the past we had issues with the priorities not being correct,
> but believe it was fixed (hopefully). Nick or Kyle might be able to tell.
> 
> Wondering what happens with the local holds priority settings - do they
> change the priority in the db or do they check highest priority for local?

I checked in our production sites, there are few cases where the highest priority is not 1, and they mostly seem to be related to other issues (suspended holds, holds in the future, missing items) so nothing that should show on holds to pull.

Local holds don't alter the priority, they get checked first when filling holds or building the queue, so this report won't catch them, but I think that is fine as this report isn't branch specific

Other options (for another bug) would be:
1 - List all the holds - just show the first and maybe add a modal to see all the others
2 - Create a cron like the holds queue, build this page on the backend and simply display - it adds a delay to new holds showing, but allows us to calculate the exact holds to be pulled
3 - Add a branch limiter to the report and default to local branches items
Comment 96 Michal Denar 2020-11-19 16:29:25 UTC
Created attachment 113829 [details] [review]
Bug 24488: Show correct first patron details on Holds to pull

This patch rewrites the complex search query to reduce the risk of
reordering the results multiple times.

This patch includes a change to schema files so may need to upgrade
schema and/or restart memcached

To test:
1) Reproduce problem following test plan in Description
2) Apply patch and refresh page
3) Notice the correct patron is now shown
4) Play with date selection, confirm correct results are still shown
5) Test cancelling holds
6) Test filtering table results
7) Test with biblios with multiple items
8) Test with making items unavailable (i.e. not for loan, checked out)

Sponsored by: IHC New Zealand

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.comm>
Comment 97 Michal Denar 2020-11-19 16:29:31 UTC
Created attachment 113830 [details] [review]
Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denra <black23@gmail.comm>
Comment 98 Michal Denar 2020-11-19 16:29:36 UTC
Created attachment 113831 [details] [review]
Bug 24488: Display patron with the highest priority hold

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denra <black23@gmail.comm>
Comment 99 Michal Denar 2020-11-19 16:29:41 UTC
Created attachment 113832 [details] [review]
Bug 24488: Simplify searches

It also:
Use dtf for date handling
add missing td

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denra <black23@gmail.comm>
Comment 100 Michal Denar 2020-11-19 16:29:46 UTC
Created attachment 113833 [details] [review]
Bug 24488: [DISCUSSION] For comparaison

I'd like someone to run a benchmark with and without this patch on a
quite big database and see which approach is the best

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denra <black23@gmail.comm>
Comment 101 Michal Denar 2020-11-19 16:29:51 UTC
Created attachment 113834 [details] [review]
Bug 24488: (QA follow-up): Fix small bugs, add checkk for waiting holds

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denra <black23@gmail.comm>
Comment 102 Michal Denar 2020-11-19 16:29:56 UTC
Created attachment 113835 [details] [review]
Bug 24488: (QA follow-up) Make pending reserves faster

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denra <black23@gmail.comm>
Comment 103 Michal Denar 2020-11-19 16:30:00 UTC
Created attachment 113836 [details] [review]
Bug 24488: (QA follow-up) Remove two queries out of foreach cycle

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denra <black23@gmail.comm>
Comment 104 Michal Denar 2020-11-19 16:30:05 UTC
Created attachment 113837 [details] [review]
Bug 24488: (QA follow-up) Move getting items out of cycle

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denra <black23@gmail.comm>
Comment 105 Michal Denar 2020-11-19 16:30:10 UTC
Created attachment 113838 [details] [review]
Bug 24488: Fix count distinct patron on a given hold

Signed-off-by: Michal Denra <black23@gmail.comm>
Comment 106 Michal Denar 2020-11-19 16:30:15 UTC
Created attachment 113839 [details] [review]
Bug 24488: perf - Group by at DBMS level

Signed-off-by: Michal Denra <black23@gmail.comm>
Comment 107 Michal Denar 2020-11-19 16:30:20 UTC
Created attachment 113840 [details] [review]
Bug 24488: perf - move patrons_count out of the loop

Signed-off-by: Michal Denra <black23@gmail.comm>
Comment 108 Michal Denar 2020-11-19 16:30:24 UTC
Created attachment 113841 [details] [review]
Bug 24488: Simplify structure passed to the template

Signed-off-by: Michal Denra <black23@gmail.comm>
Comment 109 Michal Denar 2020-11-19 16:30:29 UTC
Created attachment 113842 [details] [review]
Bug 24488: perf - Move holds search out of the loop

This will need attention from QA.
We are assuming that the hold with the highest priority has priority=1

Signed-off-by: Michal Denra <black23@gmail.comm>
Comment 110 Josef Moravec 2020-11-20 06:23:48 UTC
(In reply to Michal Denar from comment #108)
> Created attachment 113841 [details] [review] [review]
> Bug 24488: Simplify structure passed to the template
> 
> Signed-off-by: Michal Denra <black23@gmail.comm>

Thanks Michal for testing, just a note, you have misspelled your name and email in your git configuration
Comment 111 Josef Moravec 2020-11-20 06:23:54 UTC
Created attachment 113850 [details] [review]
Bug 24488: Show correct first patron details on Holds to pull

This patch rewrites the complex search query to reduce the risk of
reordering the results multiple times.

This patch includes a change to schema files so may need to upgrade
schema and/or restart memcached

To test:
1) Reproduce problem following test plan in Description
2) Apply patch and refresh page
3) Notice the correct patron is now shown
4) Play with date selection, confirm correct results are still shown
5) Test cancelling holds
6) Test filtering table results
7) Test with biblios with multiple items
8) Test with making items unavailable (i.e. not for loan, checked out)

Sponsored by: IHC New Zealand

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 112 Josef Moravec 2020-11-20 06:24:01 UTC
Created attachment 113851 [details] [review]
Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 113 Josef Moravec 2020-11-20 06:24:07 UTC
Created attachment 113852 [details] [review]
Bug 24488: Display patron with the highest priority hold

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 114 Josef Moravec 2020-11-20 06:24:13 UTC
Created attachment 113853 [details] [review]
Bug 24488: Simplify searches

It also:
Use dtf for date handling
add missing td

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 115 Josef Moravec 2020-11-20 06:24:20 UTC
Created attachment 113854 [details] [review]
Bug 24488: [DISCUSSION] For comparaison

I'd like someone to run a benchmark with and without this patch on a
quite big database and see which approach is the best

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 116 Josef Moravec 2020-11-20 06:24:27 UTC
Created attachment 113855 [details] [review]
Bug 24488: (QA follow-up): Fix small bugs, add checkk for waiting holds

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 117 Josef Moravec 2020-11-20 06:24:34 UTC
Created attachment 113856 [details] [review]
Bug 24488: (QA follow-up) Make pending reserves faster

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 118 Josef Moravec 2020-11-20 06:24:40 UTC
Created attachment 113857 [details] [review]
Bug 24488: (QA follow-up) Remove two queries out of foreach cycle

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 119 Josef Moravec 2020-11-20 06:24:47 UTC
Created attachment 113858 [details] [review]
Bug 24488: (QA follow-up) Move getting items out of cycle

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 120 Josef Moravec 2020-11-20 06:24:53 UTC
Created attachment 113859 [details] [review]
Bug 24488: Fix count distinct patron on a given hold

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 121 Josef Moravec 2020-11-20 06:25:00 UTC
Created attachment 113860 [details] [review]
Bug 24488: perf - Group by at DBMS level

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 122 Josef Moravec 2020-11-20 06:25:06 UTC
Created attachment 113861 [details] [review]
Bug 24488: perf - move patrons_count out of the loop

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 123 Josef Moravec 2020-11-20 06:25:13 UTC
Created attachment 113862 [details] [review]
Bug 24488: Simplify structure passed to the template

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 124 Josef Moravec 2020-11-20 06:25:20 UTC
Created attachment 113863 [details] [review]
Bug 24488: perf - Move holds search out of the loop

This will need attention from QA.
We are assuming that the hold with the highest priority has priority=1

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 125 Josef Moravec 2020-11-20 06:25:27 UTC
Created attachment 113864 [details] [review]
Bug 24488: (QA follow-up) Go to next loop as early as possible

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 126 Josef Moravec 2020-11-20 06:28:28 UTC
Looks good now I think

Nick or Jonathan, can you please review my last follow-up?

It does not give much speed gain, but still measurable on my testing machine.
Comment 127 Martin Renvoize 2020-11-20 09:38:17 UTC
This looks like a great improvement, movin us to searching via objects etc... but... dare I say it, can we move this to a module and thus add tests?

There's a lot of functionality here in filtering down to the right level, improving performance etc.. be great to have that solidly unit/regression tested ;)
Comment 128 Jonathan Druart 2020-11-20 09:54:53 UTC
I thought a bit more about the priority=1 thing this morning.

And actually I have the feeling that the original issue described on this bug report could be fixed with the following (...one line!) change:

diff --git a/circ/pendingreserves.pl b/circ/pendingreserves.pl
index 01cf877b55..962ebe5609 100755
--- a/circ/pendingreserves.pl
+++ b/circ/pendingreserves.pl
@@ -230,7 +230,7 @@ my $strsth =
     AND items.itemnumber NOT IN (SELECT itemnumber FROM branchtransfers where datearrived IS NULL)
     AND items.itemnumber NOT IN (SELECT itemnumber FROM reserves WHERE found IS NOT NULL AND itemnumber IS NOT NULL)
     AND issues.itemnumber IS NULL
-    AND reserves.priority <> 0 
+    AND reserves.priority = 1
     AND reserves.suspend = 0
     AND notforloan = 0 AND itemlost = 0 AND withdrawn = 0
     AND ( circulation_rules.rule_value IS NULL OR circulation_rules.rule_value != 0 )
Comment 129 Joonas Kylmälä 2020-12-01 10:41:12 UTC
(In reply to Jonathan Druart from comment #128)
> I thought a bit more about the priority=1 thing this morning.
> 
> And actually I have the feeling that the original issue described on this
> bug report could be fixed with the following (...one line!) change:
> 
> diff --git a/circ/pendingreserves.pl b/circ/pendingreserves.pl
> index 01cf877b55..962ebe5609 100755
> --- a/circ/pendingreserves.pl
> +++ b/circ/pendingreserves.pl
> @@ -230,7 +230,7 @@ my $strsth =
>      AND items.itemnumber NOT IN (SELECT itemnumber FROM branchtransfers
> where datearrived IS NULL)
>      AND items.itemnumber NOT IN (SELECT itemnumber FROM reserves WHERE
> found IS NOT NULL AND itemnumber IS NOT NULL)
>      AND issues.itemnumber IS NULL
> -    AND reserves.priority <> 0 
> +    AND reserves.priority = 1
>      AND reserves.suspend = 0

This doesn't work because then rcount i.e. "Pull this many items" would only show 1 when in reality you need to pull many more items also to fill those other reserves.

PS. unrelated, but rcount is also incorrect, it should be the number of holds, not amount of borrowers.
Comment 130 Joonas Kylmälä 2020-12-01 11:01:46 UTC
(In reply to Martin Renvoize from comment #127)
> This looks like a great improvement, movin us to searching via objects
> etc... but... dare I say it, can we move this to a module and thus add tests?

I think this definitely needs to use a module, the code population of @all_items should be replaced with an object's method call that returns items that can fill holds. Then it will make super easy to read code where you just iterate over all the items and add the callnumber, shelving location, etc. info to the $reserves variable that goes.

Btw, to resolve the original issue, can't we just order by reserve priority? This refactoring could be in a separate bug.
Comment 131 Jonathan Druart 2020-12-01 11:38:01 UTC
(In reply to Joonas Kylmälä from comment #130)
> (In reply to Martin Renvoize from comment #127)
> > This looks like a great improvement, movin us to searching via objects
> > etc... but... dare I say it, can we move this to a module and thus add tests?
> 
> I think this definitely needs to use a module, the code population of
> @all_items should be replaced with an object's method call that returns
> items that can fill holds. Then it will make super easy to read code where
> you just iterate over all the items and add the callnumber, shelving
> location, etc. info to the $reserves variable that goes.

Yes, it would be nice to have tests and make the code even more clean. But given from when we started, I think it's already a great improvement.
I really would like to have this into master at the beginning of the release cycle, and I am not sure we are going to find volunteers to write tests soon...

> Btw, to resolve the original issue, can't we just order by reserve priority?
> This refactoring could be in a separate bug.

No, I don't think it will work, because of the group by.
Comment 132 Jonathan Druart 2020-12-01 13:14:24 UTC
I had a (quick) look at how we could move the @all_items construction into a Koha::Items->filter_by_ method. I didn't find a simply way to do it, keeping code readable and without duplication.
Comment 133 Joonas Kylmälä 2020-12-01 13:22:34 UTC
(In reply to Jonathan Druart from comment #132)
> I had a (quick) look at how we could move the @all_items construction into a
> Koha::Items->filter_by_ method. I didn't find a simply way to do it, keeping
> code readable and without duplication.

Having it under Koha::Items was not my intention and would not lead to any clearer result. It should be something like Koha::Holds->find($biblionumber)->items_that_can_fill and Koha::Hold->items_that_can_fill. But for this we need the CanItemFillHold from https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659#c6 I think. And of course some other naming scheme might be better, take items_that_can_fill as just an example.
Comment 134 Jonathan Druart 2020-12-02 08:21:24 UTC
I've opened a new bug report. I don't think we should add more patch to this one.

See bug 27131.
Comment 135 Jonathan Druart 2021-01-05 08:30:07 UTC
Created attachment 114835 [details] [review]
Bug 24488: Show correct first patron details on Holds to pull

This patch rewrites the complex search query to reduce the risk of
reordering the results multiple times.

This patch includes a change to schema files so may need to upgrade
schema and/or restart memcached

To test:
1) Reproduce problem following test plan in Description
2) Apply patch and refresh page
3) Notice the correct patron is now shown
4) Play with date selection, confirm correct results are still shown
5) Test cancelling holds
6) Test filtering table results
7) Test with biblios with multiple items
8) Test with making items unavailable (i.e. not for loan, checked out)

Sponsored by: IHC New Zealand

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 136 Jonathan Druart 2021-01-05 08:30:12 UTC
Created attachment 114836 [details] [review]
Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 137 Jonathan Druart 2021-01-05 08:30:16 UTC
Created attachment 114837 [details] [review]
Bug 24488: Display patron with the highest priority hold

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 138 Jonathan Druart 2021-01-05 08:30:20 UTC
Created attachment 114838 [details] [review]
Bug 24488: Simplify searches

It also:
Use dtf for date handling
add missing td

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 139 Jonathan Druart 2021-01-05 08:30:25 UTC
Created attachment 114839 [details] [review]
Bug 24488: [DISCUSSION] For comparaison

I'd like someone to run a benchmark with and without this patch on a
quite big database and see which approach is the best

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 140 Jonathan Druart 2021-01-05 08:30:29 UTC
Created attachment 114840 [details] [review]
Bug 24488: (QA follow-up): Fix small bugs, add checkk for waiting holds

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 141 Jonathan Druart 2021-01-05 08:30:33 UTC
Created attachment 114841 [details] [review]
Bug 24488: (QA follow-up) Make pending reserves faster

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 142 Jonathan Druart 2021-01-05 08:30:38 UTC
Created attachment 114842 [details] [review]
Bug 24488: (QA follow-up) Remove two queries out of foreach cycle

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 143 Jonathan Druart 2021-01-05 08:30:42 UTC
Created attachment 114843 [details] [review]
Bug 24488: (QA follow-up) Move getting items out of cycle

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 144 Jonathan Druart 2021-01-05 08:30:46 UTC
Created attachment 114844 [details] [review]
Bug 24488: Fix count distinct patron on a given hold

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 145 Jonathan Druart 2021-01-05 08:30:51 UTC
Created attachment 114845 [details] [review]
Bug 24488: perf - Group by at DBMS level

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 146 Jonathan Druart 2021-01-05 08:30:55 UTC
Created attachment 114846 [details] [review]
Bug 24488: perf - move patrons_count out of the loop

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 147 Jonathan Druart 2021-01-05 08:31:00 UTC
Created attachment 114847 [details] [review]
Bug 24488: Simplify structure passed to the template

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 148 Jonathan Druart 2021-01-05 08:31:04 UTC
Created attachment 114848 [details] [review]
Bug 24488: perf - Move holds search out of the loop

This will need attention from QA.
We are assuming that the hold with the highest priority has priority=1

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 149 Jonathan Druart 2021-01-05 08:31:09 UTC
Created attachment 114849 [details] [review]
Bug 24488: (QA follow-up) Go to next loop as early as possible

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 150 Jonathan Druart 2021-01-05 08:31:13 UTC
Created attachment 114850 [details] [review]
Bug 24488: create a loop to avoid copy paste
Comment 151 Jonathan Druart 2021-01-05 08:32:34 UTC
Patches rebased.

I have provided a small enhancement on bug 27131 to move some code to a method.
I don't think we could do more here, if you look at the code it read good and is pretty trivial (it's just a matter a few ->search calls).

I have done all what I could do get this ready, hopefully it will be approved and pushed soon :)

Sending back to the NQA queue.
Comment 152 Marcel de Rooy 2021-02-05 08:29:21 UTC
(In reply to Jonathan Druart from comment #151)
> Patches rebased.
> 
> I have provided a small enhancement on bug 27131 to move some code to a
> method.
> I don't think we could do more here, if you look at the code it read good
> and is pretty trivial (it's just a matter a few ->search calls).
> 
> I have done all what I could do get this ready, hopefully it will be
> approved and pushed soon :)
> 
> Sending back to the NQA queue.

My first impression here is: I knew that the query was ugly but replacing it by a huge number of DBIx calls will probably degrade the performance significantly. And if we make so many changes in a script, we should really go to a module.
Comment 153 Marcel de Rooy 2021-02-05 08:36:55 UTC
        my ( $k, $f ) = each %$fff )

Clear variables names btw :)
Comment 154 Marcel de Rooy 2021-02-05 08:53:32 UTC
QA: Still looking here
Comment 155 Marcel de Rooy 2021-02-05 10:28:30 UTC
Created attachment 116362 [details] [review]
Bug 24488: Show correct first patron details on Holds to pull

This patch rewrites the complex search query to reduce the risk of
reordering the results multiple times.

This patch includes a change to schema files so may need to upgrade
schema and/or restart memcached

To test:
1) Reproduce problem following test plan in Description
2) Apply patch and refresh page
3) Notice the correct patron is now shown
4) Play with date selection, confirm correct results are still shown
5) Test cancelling holds
6) Test filtering table results
7) Test with biblios with multiple items
8) Test with making items unavailable (i.e. not for loan, checked out)

Sponsored by: IHC New Zealand

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 156 Marcel de Rooy 2021-02-05 10:28:35 UTC
Created attachment 116363 [details] [review]
Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 157 Marcel de Rooy 2021-02-05 10:28:40 UTC
Created attachment 116364 [details] [review]
Bug 24488: Display patron with the highest priority hold

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 158 Marcel de Rooy 2021-02-05 10:28:45 UTC
Created attachment 116365 [details] [review]
Bug 24488: Simplify searches

It also:
Use dtf for date handling
add missing td

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 159 Marcel de Rooy 2021-02-05 10:28:51 UTC
Created attachment 116366 [details] [review]
Bug 24488: [DISCUSSION] For comparaison

I'd like someone to run a benchmark with and without this patch on a
quite big database and see which approach is the best

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 160 Marcel de Rooy 2021-02-05 10:28:55 UTC
Created attachment 116367 [details] [review]
Bug 24488: (QA follow-up): Fix small bugs, add checkk for waiting holds

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 161 Marcel de Rooy 2021-02-05 10:29:01 UTC
Created attachment 116368 [details] [review]
Bug 24488: (QA follow-up) Make pending reserves faster

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 162 Marcel de Rooy 2021-02-05 10:29:07 UTC
Created attachment 116369 [details] [review]
Bug 24488: (QA follow-up) Remove two queries out of foreach cycle

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 163 Marcel de Rooy 2021-02-05 10:29:12 UTC
Created attachment 116370 [details] [review]
Bug 24488: (QA follow-up) Move getting items out of cycle

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 164 Marcel de Rooy 2021-02-05 10:29:18 UTC
Created attachment 116371 [details] [review]
Bug 24488: Fix count distinct patron on a given hold

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 165 Marcel de Rooy 2021-02-05 10:29:25 UTC
Created attachment 116372 [details] [review]
Bug 24488: perf - Group by at DBMS level

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 166 Marcel de Rooy 2021-02-05 10:29:31 UTC
Created attachment 116373 [details] [review]
Bug 24488: perf - move patrons_count out of the loop

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 167 Marcel de Rooy 2021-02-05 10:29:37 UTC
Created attachment 116374 [details] [review]
Bug 24488: Simplify structure passed to the template

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 168 Marcel de Rooy 2021-02-05 10:29:43 UTC
Created attachment 116375 [details] [review]
Bug 24488: perf - Move holds search out of the loop

This will need attention from QA.
We are assuming that the hold with the highest priority has priority=1

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 169 Marcel de Rooy 2021-02-05 10:29:49 UTC
Created attachment 116376 [details] [review]
Bug 24488: (QA follow-up) Go to next loop as early as possible

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 170 Marcel de Rooy 2021-02-05 10:29:54 UTC
Created attachment 116377 [details] [review]
Bug 24488: create a loop to avoid copy paste

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 171 Marcel de Rooy 2021-02-05 10:31:58 UTC
Some timings from the browser:

500 holds -> only 6 selected
150 ms OLD
300 ms  NEW

2500 holds -> 40 selected
500 ms OLD
800 ms NEW

20000 holds -> 1000 selected
Gateway timeouts or unclear results.
Comment 172 Marcel de Rooy 2021-02-05 10:32:38 UTC
In view of the attention given already, passing QA. This should get further follow-up. Move essential code into a module, test it.

The original query was terrible, but the new code is far from ideal too. The performance has been degraded. Hard to say when it is no longer acceptable.
Comment 173 Jonathan Druart 2021-02-05 10:34:56 UTC
(In reply to Marcel de Rooy from comment #152)
> (In reply to Jonathan Druart from comment #151)
> > Patches rebased.
> > 
> > I have provided a small enhancement on bug 27131 to move some code to a
> > method.
> > I don't think we could do more here, if you look at the code it read good
> > and is pretty trivial (it's just a matter a few ->search calls).
> > 
> > I have done all what I could do get this ready, hopefully it will be
> > approved and pushed soon :)
> > 
> > Sending back to the NQA queue.
> 
> My first impression here is: I knew that the query was ugly but replacing it
> by a huge number of DBIx calls will probably degrade the performance
> significantly. And if we make so many changes in a script, we should really
> go to a module.

Have you seen bug 27131?

Thanks for the review, Marcel!
Comment 174 Jonathan Druart 2021-02-05 10:53:50 UTC
Created attachment 116384 [details] [review]
Bug 24488: create a loop to avoid copy paste

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

JD amended patch: Rename variables
Comment 175 Jonathan Druart 2021-02-08 13:57:18 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 176 Jonathan Druart 2021-02-09 09:50:51 UTC
*** Bug 22431 has been marked as a duplicate of this bug. ***
Comment 177 Fridolin Somers 2021-02-12 11:27:45 UTC
Do you think this is safe for backport on 20.11.x ?
Comment 178 Jonathan Druart 2021-02-12 11:30:40 UTC
(In reply to Fridolin Somers from comment #177)
> Do you think this is safe for backport on 20.11.x ?

No, it's too big.
Comment 179 Fridolin Somers 2021-02-12 13:01:29 UTC
Not backported to 20.11.x