Bug 30860 - Performance: Add option for CanBookBeReserved to return all item values
Summary: Performance: Add option for CanBookBeReserved to return all item values
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords: rel_23_11_candidate
Depends on: 33302
Blocks: 7923
  Show dependency treegraph
 
Reported: 2022-05-27 12:19 UTC by Nick Clemens
Modified: 2023-08-03 22:05 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.02


Attachments
Bug 30860: Add return_all_items parameter to CanBookBeReserved (2.07 KB, patch)
2022-05-27 13:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30860: Unit test (2.92 KB, patch)
2022-05-27 13:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30860: Return all items from CanBookBeReserved on request.pl (3.16 KB, patch)
2022-05-27 13:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30860: Get all items info for CanBookBeReserved on opac-reserve.pl (1.93 KB, patch)
2022-05-27 13:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30860: Add return_all_items parameter to CanBookBeReserved (2.07 KB, patch)
2022-12-07 18:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30860: Unit test (2.87 KB, patch)
2022-12-07 18:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30860: Return all items from CanBookBeReserved on request.pl (3.15 KB, patch)
2022-12-07 18:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30860: Get all items info for CanBookBeReserved on opac-reserve.pl (1.81 KB, patch)
2022-12-07 18:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30860: Add return_all_items parameter to CanBookBeReserved (2.13 KB, patch)
2022-12-08 20:07 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 30860: Unit test (2.93 KB, patch)
2022-12-08 20:07 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 30860: Return all items from CanBookBeReserved on request.pl (3.21 KB, patch)
2022-12-08 20:07 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 30860: Get all items info for CanBookBeReserved on opac-reserve.pl (1.87 KB, patch)
2022-12-08 20:08 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 30860: Add return_all_items parameter to CanBookBeReserved (2.07 KB, patch)
2023-01-23 15:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30860: Unit test (2.87 KB, patch)
2023-01-23 15:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30860: Return all items from CanBookBeReserved on request.pl (3.15 KB, patch)
2023-01-23 15:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30860: Get all items info for CanBookBeReserved on opac-reserve.pl (1.78 KB, patch)
2023-01-23 15:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30860: Add return_all_items parameter to CanBookBeReserved (2.07 KB, patch)
2023-01-23 15:25 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 30860: Unit test (2.93 KB, patch)
2023-01-23 15:25 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 30860: Return all items from CanBookBeReserved on request.pl (3.21 KB, patch)
2023-01-23 15:25 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 30860: Get all items info for CanBookBeReserved on opac-reserve.pl (1.83 KB, patch)
2023-01-23 15:25 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 30860: (follow-up) Remove repeated variable declaration (904 bytes, patch)
2023-03-03 13:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30860: [alternative patch] Cache CanItemBeReserved return value (10.74 KB, patch)
2023-03-22 15:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30860: Cache CanItemBeReserved return value (11.53 KB, patch)
2023-03-28 10:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30860: Cache CanItemBeReserved return value (11.53 KB, patch)
2023-04-04 10:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30860: Cache CanItemBeReserved return value (11.59 KB, patch)
2023-04-09 15:56 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2022-05-27 12:19:52 UTC
When placing a request we call CanBookBeReserved and loop items, calling CanItemBeReserved until we ifnd one that can fill a hold.

We then loop CanItemBeReserved to check which items can be held.

In some cases it would be nice to get the return values for all items passed back and avoid needing to call again
Comment 1 Nick Clemens 2022-05-27 13:19:55 UTC
Created attachment 135426 [details] [review]
Bug 30860: Add return_all_items parameter to CanBookBeReserved
Comment 2 Nick Clemens 2022-05-27 13:19:59 UTC
Created attachment 135427 [details] [review]
Bug 30860: Unit test
Comment 3 Nick Clemens 2022-05-27 13:20:03 UTC
Created attachment 135428 [details] [review]
Bug 30860: Return all items from CanBookBeReserved on request.pl

This patch requests all item values from CanBookBeReserved on request.pl

Before this we either:
- Looped every item to find out that book could not be reserved
- Looped until we found an item that could be reserved, then looped all items to get statuses

