If you add or change an authorized value linked to an item field, you will not be able to use your changed authorized value under Plack without resetting the Plack server. Possibly relevant warnings: Subroutine AuthorizedValuesForCategory redefined at /home/jcamins/kohaclone/admin/authorised_values.pl line 31. Subroutine default_form redefined at /home/jcamins/kohaclone/admin/authorised_values.pl line 238.
Jared, do you mean you can't use the value on modify item/add item screen?
I tested authorized values on the item editing screen. I just did a bit more testing, and it looks like the problem might actually be with caching on the item editing screen and not the administration screen as I had thought.
Attempting to reproduce: * Plack setup with 10 workers for the staff client. * Go to add an item to a biblio (to populate caches.) * Added a new value in LOC, "TOASTER" * Selected New -> Item again on the biblio. * "Toaster" showed up in shelving location. * Went to "Items" tab, selected an item to edit. * "Toaster" did _not_ appear in the shelving location. * After some other unidentifiable messing around, it did appear. Definitely seems to be a bug.
Ah, I did manage to see the issue when adding a new item too. It may depend what worker you end up hitting. This is good, as otherwise it made no sense at all.
Ah yes, there is a local cache for GetMarcStructure. It is also supposed to be memoised, but I don't see any result of that in memcache, which is strange to me. I think the way to fix this is to: * add a convenient ClearFrameworkCache function and call that from places like the framework editor, the authorised values editor, and so on, * remove the memoisation of the function (which I've found tends to be slower than the local cache anyway, when it works), and * add a cache timeout (5 minutes?) so that it won't screw up long-term if we fail to clear the cache somewhere, but still long enough that it'll likely come out of the woodwork as a bug. This'll also let us tide things over when under load.
Actually, I may have spoken too soon there. It doesn't seem to actually be storing authorised values in there. So while that caching should be fixed, this particular instance might be coming from somewhere else.
Oh, GetAuthorisedValues is memoised using a method that just sticks it in a hash. That'll be it. So, there is the Memoize::Expire package that we can plug in and give authorised values a low timeout. Probably something like 10 seconds so that it'll cover that request and not much more. We can also force a clear when we set something, so that the current process will see the update immediately.
Created attachment 26786 [details] [review] Bug 9967 - expire cached GetAuthorisedValues Adds Memoize::Expire to the caching for GetAuthorisedValues so that updates actually show up in a persistent environment (e.g. Plack) NOTE: This is an experimental patch only. It doesn't work in my testing for no reason I can see. It's for review in case someone can see what silly mistake I made.
According to the docs, this patch _should_ work, but it's not. I've verified that it's not actually expiring the cache. However I don't know why, so if someone can have a quick look and see what I've missed, that'd be handy. If no one else can work it out, I'll add a simple hand-rolled cache to it instead.
(In reply to Robin Sheat from comment #9) 1/ In AddAuthorisedValue() there is a call to flush_cache('AddAuthorisedValue') instead of flush_cache('GetAuthorisedValue') - probably irrelevant. 2/ There are some Memoize and Memoize::Expire bugs reported in CPAN, perl monks etc. which are still unresolved since last available version (1.03) was published, e.g.: http://www.perlmonks.org/index.pl?node_id=1049426 . 3/ I'm wondering if memoizing GetAuthorisedValue is such a good idea in the persistent environment.. Same thing for any subroutines which return references of any kind - unless we are 100% sure than the result data structures are always used in strictly read-only mode. Looks like it's not a case for GetAuthorisedValue(): it returns reference to array of hashrefs, and it's result is often passed directly to C4::Templates::param() in various scripts. C4::Templates::output() will then traverse this result and utf8::encode() any scalar values in the data structures. Subsequent GetAuthorisedValue() calls will return the same, memoized arrayref but with contents which got possibly altered by previous request. Not pretty :).
(In reply to Jacek Ablewicz from comment #10) > 1/ In AddAuthorisedValue() there is a call to > flush_cache('AddAuthorisedValue') instead of > flush_cache('GetAuthorisedValue') - probably irrelevant. Doh! But not the source of this issue. > 2/ There are some Memoize and Memoize::Expire bugs reported in CPAN, perl > monks etc. which are still unresolved since last available version (1.03) > was published, e.g.: http://www.perlmonks.org/index.pl?node_id=1049426 . Yeah, that's a bit fishy. > 3/ I'm wondering if memoizing GetAuthorisedValue is such a good idea in the > persistent environment.. Same thing for any subroutines which return > references of any kind - unless we are 100% sure than the result data > structures are always used in strictly read-only mode. Looks like it's not a > case for GetAuthorisedValue(): it returns reference to array of hashrefs, Well, it's something that gets called a lot so we need some optimisation. Really what should happen is that you ask for all the authorised values in a category, and then use the result of that for the rest of your scope. However I don't plan on refactoring that right now :) What I'll do instead is make some simple caching functions, including the ability to a deep copy to avoid the need for a structure to never be modified. This is something that'll come up more and more with plack. I'd still expect it to be faster than a bazillion database requests, and it'll be safe enough with persistence (and can be extended to use memcache in the future should that be considered worthwhile.) Thanks.
> What I'll do instead is make some simple caching functions, including the > ability to a deep copy to avoid the need for a structure to never be ...Or allow Koha::Cache to do this sort of thing.
I've spun off the improvement of Koha::Cache into its own bug (bug 12041), as it was getting bigger.
Created attachment 26910 [details] [review] Bug 9967 - Cache authorised value requests properly Authorised values are now cached using the proper Koha::Cache mechanism, rather than a simple internal cache. Memoize has been removed because it didn't really work like we needed it to. Test plan: * running a persistent environment: * load the edit item screen * refresh several times to ensure any process-based caches are filled * add a new LOC authorised value as fast as possible (prepare it on another tab.) * refresh the edit item page * note that the new LOC value may or may not be showing in the "shelving location" * if more than 5 seconds have passed since saving it, it must now show up. * refresh a few times to ensure that it's showing on all processes. Note: * This patch depends on the caching changes of 9967.
Created attachment 27383 [details] [review] Bug 9967 - Cache authorised value requests properly Authorised values are now cached using the proper Koha::Cache mechanism, rather than a simple internal cache. Memoize has been removed because it didn't really work like we needed it to. Test plan: * running a persistent environment: * load the edit item screen * refresh several times to ensure any process-based caches are filled * add a new LOC authorised value as fast as possible (prepare it on another tab.) * refresh the edit item page * note that the new LOC value may or may not be showing in the "shelving location" * if more than 5 seconds have passed since saving it, it must now show up. * refresh a few times to ensure that it's showing on all processes. Note: * This patch depends on the caching changes of 9967.
Resetting severity to normal - Plack support is still experimental.
Created attachment 28412 [details] [review] [signed-off] Bug 9967 - Cache authorised value requests properly Authorised values are now cached using the proper Koha::Cache mechanism, rather than a simple internal cache. Memoize has been removed because it didn't really work like we needed it to. Test plan: * running a persistent environment: * load the edit item screen * refresh several times to ensure any process-based caches are filled * add a new LOC authorised value as fast as possible (prepare it on another tab.) * refresh the edit item page * note that the new LOC value may or may not be showing in the "shelving location" * if more than 5 seconds have passed since saving it, it must now show up. * refresh a few times to ensure that it's showing on all processes. Note: * This patch depends on the caching changes of 9967. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Robin, don't you think the $branch_limit variable should be part of the cache key?
(In reply to Jonathan Druart from comment #18) > Robin, don't you think the $branch_limit variable should be part of the > cache key? Yes, you're totally right. I'll make a followup.
BTW you've probably noticed already, but when I say: This patch depends on the caching changes of 9967. I actually mean: This patch depends on the caching changes of 12041.
Created attachment 28625 [details] [review] Bug 9967 - include $branch_limit in the cache key Make the caching more correct. Also removes a warn that got left behind.
Created attachment 31726 [details] [review] Bug 9967 - Cache authorised value requests properly Authorised values are now cached using the proper Koha::Cache mechanism, rather than a simple internal cache. Memoize has been removed because it didn't really work like we needed it to. Test plan: * running a persistent environment: * load the edit item screen * refresh several times to ensure any process-based caches are filled * add a new LOC authorised value as fast as possible (prepare it on another tab.) * refresh the edit item page * note that the new LOC value may or may not be showing in the "shelving location" * if more than 5 seconds have passed since saving it, it must now show up. * refresh a few times to ensure that it's showing on all processes. Note: * This patch depends on the caching changes of 9967. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 32086 [details] [review] Bug 9967 - Cache authorised value requests properly Authorised values are now cached using the proper Koha::Cache mechanism, rather than a simple internal cache. Memoize has been removed because it didn't really work like we needed it to. Test plan: * running a persistent environment: * load the edit item screen * refresh several times to ensure any process-based caches are filled * add a new LOC authorised value as fast as possible (prepare it on another tab.) * refresh the edit item page * note that the new LOC value may or may not be showing in the "shelving location" * if more than 5 seconds have passed since saving it, it must now show up. * refresh a few times to ensure that it's showing on all processes. Note: * This patch depends on the caching changes of 9967. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 32087 [details] [review] Bug 9967 - include $branch_limit in the cache key Make the caching more correct. Also removes a warn that got left behind. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Brendan signed the patches off. I confirm the bug and that the patches fix it. No regression found. Marked as Passed QA.
Patches pushed to master. Thanks Robin!