We are retrieving the authorised values from the DB whereas they can be stored in a cache.
Created attachment 184893 [details] [review] Bug 40554: find_description
Created attachment 184894 [details] [review] Bug 40554: unsafe not used in set_in_cache
Created attachment 184895 [details] [review] Bug 40554: Use a dedicated namespace for AVs
I believe we can reduce the number of DB hits, particularly when retrieving authorised value descriptions within loops, by using our caching mechanism. Here are my suggestions: 1. Use a cache for single descriptions to avoid one DB fetch per description (see Koha::AuthorisedValues->find_description) 2. Replace ->search with ->find since we now have a unique constraint on (code, value).3. Remove the 5-second expiration in C4::Koha::GetAuthorisedValues. This seems to be a leftover from when we were less confident in our cache invalidation. 4. Use a dedicated namespace for authorised values. The trade-off here is that any modification to a value will clear the entire cache, but this should not be a frequent operation. Is it worth investing more time in this? Who supports this idea? TODO: * Should Koha::ILL::Request->statusalias use search_with_library_limits instead of search? * Can Koha::Patron::Attribute->authorised_value be removed now? LATER: * Remove the in-memory cache only from the Koha::AuthorisedValues->search_* methods and consider using L2 cache. (?)
I have a vague memory of adding/fixing some caching in Koha::AuthorisedValues, so I'm sure there's more that could be done there for sure. Bug 35579 Bug 34051 Maybe those two could be adapted to use the model here instead. I haven't looked at them for a couple of years, so not sure off the top of my head.
(In reply to Jonathan Druart from comment #4) > Is it worth investing more time in this? Who supports this idea? On the surface, it sounds like a good idea. As for investing time, I'm not 100% sure. Personally, I find it difficult to test bugs like this, because they make a lot of changes all at once, so it feels overwhelming trying to test them. If this bug introduced the caching mechanism, added some unit tests, and then changed one or two places and had a detailed test plan, then it would feel doable testing it, and then follow-up reports could be added for other .pm/.pl files which depend on this report. Does that answer the question?
(In reply to David Cook from comment #6) > (In reply to Jonathan Druart from comment #4) > > Is it worth investing more time in this? Who supports this idea? > > On the surface, it sounds like a good idea. > > As for investing time, I'm not 100% sure. Personally, I find it difficult to > test bugs like this, because they make a lot of changes all at once, so it > feels overwhelming trying to test them. Yes it's not the kind of patches we should test all the occurrences. There are only few different patterns (here mostly is: opac or not, default to blank or not, and that's all. If you test a couple of each then a code review is enough. > If this bug introduced the caching mechanism, added some unit tests, and > then changed one or two places and had a detailed test plan, then it would > feel doable testing it, and then follow-up reports could be added for other > .pm/.pl files which depend on this report. > > Does that answer the question? The caching mechanism is already half implemented. The idea here is to adjust and reuse it for the different AV fetches. I don't like the 1 bug report per file pattern for this kind of changes: it always ends up half done, and it's hard to review. Especially when we change something in the middle of the tree and we need to adjust what has been done already. That and the dependency tree that is a nightmare to maintain. However we can implement the concepts incrementally: start with find_description, then replace search with find, add caching mechanism for single fetches, implement L2 when only L1 is used, etc. But here we are not really talking about huge changes in term of diff :)