In the worst case we avoid double processing a single item, in the best case we avoid double
processing all items (if only last on record is holdable)

To test:
1 - Find a record in staff client with several items
2 - Set AllowHoldsOnDamagedItems  to 'Dont allow'
3 - Add a damaged item to record
4 - Set a hold rule to only allow holds form homebranch and ensure record has items from other branches
5 - Setup things to prevent more items from being held
6 - Attempt hold for patron
7 - Note item statuses
8 - Apply patch
9 - Confirm statuses are as they were before
Comment 4 Nick Clemens 2022-05-27 13:20:07 UTC
Created attachment 135429 [details] [review]
Bug 30860: Get all items info for CanBookBeReserved on opac-reserve.pl

Repeat test above on OPAC
Comment 5 Nick Clemens 2022-12-07 18:29:31 UTC
Created attachment 144480 [details] [review]
Bug 30860: Add return_all_items parameter to CanBookBeReserved
Comment 6 Nick Clemens 2022-12-07 18:29:35 UTC
Created attachment 144481 [details] [review]
Bug 30860: Unit test
Comment 7 Nick Clemens 2022-12-07 18:29:39 UTC
Created attachment 144482 [details] [review]
Bug 30860: Return all items from CanBookBeReserved on request.pl

This patch requests all item values from CanBookBeReserved on request.pl

Before this we either:
- Looped every item to find out that book could not be reserved
- Looped until we found an item that could be reserved, then looped all items to get statuses

In the worst case we avoid double processing a single item, in the best case we avoid double
processing all items (if only last on record is holdable)

To test:
1 - Find a record in staff client with several items
2 - Set AllowHoldsOnDamagedItems  to 'Dont allow'
3 - Add a damaged item to record
4 - Set a hold rule to only allow holds form homebranch and ensure record has items from other branches
5 - Setup things to prevent more items from being held
6 - Attempt hold for patron
7 - Note item statuses
8 - Apply patch
9 - Confirm statuses are as they were before
Comment 8 Nick Clemens 2022-12-07 18:29:43 UTC
Created attachment 144483 [details] [review]
Bug 30860: Get all items info for CanBookBeReserved on opac-reserve.pl

Repeat test above on OPAC
Comment 9 ByWater Sandboxes 2022-12-08 20:07:49 UTC
Created attachment 144506 [details] [review]
Bug 30860: Add return_all_items parameter to CanBookBeReserved

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 10 ByWater Sandboxes 2022-12-08 20:07:54 UTC
Created attachment 144507 [details] [review]
Bug 30860: Unit test

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 11 ByWater Sandboxes 2022-12-08 20:07:58 UTC
Created attachment 144508 [details] [review]
Bug 30860: Return all items from CanBookBeReserved on request.pl

This patch requests all item values from CanBookBeReserved on request.pl

Before this we either:
- Looped every item to find out that book could not be reserved
- Looped until we found an item that could be reserved, then looped all items to get statuses

In the worst case we avoid double processing a single item, in the best case we avoid double
processing all items (if only last on record is holdable)

To test:
1 - Find a record in staff client with several items
2 - Set AllowHoldsOnDamagedItems  to 'Dont allow'
3 - Add a damaged item to record
4 - Set a hold rule to only allow holds form homebranch and ensure record has items from other branches
5 - Setup things to prevent more items from being held
6 - Attempt hold for patron
7 - Note item statuses
8 - Apply patch
9 - Confirm statuses are as they were before

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 12 ByWater Sandboxes 2022-12-08 20:08:02 UTC
Created attachment 144509 [details] [review]
Bug 30860: Get all items info for CanBookBeReserved on opac-reserve.pl

Repeat test above on OPAC

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 13 Andrew Fuerste-Henry 2022-12-08 20:13:01 UTC
Whoops, I added a signoff via a sandbox, was about to change the status here, and realized I hadn't tested on the OPAC.

My bib has 12 items. When trying to place a hold via the staff client, 2 items show as holdable. The other 10 are not holdable: 1 is damaged, 1 is not for loan, the other 8 don't match the patron's library.

