In some of our Koha:: objects we have methods that cache their result and return it in subsequent calls. However there is no invalidation of the cache if the object is modified. For instance, in Koha/ArticleRequest.pm sub biblio { my ($self) = @_; $self->{_biblio} ||= Koha::Biblios->find( $self->biblionumber() ); return $self->{_biblio}; } This pattern exists in several places. It can lead to confusion and incorrect results, such as: use Koha::ArticleRequests; my $ar = Koha::ArticleRequest->new({ borrowernumber => 42, biblionumber => 42, })->store; say $ar->biblio->biblionumber; # Display 42, correct $ar->set({ biblionumber => 24 })->store; say $ar->biblio->biblionumber; # Display 42, wrong $ar->discard_changes; say $ar->biblio->biblionumber; # Display 42, wrong $ar->delete; We should remove those caching and rely on DBIC/DBMS caching mechanism instead.
No Discussion here, is the status correct? :)
(In reply to Katrin Fischer from comment #1) > No Discussion here, is the status correct? :) Yes, it's still expecting feedback from other devs.
I actually thought we'd alr Ady made he decision to not cache like this. I've not been actively removing such caches but I have generally tried to avoid adding new ones for a while. There are some rare case where this style of caching does make sense to me.. though I I couldn't point you at one off the top of my head... The prevent public embed fetching stuff in the API perhaps? Either way, do we think it's time to start retrospectively cleaning up the existing cases? It could make a good hackfest project to bike them all together.
Created attachment 147499 [details] [review] Bug 32894: Remove wrong caching from Koha:: methods - simple In some of our Koha:: objects we have methods that cache their result and return it in subsequent calls. However there is no invalidation of the cache if the object is modified. For instance, in Koha/ArticleRequest.pm sub biblio { my ($self) = @_; $self->{_biblio} ||= Koha::Biblios->find( $self->biblionumber() ); return $self->{_biblio}; } This pattern exists in several places. It can lead to confusion and incorrect results, such as: use Koha::ArticleRequests; my $ar = Koha::ArticleRequest->new({ borrowernumber => 42, biblionumber => 42, })->store; say $ar->biblio->biblionumber; # Display 42, correct $ar->set({ biblionumber => 24 })->store; say $ar->biblio->biblionumber; # Display 42, wrong $ar->discard_changes; say $ar->biblio->biblionumber; # Display 42, wrong $ar->delete; We should remove those caching and rely on DBIC/DBMS caching mechanism instead. This patch is adjusting the trivial occurrences
Created attachment 147500 [details] [review] Bug 32894: Koha::Item->last_returned_by
Created attachment 147502 [details] [review] Bug 32894: Koha::Biblio->biblioitem Can we do better here?
Created attachment 147503 [details] [review] Bug 32894: Fix test
Those patches deal with most of the cases. What about Koha::Item::_bundle_items_cached and Koha::Objects? I would not touch Koha::Objects behaviour here, maybe on a follow-up bug.
Martin, could you have a look at Koha::Item::_bundle_items_cached?
Yes, sorry.. I've just not had a moment to breath yet.. This is certainly still near the top of my list.
Created attachment 150016 [details] [review] Bug 32894: Remove wrong caching from Koha:: methods - simple In some of our Koha:: objects we have methods that cache their result and return it in subsequent calls. However there is no invalidation of the cache if the object is modified. For instance, in Koha/ArticleRequest.pm sub biblio { my ($self) = @_; $self->{_biblio} ||= Koha::Biblios->find( $self->biblionumber() ); return $self->{_biblio}; } This pattern exists in several places. It can lead to confusion and incorrect results, such as: use Koha::ArticleRequests; my $ar = Koha::ArticleRequest->new({ borrowernumber => 42, biblionumber => 42, })->store; say $ar->biblio->biblionumber; # Display 42, correct $ar->set({ biblionumber => 24 })->store; say $ar->biblio->biblionumber; # Display 42, wrong $ar->discard_changes; say $ar->biblio->biblionumber; # Display 42, wrong $ar->delete; We should remove those caching and rely on DBIC/DBMS caching mechanism instead. This patch is adjusting the trivial occurrences Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 150017 [details] [review] Bug 32894: Koha::Item->last_returned_by Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 150018 [details] [review] Bug 32894: Koha::Biblio->biblioitem Can we do better here? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 150019 [details] [review] Bug 32894: Fix test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 150020 [details] [review] Bug 32894: Remove incorrect caching from bundle_items Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 150021 [details] [review] Bug 32894: (QA follow-up) Fix typo Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Added a tiny typo fix and also a patch for item_bundles. I agree.. lets to Koha::Objects as it's own bug. Signing off
Something is not right with Koha::Item::last_returned_by It returns a patron object, but the relationship is to ItemsLastBorrower and the call to set it doesn't work prove -v t/db_dependent/Circulation/StoreLastBorrower.t Also Koha/Virtualshelfshare.pm : everywhere else we assign resultset to rs, then create new - it isn't wrong, just inconsistent
Created attachment 150827 [details] [review] Bug 32894: (follow-up) Koha::Item->last_returned_by
Created attachment 150828 [details] [review] Bug 32894: Make ->sharee looks like others
(In reply to Nick Clemens from comment #18) > Something is not right with Koha::Item::last_returned_by > > It returns a patron object, but the relationship is to ItemsLastBorrower and > the call to set it doesn't work > > prove -v t/db_dependent/Circulation/StoreLastBorrower.t Oops! Thanks! > Also Koha/Virtualshelfshare.pm : > everywhere else we assign resultset to rs, then create new - it isn't wrong, > just inconsistent Done.
Created attachment 150847 [details] [review] Bug 32894: Remove wrong caching from Koha:: methods - simple In some of our Koha:: objects we have methods that cache their result and return it in subsequent calls. However there is no invalidation of the cache if the object is modified. For instance, in Koha/ArticleRequest.pm sub biblio { my ($self) = @_; $self->{_biblio} ||= Koha::Biblios->find( $self->biblionumber() ); return $self->{_biblio}; } This pattern exists in several places. It can lead to confusion and incorrect results, such as: use Koha::ArticleRequests; my $ar = Koha::ArticleRequest->new({ borrowernumber => 42, biblionumber => 42, })->store; say $ar->biblio->biblionumber; # Display 42, correct $ar->set({ biblionumber => 24 })->store; say $ar->biblio->biblionumber; # Display 42, wrong $ar->discard_changes; say $ar->biblio->biblionumber; # Display 42, wrong $ar->delete; We should remove those caching and rely on DBIC/DBMS caching mechanism instead. This patch is adjusting the trivial occurrences Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 150848 [details] [review] Bug 32894: Koha::Item->last_returned_by Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 150849 [details] [review] Bug 32894: Koha::Biblio->biblioitem Can we do better here? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 150850 [details] [review] Bug 32894: Fix test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 150851 [details] [review] Bug 32894: Remove incorrect caching from bundle_items Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 150852 [details] [review] Bug 32894: (QA follow-up) Fix typo Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 150853 [details] [review] Bug 32894: (follow-up) Koha::Item->last_returned_by Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 150854 [details] [review] Bug 32894: Make ->sharee looks like others Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 150855 [details] [review] Bug 32894: (QA follow-up) Fix tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Looks good, t/db_dependent all pass now
I do not think this is a great idea long term. This is moving backwards when it comes to efficiency and will introduce far more database calls. It seems like it might be better to fix the code that is making a bad assumption. The short term fix is to use discard_changes when needed. The long term fix is to use Koha objects from the perl scripts and pass them to subroutines so we have no need to re-fetch data from the database. Have you identified any bugs being caused by this, or it just a possibility as you've shown?
(In reply to Kyle M Hall from comment #32) > I do not think this is a great idea long term. This is moving backwards when > it comes to efficiency and will introduce far more database calls. It seems > like it might be better to fix the code that is making a bad assumption. The > short term fix is to use discard_changes when needed. The long term fix is > to use Koha objects from the perl scripts and pass them to subroutines so we > have no need to re-fetch data from the database. > > Have you identified any bugs being caused by this, or it just a possibility > as you've shown? This is a bad pattern, we should get rid of it and prevent it from propagating over the codebase. The correct way to fix the refetch is patch like bug 32496 and bug 31735. There is also bug 32476 that can be helpful to cache *calculated* values when specific performance problems are identified. Here we are caching only 1 DB call that is already cached by an underlying mechanism anyway (DBMS or DBIC). > It seems like it might be better to fix the code that is making a bad assumption. No need to make bad assumption to get incorrect results, see comment 0.
Can be backported. Let's push early next cycle.
reset_all && multitime -n 10 prove t/db_dependent/Koha Just Master: 1: prove t/db_dependent/Koha Mean Std.Dev. Min Median Max real 129.754 3.631 121.324 130.872 134.770 user 107.146 2.759 100.859 108.025 111.129 sys 17.488 0.720 15.979 17.469 18.793 Master + 32890: 1: prove t/db_dependent/Koha Mean Std.Dev. Min Median Max real 128.499 4.333 123.117 127.620 139.403 user 106.268 3.456 102.384 105.515 114.984 sys 17.237 0.992 15.540 17.337 19.357 I'm no statistician so take from this what you will. I think it's generally positive. Lower average with the patches but a higher standard deviation.
Please rebase.
Created attachment 152086 [details] [review] Bug 32894: Remove wrong caching from Koha:: methods - simple In some of our Koha:: objects we have methods that cache their result and return it in subsequent calls. However there is no invalidation of the cache if the object is modified. For instance, in Koha/ArticleRequest.pm sub biblio { my ($self) = @_; $self->{_biblio} ||= Koha::Biblios->find( $self->biblionumber() ); return $self->{_biblio}; } This pattern exists in several places. It can lead to confusion and incorrect results, such as: use Koha::ArticleRequests; my $ar = Koha::ArticleRequest->new({ borrowernumber => 42, biblionumber => 42, })->store; say $ar->biblio->biblionumber; # Display 42, correct $ar->set({ biblionumber => 24 })->store; say $ar->biblio->biblionumber; # Display 42, wrong $ar->discard_changes; say $ar->biblio->biblionumber; # Display 42, wrong $ar->delete; We should remove those caching and rely on DBIC/DBMS caching mechanism instead. This patch is adjusting the trivial occurrences Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 152087 [details] [review] Bug 32894: Koha::Item->last_returned_by Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 152088 [details] [review] Bug 32894: Koha::Biblio->biblioitem Can we do better here? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 152089 [details] [review] Bug 32894: Fix test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 152090 [details] [review] Bug 32894: Remove incorrect caching from bundle_items Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 152091 [details] [review] Bug 32894: (QA follow-up) Fix typo Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 152092 [details] [review] Bug 32894: (follow-up) Koha::Item->last_returned_by Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 152093 [details] [review] Bug 32894: Make ->sharee looks like others Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 152094 [details] [review] Bug 32894: (QA follow-up) Fix tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Pushed to master for 23.11. Nice work everyone, thanks!
Thanks for all the hard work! Pushed to 23.05.x for the next release
Nice work everyone! Pushed to oldstable for 22.11.x
Stop backporting this.