Bug 31856 - Improve performance of serials subscriptions search
Summary: Improve performance of serials subscriptions search
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Gustafsson
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 33746
  Show dependency treegraph
 
Reported: 2022-10-18 13:41 UTC by David Gustafsson
Modified: 2023-05-25 14:11 UTC (History)
3 users (show)

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


Attachments
Bug 31856: Improve performance of serials subscriptions search (8.03 KB, patch)
2022-10-18 13:48 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 31856: Don't fetch additional field valudes if there are no fields (1.75 KB, patch)
2022-10-19 13:59 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 31856: Add test for get_description_by_category_and_authorised_value (1.65 KB, patch)
2022-10-19 14:11 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 31856: Improve performance of serials subscriptions search (8.05 KB, patch)
2023-05-04 12:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31856: Don't fetch additional field valudes if there are no fields (1.75 KB, patch)
2023-05-04 12:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31856: Add test for get_description_by_category_and_authorised_value (1.90 KB, patch)
2023-05-04 12:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31856: Prefetch additional fields for increased performance (2.62 KB, patch)
2023-05-11 13:19 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 31856: Refactor GetAuthorisedValueDesc (5.22 KB, patch)
2023-05-11 13:19 UTC, David Gustafsson
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Gustafsson 2022-10-18 13:41:26 UTC
Currently the serials subscriptions search is very inefficient for searches producing a large number of results. The current patch introduces caching for authorized value lookups, the retrieval of search results has also been optimized to drastically reduces the number of performed queries and creation of Koha objects resulting in a speedup of about 5.5x.
Comment 1 David Gustafsson 2022-10-18 13:48:22 UTC
Created attachment 142058 [details] [review]
Bug 31856: Improve performance of serials subscriptions search

To test:
1) Perform an advanced search that produces a reasonably large resultset
   for serial subscriptions (preferably at least a couple of hundred)
   and take not of the execution time.
2) Apply the patch.
3) Perform the search again, the execution should now be about one fifth
   of the previous search.
4) Ensure tests in t/db_dependent/AuthorisedValues.t all pass.

Sponsored-by: Gothenburg University Library
Comment 2 David Gustafsson 2022-10-18 14:05:13 UTC
Also removed a test in t/db_dependent/AuthorisedValues.t that I found both not very useful and incorrect as categories where sorting is performed on the database level is compared with the same list of categories sorted in Perl (which does not sort on the same conditions as collation etc is not taken into account).
Comment 3 David Gustafsson 2022-10-19 13:59:49 UTC
Created attachment 142156 [details] [review]
Bug 31856: Don't fetch additional field valudes if there are no fields
Comment 4 David Gustafsson 2022-10-19 14:11:44 UTC
Created attachment 142157 [details] [review]
Bug 31856: Add test for get_description_by_category_and_authorised_value
Comment 5 David Nind 2022-11-01 01:38:39 UTC
Updated the assignee..
Comment 6 Katrin Fischer 2022-11-19 20:22:25 UTC
Is there a trick or script we could use to add a lot of subscriptions? Have been pondering how to best test this.
Comment 7 Jonathan Druart 2023-05-04 12:09:37 UTC
Created attachment 150651 [details] [review]
Bug 31856: Improve performance of serials subscriptions search

To test:
1) Perform an advanced search that produces a reasonably large resultset
   for serial subscriptions (preferably at least a couple of hundred)
   and take not of the execution time.
2) Apply the patch.
3) Perform the search again, the execution should now be about one fifth
   of the previous search.
4) Ensure tests in t/db_dependent/AuthorisedValues.t all pass.

Sponsored-by: Gothenburg University Library
Comment 8 Jonathan Druart 2023-05-04 12:09:41 UTC
Created attachment 150652 [details] [review]
Bug 31856: Don't fetch additional field valudes if there are no fields
Comment 9 Jonathan Druart 2023-05-04 12:09:44 UTC
Created attachment 150653 [details] [review]
Bug 31856: Add test for get_description_by_category_and_authorised_value
Comment 10 Jonathan Druart 2023-05-04 12:09:54 UTC
Patches rebased against master.
Comment 11 Jonathan Druart 2023-05-04 12:13:08 UTC
Is the change in SearchSubscriptions really makes things faster?
Comment 12 Jonathan Druart 2023-05-04 12:17:05 UTC
(In reply to Jonathan Druart from comment #11)
> Is the change in SearchSubscriptions really makes things faster?

Right, there is no FK. Maybe we are just missing an index on a DB column then?
Comment 13 David Gustafsson 2023-05-11 13:19:40 UTC
Created attachment 151077 [details] [review]
Bug 31856: Prefetch additional fields for increased performance
Comment 14 David Gustafsson 2023-05-11 13:19:42 UTC
Created attachment 151078 [details] [review]
Bug 31856: Refactor GetAuthorisedValueDesc
Comment 15 David Gustafsson 2023-05-11 13:24:55 UTC
(In reply to Jonathan Druart from comment #11)
> Is the change in SearchSubscriptions really makes things faster?

Yes, fetching the subsctiption object is quite expensive, so the change (in SearchSubscriptions alone) makes searches about twice as fast. Also decided to prefetch additional values, so now is done one query instead of on per subscripton witch speeds things up an additional 25% or so.
Comment 16 David Gustafsson 2023-05-11 13:27:08 UTC
(In reply to Jonathan Druart from comment #12)
> (In reply to Jonathan Druart from comment #11)
> > Is the change in SearchSubscriptions really makes things faster?
> 
> Right, there is no FK. Maybe we are just missing an index on a DB column
> then?

I tried adding an index for that column and did not make much of a difference, perhaps a few percent at most if even that. 99% of the cost is DBIx, the database query itself is a neglectable part if using and index or not.
Comment 17 David Gustafsson 2023-05-11 13:31:10 UTC
Not saying there should not be an index, perhaps there are use cases where it could matter, my guess is that in the absolute majority of cases the number of rows in that table is small enough that a sequential scan will be about as fast as using an index.
Comment 18 Jonathan Druart 2023-05-16 15:02:56 UTC
What's the purpose of this line?

632             $subscriptions_by_id{$field_value->record_id}->{additional_fields}->{$field_name} = $field_value->value;
Comment 19 Jonathan Druart 2023-05-16 15:05:03 UTC
ok got it.

I will need another review another day, with a fresh brain. We should make the code a bit more readable, I find it a bit obscure at the moment.
Comment 20 David Gustafsson 2023-05-25 14:11:24 UTC
(In reply to Jonathan Druart from comment #18)
> What's the purpose of this line?
> 
> 632            
> $subscriptions_by_id{$field_value->record_id}->{additional_fields}-
> >{$field_name} = $field_value->value;

Additional fields are first prefetched for all subscription. This line assigns each additional field value to the subscription it belongs to, using a hash where subscriptions are indexed by id. It is not obvious to me how this code could be made more readable? Previously additional fields where fetched repeatedly for each subscription, by fetching them all at once there is only one database query instead of one per subscription, which is much more efficient.