When trying to place a hold on the OPAC for the same title and patron, I'm told there are no items that can be placed on hold.
Comment 14 Nick Clemens 2023-01-23 13:15:25 UTC
(In reply to Andrew Fuerste-Henry from comment #13)
> Whoops, I added a signoff via a sandbox, was about to change the status
> here, and realized I hadn't tested on the OPAC.
> 
> My bib has 12 items. When trying to place a hold via the staff client, 2
> items show as holdable. The other 10 are not holdable: 1 is damaged, 1 is
> not for loan, the other 8 don't match the patron's library.
> 
> When trying to place a hold on the OPAC for the same title and patron, I'm
> told there are no items that can be placed on hold.

In this case I believe the correct response is no items available on either interface, and that is what I see when I try to recreate. Can you retest, and share a screen shot of the record if you recreate?
Comment 15 Nick Clemens 2023-01-23 15:14:53 UTC
Created attachment 145577 [details] [review]
Bug 30860: Add return_all_items parameter to CanBookBeReserved

Current status: Needs Signoff
Comment 16 Nick Clemens 2023-01-23 15:14:57 UTC
Created attachment 145578 [details] [review]
Bug 30860: Unit test
Comment 17 Nick Clemens 2023-01-23 15:15:01 UTC
Created attachment 145579 [details] [review]
Bug 30860: Return all items from CanBookBeReserved on request.pl

This patch requests all item values from CanBookBeReserved on request.pl

Before this we either:
- Looped every item to find out that book could not be reserved
- Looped until we found an item that could be reserved, then looped all items to get statuses

In the worst case we avoid double processing a single item, in the best case we avoid double
processing all items (if only last on record is holdable)

To test:
1 - Find a record in staff client with several items
2 - Set AllowHoldsOnDamagedItems  to 'Dont allow'
3 - Add a damaged item to record
4 - Set a hold rule to only allow holds form homebranch and ensure record has items from other branches
5 - Setup things to prevent more items from being held
6 - Attempt hold for patron
7 - Note item statuses
8 - Apply patch
9 - Confirm statuses are as they were before
Comment 18 Nick Clemens 2023-01-23 15:15:06 UTC
Created attachment 145580 [details] [review]
Bug 30860: Get all items info for CanBookBeReserved on opac-reserve.pl

Repeat test above on OPAC
Comment 19 ByWater Sandboxes 2023-01-23 15:25:34 UTC
Created attachment 145581 [details] [review]
Bug 30860: Add return_all_items parameter to CanBookBeReserved
Comment 20 ByWater Sandboxes 2023-01-23 15:25:40 UTC
Created attachment 145582 [details] [review]
Bug 30860: Unit test

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 21 ByWater Sandboxes 2023-01-23 15:25:44 UTC
Created attachment 145583 [details] [review]
Bug 30860: Return all items from CanBookBeReserved on request.pl

This patch requests all item values from CanBookBeReserved on request.pl

Before this we either:
- Looped every item to find out that book could not be reserved
- Looped until we found an item that could be reserved, then looped all items to get statuses

In the worst case we avoid double processing a single item, in the best case we avoid double
processing all items (if only last on record is holdable)

To test:
1 - Find a record in staff client with several items
2 - Set AllowHoldsOnDamagedItems  to 'Dont allow'
3 - Add a damaged item to record
4 - Set a hold rule to only allow holds form homebranch and ensure record has items from other branches
5 - Setup things to prevent more items from being held
6 - Attempt hold for patron
7 - Note item statuses
8 - Apply patch
9 - Confirm statuses are as they were before

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 22 ByWater Sandboxes 2023-01-23 15:25:48 UTC
Created attachment 145584 [details] [review]
Bug 30860: Get all items info for CanBookBeReserved on opac-reserve.pl

Repeat test above on OPAC

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 23 Katrin Fischer 2023-02-27 21:22:17 UTC
Can you please take a look?

 FAIL	t/db_dependent/Reserves.t
   FAIL	  valid
	  "my" variable $can_be_all masks earlier declaration in same scope
Comment 24 Nick Clemens 2023-03-03 13:14:24 UTC
Created attachment 147695 [details] [review]
Bug 30860: (follow-up) Remove repeated variable declaration
Comment 25 Jonathan Druart 2023-03-22 14:46:38 UTC
I get the idea, but the implementation is weird. Why don't you simply cache the items holdability info in L1?
Comment 26 Jonathan Druart 2023-03-22 14:49:26 UTC
(In reply to Jonathan Druart from comment #25)
> I get the idea, but the implementation is weird. Why don't you simply cache
> the items holdability info in L1?

"Simply" => It's not as simple as what you wrote. The idea is simple, not the implementation. But maybe it's worth spending time on it?
Comment 27 Nick Clemens 2023-03-22 14:52:25 UTC
(In reply to Jonathan Druart from comment #25)
> I get the idea, but the implementation is weird. Why don't you simply cache
> the items holdability info in L1?

I didn't think it was weird, it seemed straightforward to me. Rather than put in cache, retrieve from cache we just call and get all the values.

I think moving to L1 cache would be a great patch if you want to write it :-)
Comment 28 Jonathan Druart 2023-03-22 14:52:45 UTC
(In reply to Jonathan Druart from comment #26)
> (In reply to Jonathan Druart from comment #25)
> > I get the idea, but the implementation is weird. Why don't you simply cache
> > the items holdability info in L1?
> 
> "Simply" => It's not as simple as what you wrote. The idea is simple, not
> the implementation. But maybe it's worth spending time on it?

I was thinking about the daemon, but maybe we deal with that already (there is a flush in rebuild_zebra). So it may be pretty easy actually.
Comment 29 Jonathan Druart 2023-03-22 15:33:04 UTC
Created attachment 148562 [details] [review]
Bug 30860: [alternative patch] Cache CanItemBeReserved return value

Instead of hacking CanBookBeReserved this patch suggests to simply cache
the return value of CanItemBeReserved that could be then returned *on
demand*
We don't want to introduce side-effects hard to catch from this simple
change, so let's return the cache value only from the 2 scripts we are
dealing with.
Comment 30 Nick Clemens 2023-03-22 18:58:38 UTC
(In reply to Jonathan Druart from comment #29)
> Created attachment 148562 [details] [review] [review]
> Bug 30860: [alternative patch] Cache CanItemBeReserved return value
> 
> Instead of hacking CanBookBeReserved this patch suggests to simply cache
> the return value of CanItemBeReserved that could be then returned *on
> demand*
> We don't want to introduce side-effects hard to catch from this simple
> change, so let's return the cache value only from the 2 scripts we are
> dealing with.

This works, and in initial testing seems to offer improvement.

I have one hesitation

Current:
code calls CanBookBeReserved, which exits on first holdable item,
then we call CanItemBeReserved for every item
Worst case, last item is holdable - for 100 items we call CanItemBeReserved 200 times

My patches:
code calls CanBookBeReserved which in turn calls CanItemBeReserved
for every item. Then we loop over the results for each item
Worst case, we call CanItemBeReserved 100 times - we do this no matter what

Alternate patches:
code calls CanBookBeReserved which exits on first holdable item
we then retrieve any alreayd calculated from cache, then we cache each result as we go forward.
Worst case - first item is holdable - we retrieve a single value from cache, then cache every subsequent CanItemBeReserved call and never use it - though we still only call CanItemBeReserved 100 times max

I don't know what the actual tendency for number of items holdable, but it feels like we can do a lot of caching that we don't necessarily need.
Comment 31 Jonathan Druart 2023-03-23 08:13:40 UTC
(In reply to Nick Clemens from comment #30)
> I don't know what the actual tendency for number of items holdable, but it
> feels like we can do a lot of caching that we don't necessarily need.

We can consider L1 caching insignificant in term of performance (unless we are caching huge structures!).
Comment 32 Nick Clemens 2023-03-28 10:58:29 UTC
Created attachment 148812 [details] [review]
Bug 30860: Cache CanItemBeReserved return value

This patch caches the return value of CanItemBeReserved that could
be then returned *on
demand*
We don't want to introduce side-effects hard to catch from this simple
change, so let's return the cache value only from the 2 scripts we are
dealing with.

This patch requests all item values from CanBookBeReserved on request.pl

Before this we either:
- Looped every item to find out that book could not be reserved
- Looped until we found an item that could be reserved, then looped all items to get statuses

In the worst case we avoid double processing a single item, in the best case we avoid double
processing all items (if only last on record is holdable)

To test:
1 - Find a record in staff client with several items
2 - Set AllowHoldsOnDamagedItems  to 'Dont allow'
3 - Add a damaged item to record
4 - Set a hold rule to only allow holds form homebranch and ensure record has items from other branches
5 - Setup things to prevent more items from being held
6 - Attempt hold for patron
7 - Note item statuses
8 - Apply patch
9 - Confirm statuses are as they were before

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 33 Nick Clemens 2023-04-04 10:25:04 UTC
Created attachment 149124 [details] [review]
Bug 30860: Cache CanItemBeReserved return value

This patch caches the return value of CanItemBeReserved that could
be then returned *on
demand*
We don't want to introduce side-effects hard to catch from this simple
change, so let's return the cache value only from the 2 scripts we are
dealing with.

This patch requests all item values from CanBookBeReserved on request.pl

Before this we either:
- Looped every item to find out that book could not be reserved
- Looped until we found an item that could be reserved, then looped all items to get statuses

In the worst case we avoid double processing a single item, in the best case we avoid double
processing all items (if only last on record is holdable)

To test:
1 - Find a record in staff client with several items
2 - Set AllowHoldsOnDamagedItems  to 'Dont allow'
3 - Add a damaged item to record
4 - Set a hold rule to only allow holds form homebranch and ensure record has items from other branches
5 - Setup things to prevent more items from being held
6 - Attempt hold for patron
7 - Note item statuses
8 - Apply patch
9 - Confirm statuses are as they were before

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 34 Katrin Fischer 2023-04-09 15:56:28 UTC
Created attachment 149377 [details] [review]
Bug 30860: Cache CanItemBeReserved return value

This patch caches the return value of CanItemBeReserved that could
be then returned *on
demand*
We don't want to introduce side-effects hard to catch from this simple
change, so let's return the cache value only from the 2 scripts we are
dealing with.

This patch requests all item values from CanBookBeReserved on request.pl

Before this we either:
- Looped every item to find out that book could not be reserved
- Looped until we found an item that could be reserved, then looped all items to get statuses

In the worst case we avoid double processing a single item, in the best case we avoid double
processing all items (if only last on record is holdable)

To test:
1 - Find a record in staff client with several items
2 - Set AllowHoldsOnDamagedItems  to 'Dont allow'
3 - Add a damaged item to record
4 - Set a hold rule to only allow holds form homebranch and ensure record has items from other branches
5 - Setup things to prevent more items from being held
6 - Attempt hold for patron
7 - Note item statuses
8 - Apply patch
9 - Confirm statuses are as they were before

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 35 Katrin Fischer 2023-04-09 15:57:44 UTC
Also ran prove t/db_dependent/Holds.t t/db_dependent/Circulation.t
Circulation.t gave me some trouble in an unrelated test, but results between master and patched branch match.
Comment 36 Tomás Cohen Arazi 2023-06-23 13:01:37 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 37 Jonathan Druart 2023-07-07 08:35:37 UTC
This is so ugly
Comment 38 Katrin Fischer 2023-07-08 13:52:58 UTC
(In reply to Jonathan Druart from comment #37)
> This is so ugly

Writing that about your own creation?
Comment 39 Jonathan Druart 2023-07-11 08:42:00 UTC
(In reply to Katrin Fischer from comment #38)
> (In reply to Jonathan Druart from comment #37)
> > This is so ugly
> 
> Writing that about your own creation?

I would not date otherwise!
Comment 40 Martin Renvoize 2023-07-17 14:02:42 UTC
Thanks for all the hard work!

Pushed to 23.05.x for the next release
Comment 41 Pedro Amorim 2023-07-18 11:57:11 UTC
Enhancement. Not pushing to 22.11.x.