|
Description
David Gustafsson
2022-10-18 13:41:26 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 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). Created attachment 142156 [details] [review] Bug 31856: Don't fetch additional field valudes if there are no fields Created attachment 142157 [details] [review] Bug 31856: Add test for get_description_by_category_and_authorised_value Updated the assignee.. Is there a trick or script we could use to add a lot of subscriptions? Have been pondering how to best test this. 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 Created attachment 150652 [details] [review] Bug 31856: Don't fetch additional field valudes if there are no fields Created attachment 150653 [details] [review] Bug 31856: Add test for get_description_by_category_and_authorised_value Patches rebased against master. Is the change in SearchSubscriptions really makes things faster? (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? Created attachment 151077 [details] [review] Bug 31856: Prefetch additional fields for increased performance Created attachment 151078 [details] [review] Bug 31856: Refactor GetAuthorisedValueDesc (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. (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. 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. What's the purpose of this line?
632 $subscriptions_by_id{$field_value->record_id}->{additional_fields}->{$field_name} = $field_value->value;
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. (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. Is it possible to provide a sample file with a large number of serial records and items, rather than manually creating them? This would make it much easier to test using koha-testing-docker (KTD). |