Bug 11998 - Syspref caching issues
Summary: Syspref caching issues
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
: 15970 15973 (view as bug list)
Depends on:
Blocks: 15341 15342 16104 7172 12173 16044 16067 16068 16070 16088 16105 16758
  Show dependency treegraph
 
Reported: 2014-03-26 06:04 UTC by Robin Sheat
Modified: 2018-06-04 20:10 UTC (History)
18 users (show)

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


Attachments
Bug 11998 - timeout for syspref caching (2.12 KB, patch)
2014-03-26 06:09 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - move most syspref db access into Context.pm (9.40 KB, patch)
2014-03-27 04:47 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - sysprefs in memcached (4.69 KB, patch)
2014-04-01 03:18 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - add test case, fix issue found (3.23 KB, patch)
2014-04-01 04:19 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - add test case, fix issue found (3.35 KB, patch)
2014-04-01 04:22 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - add test case, fix issue found (3.35 KB, patch)
2014-04-22 05:17 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - make syspref caching use Koha::Cache (5.45 KB, patch)
2014-09-22 04:42 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - make syspref caching use Koha::Cache (6.28 KB, patch)
2014-09-22 05:43 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - timeout for syspref caching (2.18 KB, patch)
2014-10-28 21:45 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11998 - move most syspref db access into Context.pm (9.47 KB, patch)
2014-10-28 21:45 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11998 - sysprefs in memcached (4.75 KB, patch)
2014-10-28 21:45 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11998 - add test case, fix issue found (3.42 KB, patch)
2014-10-28 21:45 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11998 - make syspref caching use Koha::Cache (6.35 KB, patch)
2014-10-28 21:45 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11998: QA follow-up - fix the perlcritic error (1000 bytes, patch)
2014-11-04 08:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11998 - fix compile warnings (3.70 KB, patch)
2014-11-11 03:26 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - fix compile warnings (4.52 KB, patch)
2014-11-19 04:06 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - remove logging from places where it shouldn't be (1.23 KB, patch)
2014-11-19 04:07 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998: (RM followup) avoid adding an unneeded workaround (4.89 KB, patch)
2014-11-19 17:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11998 - remove logging from places where it shouldn't be (2.05 KB, patch)
2014-11-20 02:50 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - fix test case so it's testing the cache properly (1.24 KB, patch)
2014-11-20 02:52 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998: Add Koha::Config::SystemPreference (4.32 KB, patch)
2014-11-20 16:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11998 - tidy up the Koha::Config::SystemPreferences module a bit (5.51 KB, patch)
2014-11-24 03:52 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - fix some compile warnings (2.98 KB, patch)
2014-11-26 05:00 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - timeout for syspref caching (2.18 KB, patch)
2015-03-05 04:45 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - move most syspref db access into Context.pm (9.43 KB, patch)
2015-03-05 04:45 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - sysprefs in memcached (4.75 KB, patch)
2015-03-05 04:45 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - add test case, fix issue found (3.41 KB, patch)
2015-03-05 04:45 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - make syspref caching use Koha::Cache (6.34 KB, patch)
2015-03-05 04:46 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998: QA follow-up - fix the perlcritic error (1000 bytes, patch)
2015-03-05 04:46 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - fix compile warnings (4.52 KB, patch)
2015-03-05 04:46 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - remove logging from places where it shouldn't be (2.05 KB, patch)
2015-03-05 04:46 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - fix test case so it's testing the cache properly (1.24 KB, patch)
2015-03-05 04:46 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998: Add Koha::Config::SystemPreference (4.54 KB, patch)
2015-03-05 04:46 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - tidy up the Koha::Config::SystemPreferences module a bit (5.51 KB, patch)
2015-03-05 04:46 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - fix some compile warnings (3.04 KB, patch)
2015-03-05 04:46 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11998 - Syspref caching issues (841 bytes, patch)
2015-03-05 11:38 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 11998 - timeout for syspref caching (2.25 KB, patch)
2015-03-05 13:36 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11998 - move most syspref db access into Context.pm (9.50 KB, patch)
2015-03-05 13:36 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11998 - sysprefs in memcached (4.82 KB, patch)
2015-03-05 13:36 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11998 - add test case, fix issue found (3.48 KB, patch)
2015-03-05 13:36 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11998 - make syspref caching use Koha::Cache (6.41 KB, patch)
2015-03-05 13:37 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11998: QA follow-up - fix the perlcritic error (1.05 KB, patch)
2015-03-05 13:37 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11998 - fix compile warnings (4.59 KB, patch)
2015-03-05 13:37 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11998 - remove logging from places where it shouldn't be (2.12 KB, patch)
2015-03-05 13:37 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11998 - fix test case so it's testing the cache properly (1.31 KB, patch)
2015-03-05 13:37 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11998: Add Koha::Config::SystemPreference (4.61 KB, patch)
2015-03-05 13:37 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11998 - tidy up the Koha::Config::SystemPreferences module a bit (5.58 KB, patch)
2015-03-05 13:37 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11998 - fix some compile warnings (3.11 KB, patch)
2015-03-05 13:38 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11998 - Syspref caching issues (889 bytes, patch)
2015-03-05 13:38 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11998: Use Koha::Cache to cache sysprefs (16.08 KB, patch)
2016-03-03 17:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11998: Use Koha::Cache to cache sysprefs (16.14 KB, patch)
2016-03-03 20:25 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11998: Use Koha::Cache to cache sysprefs (16.36 KB, patch)
2016-03-04 08:30 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 11998: Use t::lib::Mocks::mock_preference in tests (58.30 KB, patch)
2016-03-04 13:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11998: Use C4::Context->preference in Koha/Schema/Result/Item.pm (2.33 KB, patch)
2016-03-04 13:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11998: Make HoldsQueue.t tests pass [TO REVERT] (7.69 KB, patch)
2016-03-07 09:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11998: Delay cache intialisation (836 bytes, patch)
2016-03-07 16:24 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 11998: Use Koha::Cache to cache sysprefs (16.36 KB, patch)
2016-03-10 10:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11998: Use t::lib::Mocks::mock_preference in tests (56.62 KB, patch)
2016-03-10 10:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11998: Use C4::Context->preference in Koha/Schema/Result/Item.pm (2.33 KB, patch)
2016-03-10 10:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11998: Make HoldsQueue.t tests pass [TO REVERT] (7.69 KB, patch)
2016-03-10 10:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11998: Do not clear syspref cache in psgi file (1.30 KB, patch)
2016-03-10 10:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11998: Revert "Bug 13805: (Plack) Disable syspref cache" (1.17 KB, patch)
2016-03-10 10:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11998: follow-up for the debian package psgi file (725 bytes, patch)
2016-03-10 10:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11998: Add a L1 cache for sysprefs (3.02 KB, patch)
2016-03-10 10:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11998: Clear L1 cache from psgi files (1.20 KB, patch)
2016-03-10 10:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11998: Clear L1 cache from psgi files (1.12 KB, patch)
2016-03-10 13:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11998: Use Koha::Cache to cache sysprefs (16.41 KB, patch)
2016-03-11 14:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11998: Use t::lib::Mocks::mock_preference in tests (56.65 KB, patch)
2016-03-11 14:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11998: Use C4::Context->preference in Koha/Schema/Result/Item.pm (2.39 KB, patch)
2016-03-11 14:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11998: Make HoldsQueue.t tests pass [TO REVERT] (7.75 KB, patch)
2016-03-11 14:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11998: Do not clear syspref cache in psgi file (1.36 KB, patch)
2016-03-11 14:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11998: Revert "Bug 13805: (Plack) Disable syspref cache" (1.23 KB, patch)
2016-03-11 14:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11998: follow-up for the debian package psgi file (782 bytes, patch)
2016-03-11 14:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11998: Add a L1 cache for sysprefs (3.08 KB, patch)
2016-03-11 14:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11998: Clear L1 cache from psgi files (1.18 KB, patch)
2016-03-11 14:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11998: Use Koha::Cache to cache sysprefs (16.62 KB, patch)
2016-03-14 15:21 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 11998: Use t::lib::Mocks::mock_preference in tests (56.74 KB, patch)
2016-03-14 15:21 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 11998: Use C4::Context->preference in Koha/Schema/Result/Item.pm (2.45 KB, patch)
2016-03-14 15:22 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 11998: Make HoldsQueue.t tests pass [TO REVERT] (7.80 KB, patch)
2016-03-14 15:22 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 11998: Do not clear syspref cache in psgi file (1.42 KB, patch)
2016-03-14 15:22 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 11998: Revert "Bug 13805: (Plack) Disable syspref cache" (1.45 KB, patch)
2016-03-14 15:22 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 11998: follow-up for the debian package psgi file (841 bytes, patch)
2016-03-14 15:22 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 11998: Add a L1 cache for sysprefs (3.14 KB, patch)
2016-03-14 15:22 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 11998: Clear L1 cache from psgi files (1.24 KB, patch)
2016-03-14 15:22 UTC, Jacek Ablewicz
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Sheat 2014-03-26 06:04:22 UTC
Sysprefs are cached in process, which is a problem when you have multiple plack workers.
Comment 1 Robin Sheat 2014-03-26 06:09:11 UTC Comment hidden (obsolete)
Comment 2 Robin Sheat 2014-03-26 06:10:52 UTC
There will be some followups to this: improve the way sysprefs are saved by admin/systempreferences.pl, and to enable storing of sysprefs in memcache in preference to the in-process cache, if it's available.
Comment 3 Robin Sheat 2014-03-27 04:47:23 UTC Comment hidden (obsolete)
Comment 4 Robin Sheat 2014-04-01 03:18:28 UTC Comment hidden (obsolete)
Comment 5 Robin Sheat 2014-04-01 03:21:01 UTC
These patches will need to be treated as a series, they depend on the previous ones.
Comment 6 Robin Sheat 2014-04-01 04:19:19 UTC Comment hidden (obsolete)
Comment 7 Robin Sheat 2014-04-01 04:22:04 UTC Comment hidden (obsolete)
Comment 8 Mark Tompsett 2014-04-07 18:54:49 UTC
Comment on attachment 26621 [details] [review]
Bug 11998 - move most syspref db access into Context.pm

Review of attachment 26621 [details] [review]:
-----------------------------------------------------------------

::: C4/Context.pm
@@ +645,2 @@
>  
> +    $value = 0 if ( $db_type && $db_type eq 'YesNo' && $value eq '' );

What if $value = undef? The eq check will blow up. Should it not be !$value instead?
Comment 9 Mark Tompsett 2014-04-07 19:17:02 UTC
Comment on attachment 26714 [details] [review]
Bug 11998 - sysprefs in memcached

Review of attachment 26714 [details] [review]:
-----------------------------------------------------------------

::: C4/Context.pm
@@ +740,4 @@
>      my $sth = $dbh->prepare("DELETE FROM systempreferences WHERE variable=?");
>      my $res = $sth->execute($var);
>      if ($res) {
> +        C4::Context->set_preference($var, undef);

Isn't this supposed to be a _cache_preference call?
Comment 10 Mark Tompsett 2014-04-07 19:20:05 UTC
Comment on attachment 26717 [details] [review]
Bug 11998 - add test case, fix issue found

Review of attachment 26717 [details] [review]:
-----------------------------------------------------------------

::: C4/Context.pm
@@ +744,4 @@
>      my $sth = $dbh->prepare("DELETE FROM systempreferences WHERE variable=?");
>      my $res = $sth->execute($var);
>      if ($res) {
> +        _cache_preference($var, undef);

Ah, there's the fix. :)
Comment 11 Robin Sheat 2014-04-07 22:27:07 UTC
(In reply to M. Tompsett from comment #8)
> > +    $value = 0 if ( $db_type && $db_type eq 'YesNo' && $value eq '' );
> 
> What if $value = undef? The eq check will blow up. Should it not be !$value
> instead?

That was in the original code, and hadn't blown up that I've seen before. It probably be !$value though.
Comment 12 Robin Sheat 2014-04-22 05:17:39 UTC Comment hidden (obsolete)
Comment 13 Chris Cormack 2014-06-18 20:13:14 UTC
Can someone please sign this off, ... (I can't as its from catalyst)
Comment 14 David Cook 2014-06-25 04:20:25 UTC
I don't use Plack but here's a *bump* anyway...
Comment 15 Jared Camins-Esakov 2014-06-26 13:05:24 UTC
Is there a test plan for this? I can recreate the problem, but not consistently, and it's very difficult to prove the absence of problems with non-deterministic symptoms. Also, is there a reason you're using memcached directly rather than Koha::Cache?
Comment 16 Robin Sheat 2014-06-26 22:38:35 UTC
It's hard to test because whether you get the problem is dependent on what plack worker you end up hitting, so it's inherently non-deterministic (or at least, pretty darn hard to predict.)

As for Koha::Cache, I wrote this before I wrote Koha::Cache. Didn't think to go back and convert it. I might do that.
Comment 17 Robin Sheat 2014-09-22 04:42:18 UTC Comment hidden (obsolete)
Comment 18 Robin Sheat 2014-09-22 05:43:29 UTC Comment hidden (obsolete)
Comment 19 Bernardo Gonzalez Kriegel 2014-10-15 15:15:59 UTC
Robin, there are some errors/warnings reported by qa script

This appears on C4/Context.pm and then on every file that loads it

 FAIL   C4/Context.pm
   OK     pod
   OK     forbidden patterns
   FAIL   valid
                Name "Cache::RemovalStrategy::FIELDS" used only once: possible typo 
                Constant subroutine C4::Context::CONFIG_FNAME redefined 
                Name "Tie::Hash::FIELDS" used only once: possible typo 
                Name "Cache::RemovalStrategy::LRU::FIELDS" used only once: possible typo 
   FAIL   critic
                # Variables::ProhibitConditionalDeclarations: Got 1 violation(s).  

Can't find the reason.
Comment 20 Brendan Gallagher 2014-10-28 21:45:19 UTC Comment hidden (obsolete)
Comment 21 Brendan Gallagher 2014-10-28 21:45:26 UTC Comment hidden (obsolete)
Comment 22 Brendan Gallagher 2014-10-28 21:45:36 UTC Comment hidden (obsolete)
Comment 23 Brendan Gallagher 2014-10-28 21:45:43 UTC Comment hidden (obsolete)
Comment 24 Brendan Gallagher 2014-10-28 21:45:51 UTC Comment hidden (obsolete)
Comment 25 Martin Renvoize 2014-11-03 13:42:44 UTC
QA Comment: Should these db action in Context.pm use the dbic variation now days?

Other than that, the code looks good to me.
Comment 26 Martin Renvoize 2014-11-03 13:55:14 UTC
QA Comment: I also get qa script failure.

 FAIL   C4/Context.pm
   FAIL   critic
                # Variables::ProhibitConditionalDeclarations: Got 1 violation(s).
Comment 27 Martin Renvoize 2014-11-03 14:04:12 UTC
I also get all Bernardo's failures too..
Comment 28 Robin Sheat 2014-11-03 21:20:47 UTC
Is it really coming from Koha::Cache? I haven't had a chance to look yet, but I don't really understand those messages, and without understanding they'll be hard to fix.
Comment 29 Jonathan Druart 2014-11-04 08:03:58 UTC
(In reply to Robin Sheat from comment #28)
> Is it really coming from Koha::Cache? I haven't had a chance to look yet,
> but I don't really understand those messages, and without understanding
> they'll be hard to fix.

koha@koha-qa:~/src (BZ11998⚡)% perl -wc C4/Context.pm
Subroutine handle_errors redefined at C4/Context.pm line 28.
Constant subroutine C4::Context::CONFIG_FNAME redefined at /usr/share/perl/5.20/constant.pm line 156.
Subroutine KOHAVERSION redefined at C4/Context.pm line 214.
Subroutine final_linear_version redefined at C4/Context.pm line 234.
Subroutine read_config_file redefined at C4/Context.pm line 263.
Subroutine ismemcached redefined at C4/Context.pm line 279.
Subroutine memcached redefined at C4/Context.pm line 290.
Subroutine db_scheme2dbi redefined at C4/Context.pm line 309.
Subroutine import redefined at C4/Context.pm line 314.
Subroutine new redefined at C4/Context.pm line 353.
Subroutine set_context redefined at C4/Context.pm line 429.
Subroutine restore_context redefined at C4/Context.pm line 468.
Subroutine _common_config redefined at C4/Context.pm line 499.
Subroutine config redefined at C4/Context.pm line 512.
Subroutine zebraconfig redefined at C4/Context.pm line 515.
Subroutine ModZebrations redefined at C4/Context.pm line 518.
Subroutine preference redefined at C4/Context.pm line 540.
Subroutine boolean_preference redefined at C4/Context.pm line 567.
Subroutine enable_syspref_cache redefined at C4/Context.pm line 583.
Subroutine disable_syspref_cache redefined at C4/Context.pm line 597.
Subroutine clear_syspref_cache redefined at C4/Context.pm line 613.
Subroutine set_preference redefined at C4/Context.pm line 628.
Subroutine delete_preference redefined at C4/Context.pm line 676.
Subroutine AUTOLOAD redefined at C4/Context.pm line 703.
Subroutine Zconn redefined at C4/Context.pm line 725.
Subroutine _new_Zconn redefined at C4/Context.pm line 751.
Subroutine _new_dbh redefined at C4/Context.pm line 813.
Subroutine dbh redefined at C4/Context.pm line 878.
Subroutine new_dbh redefined at C4/Context.pm line 912.
Subroutine set_dbh redefined at C4/Context.pm line 937.
Subroutine restore_dbh redefined at C4/Context.pm line 960.
Subroutine queryparser redefined at C4/Context.pm line 984.
Subroutine _new_queryparser redefined at C4/Context.pm line 1004.
Subroutine marcfromkohafield redefined at C4/Context.pm line 1036.
Subroutine _new_marcfromkohafield redefined at C4/Context.pm line 1052.
Subroutine stopwords redefined at C4/Context.pm line 1077.
Subroutine _new_stopwords redefined at C4/Context.pm line 1093.
Subroutine userenv redefined at C4/Context.pm line 1117.
Subroutine set_userenv redefined at C4/Context.pm line 1139.
Subroutine set_shelves_userenv redefined at C4/Context.pm line 1161.
Subroutine get_shelves_userenv redefined at C4/Context.pm line 1169.
Subroutine _new_userenv redefined at C4/Context.pm line 1196.
Subroutine _unset_userenv redefined at C4/Context.pm line 1213.
Subroutine get_versions redefined at C4/Context.pm line 1230.
Subroutine tz redefined at C4/Context.pm line 1256.
Subroutine IsSuperLibrarian redefined at C4/Context.pm line 1271.
Subroutine interface redefined at C4/Context.pm line 1295.
Name "Tie::Hash::FIELDS" used only once: possible typo at /usr/share/perl/5.20/fields.pm line 135.
Name "Cache::RemovalStrategy::LRU::FIELDS" used only once: possible typo at /usr/share/perl/5.20/fields.pm line 135.
Name "Cache::RemovalStrategy::FIELDS" used only once: possible typo at /usr/share/perl/5.20/fields.pm line 135.
C4/Context.pm syntax OK

koha@koha-qa:~/src (BZ11998⚡)% perlcritic C4/Context.pm
Variable declared in conditional statement at line 544, column 5.  Declare variables outside of the condition.  (Severity: 5)

On master:
koha@koha-qa:~/src (master↓⚡)% perl -wc C4/Context.pm
C4/Context.pm syntax OK

koha@koha-qa:~/src (master↓⚡)% perlcritic C4/Context.pm
C4/Context.pm source OK

The perlcritic error is easy to fix, I can provide a followup, but I don't understand the warnings neither.
Comment 30 Jonathan Druart 2014-11-04 08:07:09 UTC Comment hidden (obsolete)
Comment 31 Robin Sheat 2014-11-09 22:29:31 UTC
I bet it's a circular dependency.

This might be problematic.
Comment 32 Robin Sheat 2014-11-09 22:52:38 UTC
(In reply to Jonathan Druart from comment #30)
> Created attachment 33183 [details] [review] [review]
> Bug 11998: QA follow-up - fix the perlcritic error
> 
> % perlcritic C4/Context.pm
> Variable declared in conditional statement at line 544, column 5.
> Declare variables outside of the condition.  (Severity: 5)

So I was wondering what purpose this change solves, turns out it can be important:

<grantm> my $var if condition    is a messy construct that doesn't do quite what you expect
<grantm> if condition is true, then $var is created in the current scope and gets cleaned up when the scope ends
<grantm> but if condition is not true, then $var does not get created at runtime and instead, the $var that was inferred at compile time is used instead
<grantm> it ends up being a file-scoped global that persists a value
<Ned> ahh
<Ned> that's a bit of a nasty side-effect :p
<grantm> actually, it's not a file-scoped global
<grantm> it's a lexical and the sub is a closure over that lexical
<grantm> the thing about 'my' is that it has compile-time and run-time effects and in this instance the run-time effects only get to happen if condition is true
<grantm> historical note: this bug/feature was how people used to do state variables before the 'state' keyword existed
<Ned> heh
<grantm> specifically: my $state_variable = 0 if 0;
<Ned> there should be a word for that
<Ned> "fug" perhaps
<grantm> it's certainly fugly
Comment 33 Tomás Cohen Arazi 2014-11-10 18:22:49 UTC
Is it possible that we strip the C4::Log circular dependency out for now?

We could even think of a proper Koha::SystemPreferences class to move this functions to in a short term (post-release). But we definitely need the bugfix, and logging syspref changes could wait if needed (it is actually logged by the /svc/config/systempref.... service).
Comment 34 Robin Sheat 2014-11-11 03:26:23 UTC Comment hidden (obsolete)
Comment 35 Robin Sheat 2014-11-11 21:43:56 UTC
(In reply to Robin Sheat from comment #34)
> Created attachment 33447 [details] [review] [review]
> Bug 11998 - fix compile warnings
> 
> This creates a simplified logging system that can be used by
> C4::Context.

Ignore that, there's a bug in it.
Comment 36 Tomás Cohen Arazi 2014-11-11 21:44:54 UTC
(In reply to Robin Sheat from comment #35)
> (In reply to Robin Sheat from comment #34)
> > Created attachment 33447 [details] [review] [review] [review]
> > Bug 11998 - fix compile warnings
> > 
> > This creates a simplified logging system that can be used by
> > C4::Context.
> 
> Ignore that, there's a bug in it.

Robin, just strip it out. We'll properly implement it as you wanted, later.
Comment 37 Robin Sheat 2014-11-19 04:06:32 UTC Comment hidden (obsolete)
Comment 38 Robin Sheat 2014-11-19 04:07:45 UTC Comment hidden (obsolete)
Comment 39 Robin Sheat 2014-11-19 04:08:43 UTC
OK, implemented in a good-enough-for-now sorta way that improves things overall.
Comment 40 Tomás Cohen Arazi 2014-11-19 16:51:41 UTC
I don't understand why we would add a transitional logging facility we don't need if we can just remove the logging introduced by this patches, that is not needed.

It seems straightforward to think we should have a Koha::SystemPreferences class soon that can do the loggin properly without introducing circular dependencies. That is out of the scope of this bug, for sure.

Also, are we sure we need global variables?

I was just about to post counter patches (attributed to Robin) that just remove the introduction of logaction here, but I thought it would be better to just write this lines here.
Comment 41 Tomás Cohen Arazi 2014-11-19 17:34:47 UTC Comment hidden (obsolete)
Comment 42 Tomás Cohen Arazi 2014-11-19 17:51:27 UTC
BTW, please take a look at t/db_dependent/Context.t failing with this patches.
Comment 43 Robin Sheat 2014-11-19 22:42:43 UTC
(In reply to Tomás Cohen Arazi from comment #40)
> I don't understand why we would add a transitional logging facility we don't
> need if we can just remove the logging introduced by this patches, that is
> not needed.
> 
> It seems straightforward to think we should have a Koha::SystemPreferences
> class soon that can do the loggin properly without introducing circular
> dependencies. That is out of the scope of this bug, for sure.
> 
> Also, are we sure we need global variables?
> 
> I was just about to post counter patches (attributed to Robin) that just
> remove the introduction of logaction here, but I thought it would be better
> to just write this lines here.

Well my thinking was:
1) Logging the same thing in multiple places is bad and wrong and should be removed whenever the chance arises.
2) C4::Context should have the facility to do logging
3) This centralises things which makes it easier to implement an even better Koha::SystemPreferences module later.
Comment 44 Robin Sheat 2014-11-19 22:49:26 UTC
(In reply to Tomás Cohen Arazi from comment #41)
> I'm still concerned about the addition of global variables in favour of a
> more OO design.

What global variable do you mean?
Comment 45 Robin Sheat 2014-11-19 23:15:21 UTC
I forgot to add: I don't really care about these details :) I'd rather see it in sooner rather than later, the issues now are very much on the level of being fixable later, whereas the patch is actually fixing a real bug.
Comment 46 Tomás Cohen Arazi 2014-11-20 00:16:09 UTC
Comment on attachment 32874 [details] [review]
Bug 11998 - make syspref caching use Koha::Cache

>+my $syspref_cache = Koha::Cache->get_instance();
>+my $use_syspref_cache = 1;

I was talking about this, which I thought could be problematic for Plack, but as I said I'm all ears on Plack.
Comment 47 Robin Sheat 2014-11-20 00:21:56 UTC
(In reply to Tomás Cohen Arazi from comment #46)
> >+my $syspref_cache = Koha::Cache->get_instance();
> >+my $use_syspref_cache = 1;
> 
> I was talking about this, which I thought could be problematic for Plack,
> but as I said I'm all ears on Plack.

This is fine, the first case because it's a cache and so it's allowed to be on instance per thread, that's the whole point of it. The second because it's manipulating the cache setting. In general, I think it's only used by test cases and possibly by scripts running standalone.
Comment 48 Robin Sheat 2014-11-20 02:50:43 UTC Comment hidden (obsolete)
Comment 49 Robin Sheat 2014-11-20 02:52:03 UTC Comment hidden (obsolete)
Comment 50 Jonathan Druart 2014-11-20 15:08:31 UTC
(In reply to Robin Sheat from comment #48)
> Created attachment 33724 [details] [review] [review]
> Bug 11998 - remove logging from places where it shouldn't be
> 
> This prevents duplicate action_log entries when a preference is updated.

But... Is there at least one left now?
I don't see where the log is done after this patch.
Comment 51 Jonathan Druart 2014-11-20 15:11:10 UTC
After all these patches applied, there is an unused variable $logtype in C4::Context::set_preference.
Comment 52 Jonathan Druart 2014-11-20 16:15:55 UTC Comment hidden (obsolete)
Comment 53 Jonathan Druart 2014-11-20 16:16:44 UTC
(In reply to Jonathan Druart from comment #52)
> Created attachment 33746 [details] [review] [review]
> Bug 11998: Add Koha::Config::SystemPreference

I hope this patch won't add more confusion.
Feel free to obsolete it if not needed.
Comment 54 Robin Sheat 2014-11-20 21:28:06 UTC
(In reply to Jonathan Druart from comment #50)
> But... Is there at least one left now?
> I don't see where the log is done after this patch.

Oh, it wasn't clear: mine were an alternative to Tomás's patch. If you put them both together, you have no logging.
Comment 55 Robin Sheat 2014-11-20 21:29:14 UTC
(In reply to Jonathan Druart from comment #52)
> Created attachment 33746 [details] [review] [review]
> Bug 11998: Add Koha::Config::SystemPreference
> 
> This (dirty) patch extract the syspref set and delete routine into a
> module.
> It fixes the circular dependency and the logging problem.

That's probably the better solution. Though, the caching should happen inside the module too. Eventually this means we could move away from using C4::Context->preference, but for now we maintain a compatibility shim.
Comment 56 Jonathan Druart 2014-11-21 08:40:00 UTC
(In reply to Robin Sheat from comment #55)
> (In reply to Jonathan Druart from comment #52)
> > Created attachment 33746 [details] [review] [review] [review]
> > Bug 11998: Add Koha::Config::SystemPreference
> > 
> > This (dirty) patch extract the syspref set and delete routine into a
> > module.
> > It fixes the circular dependency and the logging problem.
> 
> That's probably the better solution. Though, the caching should happen
> inside the module too. Eventually this means we could move away from using
> C4::Context->preference, but for now we maintain a compatibility shim.

The idea was to have a quick and safe patch for 3.18.
The SystemPreference module will have to be rewritten in a OO way.
Comment 57 Robin Sheat 2014-11-21 09:10:57 UTC
(In reply to Jonathan Druart from comment #56)
> 
> The idea was to have a quick and safe patch for 3.18.
> The SystemPreference module will have to be rewritten in a OO way.

fair enough. I'd add comments to that effect. Especially about the caching, given a an small amount of technical debt is being added.
Comment 58 Bernardo Gonzalez Kriegel 2014-11-21 11:36:08 UTC
Seems to be many people interested in this, but it's not clear (for me) how to test.

Besides that, koha-qa still complains

koha-qa -c 11
testing 11 commit(s) (applied to b2649cb 'Bug 12750: koha-create should be able')

fatal: ambiguous argument 'C4/LogStatic.pm': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions
 FAIL   C4/Context.pm
 FAIL   Koha/Config/SystemPreference.pm
 FAIL   t/db_dependent/Context.t
 FAIL   admin/preferences.pl
 OK     C4/LogStatic.pm
 FAIL   t/db_dependent/Circulation.t
 FAIL   t/db_dependent/check_sysprefs.t
 FAIL   t/db_dependent/sysprefs.t
 FAIL   admin/systempreferences.pl
Comment 59 Robin Sheat 2014-11-24 03:19:12 UTC
Comment on attachment 33685 [details] [review]
Bug 11998: (RM followup) avoid adding an unneeded workaround

Marking Tomás' patch obsolete, as things are being refactored more cleanly.
Comment 60 Robin Sheat 2014-11-24 03:52:24 UTC Comment hidden (obsolete)
Comment 61 Robin Sheat 2014-11-24 03:55:16 UTC
Added docs and a copyright header to Koha::Config::SystemPreference, because nothing should ever get in without those.

 OK	C4/Context.pm
 OK	Koha/Config/SystemPreference.pm
 OK	t/db_dependent/Context.t
 OK	admin/preferences.pl
 OK	C4/LogStatic.pm
 OK	t/db_dependent/Circulation.t
 OK	t/db_dependent/check_sysprefs.t
 OK	t/db_dependent/sysprefs.t
 OK	admin/systempreferences.pl
 OK	C4/Circulation.pm
 OK	circ/returns.pl
 OK	t/db_dependent/Circulation/Returns.t
Comment 62 Jonathan Druart 2014-11-24 10:00:48 UTC
Robin, I still got the compilation error:

% more Koha/Pouet.pm
package Koha::Pouet;

use Koha::Database;

1;

% perl -wc Koha/Pouet.pm
Name "Cache::RemovalStrategy::FIELDS" used only once: possible typo at /usr/share/perl/5.20/fields.pm line 135.
Name "Cache::RemovalStrategy::LRU::FIELDS" used only once: possible typo at /usr/share/perl/5.20/fields.pm line 135.
Name "Tie::Hash::FIELDS" used only once: possible typo at /usr/share/perl/5.20/fields.pm line 135.
Koha/Pouet.pm syntax OK

Actually, it comes from the use Koha::Cache in C4::Context introduced by this patch set.
Comment 63 Robin Sheat 2014-11-26 02:37:35 UTC
Adding the Koha::Config::SystemPreferences module doesn't actually solve this problem, it just makes it harder to find. We still have a circular dependency on C4::Context.

I'm going to see if I can fix that for a bit. If not, going to roll it all back to my static logger.
Comment 64 Robin Sheat 2014-11-26 05:00:52 UTC Comment hidden (obsolete)
Comment 65 Martin Renvoize 2015-01-06 07:33:42 UTC
So... I'm a tad confused, are we back at a point where I can test these patches for signoff..?
Comment 66 Jacek Ablewicz 2015-02-05 12:24:39 UTC
(In reply to Robin Sheat from comment #64)
> Created attachment 33940 [details] [review] [review]
> Bug 11998 - fix some compile warnings
> 
> This lazy-loads C4::Context and some other modules so that they can be
> be safely used by C4::Context without introducing problematic circular
> dependencies.

This approach may be somehow problematic IMO.. Looks like it relies on the unwritten assumption that 'use C4::Context' would be always used somewhere beforehand at least once - because if not, usually there will be no default/already existing context to rely upon. Currently, initial context is most often created & stored as default in the import() function in C4::Context (and typically only there ?). Take this part for example:

--- a/Koha/Database.pm	
+++ a/Koha/Database.pm	
@@ -34,7 +34,6 @@ Koha::Database
 
 use Modern::Perl;
 use Carp;
-use C4::Context;
 use Koha::Schema;
 use base qw(Class::Accessor);
 
@@ -46,6 +45,7 @@ __PACKAGE__->mk_accessors(qw( ));
 # returns it.
 sub _new_schema {
 
+    require C4::Context;
     my $context = C4::Context->new();

with it applied, some simple code chunks like that:

use Koha::Database;
my $scnt = Koha::Database->new()->schema()->resultset('Subscription')->count();

would fail ("... Can't call method "config" on unblessed reference at /home/koha/devkohaclone/C4/Context.pm line 780").

It's possible that this particular issue is being addressed in some other patch[es] belonging to this bug report, but it's hard to say for sure, as the whole patch set does not apply on the current master at the moment.
Comment 67 Dobrica Pavlinusic 2015-03-03 10:45:32 UTC
In plack discussion we decided to use this patch as starting point for plack sysprefs problems, but it doesn't apply for me:

Applying: Bug 11998 - move most syspref db access into Context.pm
fatal: sha1 information is lacking or useless (C4/Context.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 11998 - move most syspref db access into Context.pm

This one is quite large, so I'm reluctant to apply it by hand. Robin, can you please rebase it?
Comment 68 Robin Sheat 2015-03-05 04:45:30 UTC Comment hidden (obsolete)
Comment 69 Robin Sheat 2015-03-05 04:45:36 UTC Comment hidden (obsolete)
Comment 70 Robin Sheat 2015-03-05 04:45:44 UTC Comment hidden (obsolete)
Comment 71 Robin Sheat 2015-03-05 04:45:50 UTC Comment hidden (obsolete)
Comment 72 Robin Sheat 2015-03-05 04:46:03 UTC Comment hidden (obsolete)
Comment 73 Robin Sheat 2015-03-05 04:46:08 UTC Comment hidden (obsolete)
Comment 74 Robin Sheat 2015-03-05 04:46:14 UTC Comment hidden (obsolete)
Comment 75 Robin Sheat 2015-03-05 04:46:19 UTC Comment hidden (obsolete)
Comment 76 Robin Sheat 2015-03-05 04:46:25 UTC Comment hidden (obsolete)
Comment 77 Robin Sheat 2015-03-05 04:46:31 UTC Comment hidden (obsolete)
Comment 78 Robin Sheat 2015-03-05 04:46:37 UTC Comment hidden (obsolete)
Comment 79 Robin Sheat 2015-03-05 04:46:43 UTC Comment hidden (obsolete)
Comment 80 Dobrica Pavlinusic 2015-03-05 11:38:14 UTC Comment hidden (obsolete)
Comment 81 Brendan Gallagher 2015-03-05 13:36:24 UTC Comment hidden (obsolete)
Comment 82 Brendan Gallagher 2015-03-05 13:36:32 UTC Comment hidden (obsolete)
Comment 83 Brendan Gallagher 2015-03-05 13:36:38 UTC Comment hidden (obsolete)
Comment 84 Brendan Gallagher 2015-03-05 13:36:47 UTC Comment hidden (obsolete)
Comment 85 Brendan Gallagher 2015-03-05 13:37:02 UTC Comment hidden (obsolete)
Comment 86 Brendan Gallagher 2015-03-05 13:37:20 UTC Comment hidden (obsolete)
Comment 87 Brendan Gallagher 2015-03-05 13:37:28 UTC Comment hidden (obsolete)
Comment 88 Brendan Gallagher 2015-03-05 13:37:35 UTC Comment hidden (obsolete)
Comment 89 Brendan Gallagher 2015-03-05 13:37:42 UTC Comment hidden (obsolete)
Comment 90 Brendan Gallagher 2015-03-05 13:37:52 UTC Comment hidden (obsolete)
Comment 91 Brendan Gallagher 2015-03-05 13:37:58 UTC Comment hidden (obsolete)
Comment 92 Brendan Gallagher 2015-03-05 13:38:04 UTC Comment hidden (obsolete)
Comment 93 Brendan Gallagher 2015-03-05 13:38:11 UTC Comment hidden (obsolete)
Comment 94 Jonathan Druart 2015-03-06 13:51:48 UTC
Could be "solved" by bug 13805.
Comment 95 Katrin Fischer 2015-06-08 11:19:06 UTC
We need to get moving on this, but there are conflicts in Context.pm.
Please rebase!
Comment 96 Katrin Fischer 2015-09-13 11:43:00 UTC
I am not sure about the current state of this - are the current patches still valid?
Comment 97 Jonathan Druart 2016-03-03 17:11:09 UTC Comment hidden (obsolete)
Comment 98 Jonathan Druart 2016-03-03 17:30:23 UTC
Patches squashed, rebased and adapted to current master.
I have rewritten and it seems to work as expected.
See also bug 15970, bug 15341 and bug 15873, to know why we need this patch.

Note that we still have 
Name "Cache::RemovalStrategy::FIELDS" used only once: possible typo at /usr/share/perl/5.20/fields.pm line 135.
Name "Cache::RemovalStrategy::LRU::FIELDS" used only once: possible typo at /usr/share/perl/5.20/fields.pm line 135.
Name "Tie::Hash::FIELDS" used only once: possible typo at /usr/share/perl/5.20/fields.pm line 135.

Forget them, unless you know how to fix them.
Comment 99 Chris Cormack 2016-03-03 20:25:00 UTC Comment hidden (obsolete)
Comment 100 Chris Cormack 2016-03-03 20:29:33 UTC
Please note I have tested this under CGI, caching works fine, but it still needs testing under plack.
Comment 101 Josef Moravec 2016-03-04 08:30:47 UTC Comment hidden (obsolete)
Comment 102 Jonathan Druart 2016-03-04 09:32:19 UTC
(In reply to Josef Moravec from comment #101)
> Tested with plack with syspref cache enabled, there is some time between
> setting the syspref and applying it, but it takes just one reload of page,
> it shouldn't be problem, should it?

Interesting, I have not noticed that.
If it's just a couple of seconds, I am sure it is fine :)
Comment 103 Josef Moravec 2016-03-04 09:52:04 UTC
It looks like it is just 1 or 2 seconds max, so it is absolutely OK ;)
Comment 104 Jacek Ablewicz 2016-03-04 11:42:36 UTC
Code itself looks fine to me, but I'm a bit worried about some potential problems this patch may introduce indirectly. As it would be now possible to set up permanent & shared cache for system preferences (memcache, fastmmap), in such installations we may expect some extra troubles, e.g. with:

- bunch of t/* tests (quite o lot of them) is still using ->set_preference() insted of mock_preference() - they don't commit changes to the database, but running them may now result in syspref "cache poisoning", affecting behaviour of the other processes

- remaining code chunks which are dealing with systempreferences table directly (installer, updatedatabase.pl, some tests and admin/maintenance scripts); updatedatabase.pl seems to be OK (at least at the 1st glance), not sure about installer - it does call clear_syspref_cache() 1+ time, but not necessarily everywhere where it should do it / not at the each and every step (?)

- up to now, when a given script retrieved some preference once, it was guaranted that all subsequent ->preference() calls during the script run will return the same value; now it will always use the current value instead - I guess sometimes that will be a good thing, and sometimes not so much - but probably not very important in practical circumstances

Also there will be some (hopefully very) little performace hit due to caching system overheads (and 10 sec deafult expire in case of Cache::Memory - but it will affect only the long-running scripts) - how little, it remains to be seen ;)
Comment 105 Jonathan Druart 2016-03-04 13:33:22 UTC Comment hidden (obsolete)
Comment 106 Jonathan Druart 2016-03-04 13:33:27 UTC Comment hidden (obsolete)
Comment 107 Jonathan Druart 2016-03-04 13:36:45 UTC
(In reply to Jacek Ablewicz from comment #104)
> - bunch of t/* tests (quite o lot of them) is still using ->set_preference()
> insted of mock_preference() - they don't commit changes to the database, but
> running them may now result in syspref "cache poisoning", affecting
> behaviour of the other processes

Yes, that is addressed in the last 2 patches.

> - remaining code chunks which are dealing with systempreferences table
> directly (installer, updatedatabase.pl, some tests and admin/maintenance
> scripts); updatedatabase.pl seems to be OK (at least at the 1st glance), not
> sure about installer - it does call clear_syspref_cache() 1+ time, but not
> necessarily everywhere where it should do it / not at the each and every
> step (?)

Yes, we should replace them, and avoid to update/insert sysprefs during the update DB process.

> - up to now, when a given script retrieved some preference once, it was
> guaranted that all subsequent ->preference() calls during the script run
> will return the same value; now it will always use the current value instead
> - I guess sometimes that will be a good thing, and sometimes not so much -
> but probably not very important in practical circumstances

Theoretically it could be an issue but, as you said, I don't think it could be a problem in practice.

> Also there will be some (hopefully very) little performace hit due to
> caching system overheads (and 10 sec deafult expire in case of Cache::Memory
> - but it will affect only the long-running scripts) - how little, it remains
> to be seen ;)

In conjunction with other patches (depending on this bug report), I am expecting a gain.
Comment 108 Jonathan Druart 2016-03-04 13:37:20 UTC
(In reply to Jonathan Druart from comment #105)
> Created attachment 48687 [details] [review] [review]
> Bug 11998: Use t::lib::Mocks::mock_preference in tests
> 
> Unless in t/db_dependent/Context.t where we want to test the cache
> behaviors.

Note that tests in t/db_dependent/HoldsQueue.t  do not pass, but I have no idea why!
Comment 109 Jonathan Druart 2016-03-04 13:40:56 UTC
(In reply to Jonathan Druart from comment #108)
> (In reply to Jonathan Druart from comment #105)
> > Created attachment 48687 [details] [review] [review] [review]
> > Bug 11998: Use t::lib::Mocks::mock_preference in tests
> > 
> > Unless in t/db_dependent/Context.t where we want to test the cache
> > behaviors.
> 
> Note that tests in t/db_dependent/HoldsQueue.t  do not pass, but I have no
> idea why!

If you use this file (modified by this patch) and prove it on top of master, they don't pass neither.
Comment 110 Kyle M Hall 2016-03-04 14:40:01 UTC
At some point in the tests, StaticHoldsQueueWeight becomes zero for reasons unknown and the unit tests rely on this bug!

t/db_dependent/HoldsQueue.t ..
1..26
ok 1 - use C4::Reserves;
ok 2 - use C4::HoldsQueue;
1 SET StaticHoldsQueueWeight: IggZam,xND8TnZydP,lfyZhGooM9 at t/db_dependent/HoldsQueue.t line 74.
StaticHoldsQueueWeight: IggZam,xND8TnZydP,lfyZhGooM9 at /home/vagrant/kohaclone/C4/HoldsQueue.pm line 620.
ok 3 - take from homebranch (don't use cost matrix) pick up branch
ok 4 - take from homebranch (don't use cost matrix) holding branch
ok 5 - take from homebranch (use cost matrix) pick up branch
ok 6 - take from homebranch (use cost matrix) holding branch
StaticHoldsQueueWeight: IggZam,xND8TnZydP,lfyZhGooM9 at /home/vagrant/kohaclone/C4/HoldsQueue.pm line 620.
ok 7 - take from holdingbranch (don't use cost matrix) pick up branch
ok 8 - take from holdingbranch (don't use cost matrix) holding branch
ok 9 - take from holdingbranch (use cost matrix) pick up branch
ok 10 - take from holdingbranch (use cost matrix) holding branch
StaticHoldsQueueWeight: IggZam,xND8TnZydP,lfyZhGooM9 at /home/vagrant/kohaclone/C4/HoldsQueue.pm line 620.
ok 11 - take from lowest cost branch (don't use cost matrix) pick up branch
ok 12 - take from lowest cost branch (don't use cost matrix) holding branch
ok 13 - take from lowest cost branch (use cost matrix) pick up branch
ok 14 - take from lowest cost branch (use cost matrix) holding branch
ok 15 - GetHoldsQueueItems
ok 16 - item type included in queued items list (bug 5825)
ok 17 - C4::HoldsQueue::least_cost_branch returns the local branch if it is in the list of branches to pull from
StaticHoldsQueueWeight: 0 at /home/vagrant/kohaclone/C4/HoldsQueue.pm line 620.
ok 18 - Holds queue filling correct number for default holds policy 'from home library'
ok 19 - Holds queue filling 1st correct hold for default holds policy 'from home library'
ok 20 - Holds queue filling 2nd correct hold for default holds policy 'from home library'
ok 21 - Is today a holiday for pickup branch
StaticHoldsQueueWeight: 0 at /home/vagrant/kohaclone/C4/HoldsQueue.pm line 620.
ok 22 - Holds not filled with items from closed libraries
StaticHoldsQueueWeight: 0 at /home/vagrant/kohaclone/C4/HoldsQueue.pm line 620.
ok 23 - Holds queue filling correct number for holds for default holds policy 'from any library'
StaticHoldsQueueWeight: 0 at /home/vagrant/kohaclone/C4/HoldsQueue.pm line 620.
ok 24 - Holds not filled with items from closed libraries
2 SET StaticHoldsQueueWeight: wVjTn,QwXVJjV,IhgDeror at t/db_dependent/HoldsQueue.t line 395.
StaticHoldsQueueWeight: 0 at /home/vagrant/kohaclone/C4/HoldsQueue.pm line 620.
ok 25 - Bug 14297 - Holds Queue building ignoring holds where pickup & home branch don't match and item is not from le
ok 26 - Bug 15062 - Holds queue with Transport Cost Matrix will transfer item even if transfers disabled
Comment 111 Kyle M Hall 2016-03-04 14:47:36 UTC
This must be a caching issue. If I simply make this change:

ok( exists($queue_item->{itype}), 'item type included in queued items list (bug 5825)' );
+warn "TEST: " . C4::Context->preference("StaticHoldsQueueWeight");

The tests begin to fail on master! I think the only reasonable solution is to mock the actual expected value for the time being and write a replacement for HoldsQueue.t from the ground up in time.
Comment 112 Jonathan Druart 2016-03-07 09:27:40 UTC Comment hidden (obsolete)
Comment 113 Jacek Ablewicz 2016-03-07 12:08:34 UTC
For sysprefs with undef value in the database - while testing with memcache - there are 2 warnings generated:

Use of uninitialized value in subroutine entry at /home/koha/devkohaclone/Koha/Cache.pm line 287.
Use of uninitialized value in subroutine entry at /home/koha/devkohaclone/Koha/Cache.pm line 287.

This only happens when trying to fetch such syspref first time, from unpopulated cache. There are 9 such values in my test database:

   CoceHost
   CoceProviders
   DefaultLongOverdueChargeValue
   DefaultLongOverdueDays
   DefaultLongOverdueLostValue
   MembershipExpiryDaysNotice
   NovelistSelectPassword
   NovelistSelectProfile
   TagsExternalDictionary

Non-existing preference fetch generates the same warning (but again only once).

Note that subsequent C4::Context->preference() calls will return empty string for such preferences, instead of undef value - not sure if that may cause some problems or not, but it's a change from previous behaviour. Also it does not happen for Cache::Memory (= default caching system after this patch).
Comment 114 Jacek Ablewicz 2016-03-07 12:55:11 UTC
I'm unable to test this patch set with fastmmap caching system - it seems to break it somehow.. With patch, and fastmmap cache enabled, anything (?) that uses Koha::Cache results with:

Use of uninitialized value $db_name in concatenation (.) or string at /home/koha/devkohaclone/Koha/Database.pm line 73.
Use of uninitialized value $db_host in concatenation (.) or string at /home/koha/devkohaclone/Koha/Database.pm line 73.
DBI connect('database=;host=;port=','',...) failed: Access denied for user 'koha'@'localhost' (using password: NO) at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1249
DBIx::Class::Storage::DBI::dbh(): DBI Connection failed: Access denied for user 'koha'@'localhost' (using password: NO) at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1253. at /home/koha/devkohaclone/Koha/Database.pm line 90

To reproduce (on command line)

0) ensure that you have fastmmap perl module installed (libcache-fastmmap-perl in debian)
1) apply patch (without patch: no error in current master)
2) export CACHING_SYSTEM=fastmmap
3) export GATEWAY_INTERFACE=1
4) try to run this simple script:

#!/usr/bin/perl

use Modern::Perl;
use C4::Biblio;
GetMarcStructure();
Comment 115 Jonathan Druart 2016-03-07 13:41:34 UTC
(In reply to Jacek Ablewicz from comment #114)
> I'm unable to test this patch set with fastmmap caching system - it seems to
> break it somehow.. With patch, and fastmmap cache enabled, anything (?) that
> uses Koha::Cache results with:
> 
> Use of uninitialized value $db_name in concatenation (.) or string at
> /home/koha/devkohaclone/Koha/Database.pm line 73.
> Use of uninitialized value $db_host in concatenation (.) or string at
> /home/koha/devkohaclone/Koha/Database.pm line 73.
> DBI connect('database=;host=;port=','',...) failed: Access denied for user
> 'koha'@'localhost' (using password: NO) at
> /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1249
> DBIx::Class::Storage::DBI::dbh(): DBI Connection failed: Access denied for
> user 'koha'@'localhost' (using password: NO) at
> /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1253. at
> /home/koha/devkohaclone/Koha/Database.pm line 90
> 
> To reproduce (on command line)
> 
> 0) ensure that you have fastmmap perl module installed
> (libcache-fastmmap-perl in debian)
> 1) apply patch (without patch: no error in current master)
> 2) export CACHING_SYSTEM=fastmmap
> 3) export GATEWAY_INTERFACE=1
> 4) try to run this simple script:
> 
> #!/usr/bin/perl
> 
> use Modern::Perl;
> use C4::Biblio;
> GetMarcStructure();

I don't get the error. I don't know if I did something wrong, I don't use fastmmap (I thought we agree it does not have good results).

% git diff
diff --git a/Koha/Cache.pm b/Koha/Cache.pm
index 9856e80..e0f406f 100644
--- a/Koha/Cache.pm
+++ b/Koha/Cache.pm
@@ -123,6 +123,7 @@ sub new {
 
     $ENV{DEBUG} && carp "Selected caching system: " . ($self->{'cache'} // 'none');
 
+    use Data::Dumper;warn Dumper $self;
     return
       bless $self,
       $class;

% more t.pl
#!/usr/bin/perl
use Modern::Perl;
use C4::Biblio;
GetMarcStructure();

% perl t.pl
$VAR1 = {
          'memory_cache' => bless( {
                                     'size_limit' => undef,
                                     'namespace' => 'koha',
                                     'load_callback' => undef,
                                     'default_expires' => '0 sec',
                                     'removal_strategy' => bless( {}, 'Cache::RemovalStrategy::LRU' ),
                                     'validate_callback' => undef
                                   }, 'Cache::Memory' ),
          'timeout' => 0,
          'namespace' => 'koha',
          'Cache::Memory_get' => sub { "DUMMY" },
          'cache' => bless( {
                              'Cache' => \205625248,
                              'allow_recursive' => undef,
                              'share_file' => '/tmp/sharefile-koha-koha-localhost-koha',
                              'write_back' => '',
                              'context' => undef,
                              'delete_cb' => undef,
                              'empty_on_exit' => 0,
                              'num_pages' => 89,
                              'compress' => 0,
                              'enable_stats' => 0,
                              'raw_values' => 0,
                              'write_cb' => undef,
                              'read_cb' => undef,
                              'cache_not_found' => undef,
                              'pid' => 17602,
                              'expire_time' => 0,
                              'page_size' => 65536,
                              'unlink_on_exit' => 0,
                              'cache_size' => 5832704
                            }, 'Cache::FastMmap' ),
          'fastmmap_cache' => $VAR1->{'cache'},
          'Cache::Memory_set' => sub { "DUMMY" },
          'default_type' => 'fastmmap'
Comment 116 Jonathan Druart 2016-03-07 13:45:41 UTC
Sorry, forget this last comment (I was using master... :D)
Comment 117 Jonathan Druart 2016-03-07 14:07:35 UTC
(In reply to Jacek Ablewicz from comment #114)
> I'm unable to test this patch set with fastmmap caching system - it seems to
> break it somehow.. With patch, and fastmmap cache enabled, anything (?) that
> uses Koha::Cache results with:
> 
> Use of uninitialized value $db_name in concatenation (.) or string at
> /home/koha/devkohaclone/Koha/Database.pm line 73.
> Use of uninitialized value $db_host in concatenation (.) or string at
> /home/koha/devkohaclone/Koha/Database.pm line 73.
> DBI connect('database=;host=;port=','',...) failed: Access denied for user
> 'koha'@'localhost' (using password: NO) at
> /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1249
> DBIx::Class::Storage::DBI::dbh(): DBI Connection failed: Access denied for
> user 'koha'@'localhost' (using password: NO) at
> /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1253. at
> /home/koha/devkohaclone/Koha/Database.pm line 90
> 
> To reproduce (on command line)
> 
> 0) ensure that you have fastmmap perl module installed
> (libcache-fastmmap-perl in debian)
> 1) apply patch (without patch: no error in current master)
> 2) export CACHING_SYSTEM=fastmmap
> 3) export GATEWAY_INTERFACE=1
> 4) try to run this simple script:
> 
> #!/usr/bin/perl
> 
> use Modern::Perl;
> use C4::Biblio;
> GetMarcStructure();

It works if I revert
commit 9e701294dd6ccf4fa5d8b24d1a7da15be353992e
    Bug 13431 - Shared FastMmap file causes issues
No idea why!
Comment 118 Jonathan Druart 2016-03-07 14:10:12 UTC
And also this one!!
Comment 119 Jonathan Druart 2016-03-07 14:12:58 UTC
Actually it's certainly caused by the circular dependency.
Comment 120 Jonathan Druart 2016-03-07 14:15:59 UTC
(In reply to Jonathan Druart from comment #119)
> Actually it's certainly caused by the circular dependency.

Koha::Cache::_initialize_fastmmap could read the config file instead of calling C4::Context->config.
But the question is: Does someone use fasmmap?
Comment 121 Jacek Ablewicz 2016-03-07 15:21:20 UTC
(In reply to Jonathan Druart from comment #120)
> (In reply to Jonathan Druart from comment #119)
> > Actually it's certainly caused by the circular dependency.
> 
> Koha::Cache::_initialize_fastmmap could read the config file instead of
> calling C4::Context->config.

IMO delaying cache init / moving Koha::Cache->get_instance() call into preference() sub may be a better idea. Especially as in current master C4::Context::read_config_file() is apparently called 2+ times per script run allready:

   - 1st time when we do 'use Koha::Context;' anywhere
   - 2nd time when we intialize the database connection in Koha::Database via schema() / _new_schema() sub

:)

> But the question is: Does someone use fasmmap?

Probably not (but thats the only shared / permanent caching system available apart from memcache - which has its own quirks and constraints). I only encountered this problem because I was curious how would the 3 different caching systems compare performance-wise, since we are going to use them for something so much speed-sensitive (i.e preference fetching).
Comment 122 Jacek Ablewicz 2016-03-07 16:24:18 UTC Comment hidden (obsolete)
Comment 123 Jacek Ablewicz 2016-03-07 16:45:47 UTC
(In reply to Jacek Ablewicz from comment #122)
> Created attachment 48758 [details] [review] [review]
> Bug 11998: Delay cache intialisation
> 
> Should solve a problem with fastmmap cache system (see comment #114)

Maybe not such a good idea after all - to work correctly it would need additional numerous changes in other subroutines like clear_syspref_cache(), set_preference() etc.
Comment 124 Jacek Ablewicz 2016-03-08 12:43:47 UTC
Some (not very scientific) cache performace tests below. I measured C4::Context->preference() calls; 3 scenarios:

A) feching 1 small syspref (marcflavour) over and over - syspref cache (almost) unpopulated
B) feching 'marcflavour', cache fully populated (all 545 system preferencers in cache)
C) feching all system prefrences in alphabetical order, cache fully populated, average time spent in ->preference() call / per preference

Results (A / B / C):

- without patch (syspref cache = perl hashref): 1.1us / 1.1us / 0.9us
- Cache::Memory: 36us / 53us / 110us
- memcached: 41us / 41us / 42us
- fastmmap: 14us / 16us / 60us
- mysql query cache (256MB, DBI): ... / 130us / 135us
- mysql query cache (256MB, DBIx): ... / 1.20ms / 1.20ms
Comment 125 Jonathan Druart 2016-03-08 14:49:34 UTC
Hi Jacek,
Did you try to profile a "real life" situation?
I think it would make sense to use NYTProf on different pages, with bug 15973 and bug 15970 (if needed) applied.
Comment 126 Jacek Ablewicz 2016-03-09 10:09:25 UTC
(In reply to Jonathan Druart from comment #125)
> Hi Jacek,
> Did you try to profile a "real life" situation?

catalogue/search.pl (159 search results, w/ 50 results displayed per page)

It calls ->preference() 808 times, fetching 84 different preferences.

Total time spent in ->preference() calls (excluding 1st call which triggers DBIx class loading = 250-300ms on this test server)

1) without patch, current master: 98 ms
2) without patch, Bug 13967 reverted: 11 ms
3) with patch & default caching system (Cache::Memory): 98 ms + 70 ms spent in 
->get_from_cache()
4) with patch & memcached: 34 ms spent in ->get_from_cache()

I have no idea how "typical" is that particular example... I expect that a given script will use somewhere around 50-100 different system preferences, but how many times will ->preference() be called on average is pretty much impossible to estimate (200 ... 1000 ... 2000 ?)

Re/ caching system penalty in general - this patch will be probably not the biggest offender though; for example, Bug 11842 had bigger impact on catalogue/search.pl (150-350ms) - deserialising big, complex data structures in perl turns out to be quite expensive (especially if you do it 50 times per script run, like in this case).
Comment 127 Jacek Ablewicz 2016-03-09 10:18:31 UTC
(In reply to Jacek Ablewicz from comment #121)

> C4::Context::read_config_file() is apparently called 2+ times per script run
> allready:
> 
>    - 1st time when we do 'use Koha::Context;' anywhere
>    - 2nd time when we intialize the database connection in Koha::Database
> via schema() / _new_schema() sub

and 3rd time in C4/Auth_with_cas.pm (line 40)
Comment 128 Jacek Ablewicz 2016-03-09 12:43:36 UTC
(In reply to Jacek Ablewicz from comment #126)

> 3) with patch & default caching system (Cache::Memory): 98 ms + 70 ms spent
> in ->get_from_cache()

+ 10 ms spent in ->set_in_cache() - sorry, forgot to include that statement run time

Conclusion: Cache::Memory is not very efficient when used to store a lot of simple variables - probably because:

- it's pure perl (?)
- we currently serialize/deserialize all data types (including scalars) with freeze()/thaw() when using Cache::Memory

Memcache is better (up to 2x on average) for simple variables - it's XS, and it serializes only complex data structures, but there is (more or less constant) 40usec penalty involved due to network latency/turnaround.

Both chache systems are quite terrible for caching big, complex data structures (one MARC framework: 900 kilobytes; Cache::Memory is a bit faster then memcache in this case), mostly because they need to deserialize those structures each time when the data is fetched. We can try to use better serializer (Sereal etc.) - it may help speed up things for memcache (MARC framework storage size will go down to 300-100KB), but there is virtually nothing we can do to improve deserialization time. Instead, one can avoid doing the same expensive operation 50x times per script run, i.e. by implementing cache with 2+ levels (1st level being a simple hash or CHI::Driver::RawMemory for example, and 2nd level - memcache etc.)
Comment 129 Jacek Ablewicz 2016-03-09 19:48:13 UTC
(In reply to Jacek Ablewicz from comment #126)
> (In reply to Jonathan Druart from comment #125)
> > Hi Jacek,
> > Did you try to profile a "real life" situation?
> 
> catalogue/search.pl (159 search results, w/ 50 results displayed per page)
> 
> It calls ->preference() 808 times, fetching 84 different preferences.

If I enable search results XSLT processing, the same test causes 2706 preference() calls and 102 different preferences being fetched.. So, this patch set in its current form, in that particular scenario, will "cost" us at least an extra 200-250 msec for the default Cache::Memory caching system and ~100 msec for memcached.

BTW: those usec/msec values I posted in this bug report are for tests performed on (still, relativelly) fast server (i7-3770 + 32GB RAM).

BTW #2: Bug 15263 looks like a very good start for remeding excessive preference() calls involved in search results XSLT processing - but I bet that it is not the only scenario when a lot of excessive preference() calls happens in Koha.
Comment 130 Jonathan Druart 2016-03-10 10:11:05 UTC Comment hidden (obsolete)
Comment 131 Jonathan Druart 2016-03-10 10:11:14 UTC Comment hidden (obsolete)
Comment 132 Jonathan Druart 2016-03-10 10:11:20 UTC Comment hidden (obsolete)
Comment 133 Jonathan Druart 2016-03-10 10:11:26 UTC Comment hidden (obsolete)
Comment 134 Jonathan Druart 2016-03-10 10:11:32 UTC Comment hidden (obsolete)
Comment 135 Jonathan Druart 2016-03-10 10:11:37 UTC Comment hidden (obsolete)
Comment 136 Jonathan Druart 2016-03-10 10:11:43 UTC Comment hidden (obsolete)
Comment 137 Jonathan Druart 2016-03-10 10:11:49 UTC Comment hidden (obsolete)
Comment 138 Jonathan Druart 2016-03-10 10:11:56 UTC Comment hidden (obsolete)
Comment 139 Jonathan Druart 2016-03-10 10:14:16 UTC
(In reply to Jacek Ablewicz from comment #128)
> implementing cache with 2+ levels (1st level being a simple hash or
> CHI::Driver::RawMemory for example, and 2nd level - memcache etc.)

I really like this idea, I have implemented it in the last 2 patches.
I have also moved patches from bug 15970 and bug 15973 to this one (they should be pushed altogether).

How do you measure your results? Do you have a script to share?

Putting to 'in discussion'.
Comment 140 Jonathan Druart 2016-03-10 10:15:00 UTC
*** Bug 15970 has been marked as a duplicate of this bug. ***
Comment 141 Jonathan Druart 2016-03-10 10:15:12 UTC
*** Bug 15973 has been marked as a duplicate of this bug. ***
Comment 142 Jonathan Druart 2016-03-10 13:51:08 UTC Comment hidden (obsolete)
Comment 143 Tomás Cohen Arazi 2016-03-11 14:00:00 UTC Comment hidden (obsolete)
Comment 144 Tomás Cohen Arazi 2016-03-11 14:00:12 UTC Comment hidden (obsolete)
Comment 145 Tomás Cohen Arazi 2016-03-11 14:00:22 UTC Comment hidden (obsolete)
Comment 146 Tomás Cohen Arazi 2016-03-11 14:00:30 UTC Comment hidden (obsolete)
Comment 147 Tomás Cohen Arazi 2016-03-11 14:00:40 UTC Comment hidden (obsolete)
Comment 148 Tomás Cohen Arazi 2016-03-11 14:00:53 UTC Comment hidden (obsolete)
Comment 149 Tomás Cohen Arazi 2016-03-11 14:01:03 UTC Comment hidden (obsolete)
Comment 150 Tomás Cohen Arazi 2016-03-11 14:01:12 UTC Comment hidden (obsolete)
Comment 151 Tomás Cohen Arazi 2016-03-11 14:01:19 UTC Comment hidden (obsolete)
Comment 152 Jonathan Druart 2016-03-11 14:15:09 UTC
Jacek, I am interesting by your signoff if you think I am in the right direction with the L1 cache.
Comment 153 Jacek Ablewicz 2016-03-14 15:21:42 UTC
Created attachment 49100 [details] [review]
Bug 11998: Use Koha::Cache to cache sysprefs

At the moment, the sysprefs are only cache in the thread memory
executing the processus
When using Plack, that means we need to clear the syspref cache on each
page.
To avoid that, we can use Koha::Cache to cache the sysprefs correctly.

A big part of the authorship of this patch goes to Robin Sheat.

Test plan:
1/ Add/Update/Delete local use prefs
2/ Update pref values and confirm that the changes are correctly taken
into account

Signed-off-by: Chris <chrisc@catalyst.net.nz>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Tested with plack with syspref cache enabled, there is some time between setting the syspref and applying it, but it takes just one reload of page, it shouldn't be problem, should it?
Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>

Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Tested with CGI and CGI + memcache; some small issues still remain,
but it would be better to deal with them in separate bug reports
if necessary
Comment 154 Jacek Ablewicz 2016-03-14 15:21:52 UTC
Created attachment 49101 [details] [review]
Bug 11998: Use t::lib::Mocks::mock_preference in tests

Unless in t/db_dependent/Context.t where we want to test the cache
behaviors.

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 155 Jacek Ablewicz 2016-03-14 15:22:01 UTC
Created attachment 49102 [details] [review]
Bug 11998: Use C4::Context->preference in Koha/Schema/Result/Item.pm

Otherwise some tests won't pass

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 156 Jacek Ablewicz 2016-03-14 15:22:10 UTC
Created attachment 49103 [details] [review]
Bug 11998: Make HoldsQueue.t tests pass [TO REVERT]

This patch should not be the way to make these tests pass.
The prefs should not be updated using set_preference, it will modify the
values in the cache.

I have no idea how to make them pass using the correct way, please help
:)

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 157 Jacek Ablewicz 2016-03-14 15:22:18 UTC
Created attachment 49104 [details] [review]
Bug 11998: Do not clear syspref cache in psgi file

This in only in koha.psgi, it has been introduced by bug 13815 but
should not have been added by this patch.

Removing it should not introduce any changes.
Not that it won't impact debian packages.

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 158 Jacek Ablewicz 2016-03-14 15:22:27 UTC
Created attachment 49105 [details] [review]
Bug 11998: Revert "Bug 13805: (Plack) Disable syspref cache"

This reverts commit ca00f0ddaeb76106b4aa274fd1848844ae042f58.

Bug 13805 fixes an installer bug by disabling the syspref cache.
It was not a good idea, it introduced performance issues (see bug 13805
comment 14).

Test plan:
Test plan:
0/ Create a new database and fill the database entry in the koha conf
with its name
1/ Go on the mainpage, you should be redirected to the installer
2/ Try to log in
You should not get the login form again.

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Test plan not followed by me for this patch, due to lack of working
plack setup, but I don't expect it to cause any problems, and performace
gain for plack will be tremendous
Comment 159 Jacek Ablewicz 2016-03-14 15:22:35 UTC
Created attachment 49106 [details] [review]
Bug 11998: follow-up for the debian package psgi file

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 160 Jacek Ablewicz 2016-03-14 15:22:44 UTC
Created attachment 49107 [details] [review]
Bug 11998: Add a L1 cache for sysprefs

Accessing to the cache for each call to C4::Context->preference might
have an impact on performances.
To avoid that this patch introduces a L1 cache (simple hashref). It will
be populated by accessing the L2 cache (Koha::Cache).
If a pref is retrieved 10x, the first one will get the value from the L2
cache, then the L1 cache will be check.
To do so we will need to clear the L1 cache every time a page is loaded.

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 161 Jacek Ablewicz 2016-03-14 15:22:53 UTC
Created attachment 49108 [details] [review]
Bug 11998: Clear L1 cache from psgi files

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 162 Jonathan Druart 2016-03-14 15:28:14 UTC
(In reply to Jacek Ablewicz from comment #153)

Thanks a lot Jacek for your signoff!

> Tested with CGI and CGI + memcache; some small issues still remain,
> but it would be better to deal with them in separate bug reports
> if necessary

Could you open a bug report for each of them (and mark them dependent on this one)?
Comment 163 Jacek Ablewicz 2016-03-14 16:13:57 UTC
To summarize briefly an impact of this patch, regarding performance:

1) CGI + Cache::Memory: small loss (10-15 miliseconds on average ?), due to ~100 unnecessary get_from_cache()/set_in_cache() calls per script
2) CGI + memcache: significant speed gain (150-200 ms) + reduced mysql load
3) plack + memcache: huge gain (at least 200-500 ms on average, up to 10+ seconds in some circumstances) + vastly reduced mysql load

Issue 1) is easilly fixable, but it would be better (and a lot easier) to do it on the architectural level (i.e. inside Bug 16044, after it gathers momentum).
Comment 164 Jacek Ablewicz 2016-03-14 18:01:46 UTC
(In reply to Jonathan Druart from comment #162)

> > Tested with CGI and CGI + memcache; some small issues still remain,
> > but it would be better to deal with them in separate bug reports
> > if necessary
> 
> Could you open a bug report for each of them (and mark them dependent on
> this one)?

Some previously mentioned issues are filled under Bug 16067 and Bug 16070, and one additional, not previously mentioned, in Bug 16068. To be quite frank - I have not retested them with the current/final patch set, just eyeballed the code and maked an educated guess that they will still be valid ;)
Comment 165 Brendan Gallagher 2016-03-15 07:09:47 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!
Comment 166 Jacek Ablewicz 2016-03-15 09:20:13 UTC
(In reply to Jonathan Druart from comment #162)

> > Tested with CGI and CGI + memcache; some small issues still remain,
> > but it would be better to deal with them in separate bug reports
> > if necessary
> 
> Could you open a bug report for each of them (and mark them dependent on
> this one)?

Regarding possible circular dependencies issues mentioned in the whole lot of the previous comments: there is 1+ circular dependency somewhere in current master (perl -wc C4/Context.pm test does not pass), but it's not caused by the final patch set from this report. Can't exclude the posibility that this patch set may be adding another one (it certainly has some potential), but it's hard to judge at the moment - let's deal with it later if necessary.
Comment 167 Jonathan Druart 2016-03-18 13:30:12 UTC
(In reply to Jonathan Druart from comment #98)
> Patches squashed, rebased and adapted to current master.
> I have rewritten and it seems to work as expected.
> See also bug 15970, bug 15341 and bug 15873, to know why we need this patch.
> 
> Note that we still have 
> Name "Cache::RemovalStrategy::FIELDS" used only once: possible typo at
> /usr/share/perl/5.20/fields.pm line 135.
> Name "Cache::RemovalStrategy::LRU::FIELDS" used only once: possible typo at
> /usr/share/perl/5.20/fields.pm line 135.
> Name "Tie::Hash::FIELDS" used only once: possible typo at
> /usr/share/perl/5.20/fields.pm line 135.
> 
> Forget them, unless you know how to fix them.

See bug 16104 for an attempt to remove them
Comment 168 Katrin Fischer 2016-03-22 06:16:45 UTC
This morning my OPAC is broken and git bisect leads to this bug. I am using a local git installation using localhost for the OPAC. Staff on localhost:8080 seems ok. The error shown is:

Software error:

DBIx::Class::Storage::DBI::catch {...} (): DBI Connection failed: Access denied for user 'www-data'@'localhost' (using password: NO) at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1398. at /home/katrin/kohaclone/Koha/Database.pm line 90
Comment 169 Jacek Ablewicz 2016-03-22 07:47:04 UTC
(In reply to Katrin Fischer from comment #168)
> This morning my OPAC is broken and git bisect leads to this bug. I am using
> a local git installation using localhost for the OPAC. Staff on
> localhost:8080 seems ok. The error shown is:
> 
> Software error:
> 
> DBIx::Class::Storage::DBI::catch {...} (): DBI Connection failed: Access
> denied for user 'www-data'@'localhost' (using password: NO) at
> /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1398. at
> /home/katrin/kohaclone/Koha/Database.pm line 90

Bug 16067, perhaps ?
Comment 170 Katrin Fischer 2016-03-22 08:31:04 UTC
If that's the problem - what would be the fix?
Comment 171 Jonathan Druart 2016-03-22 08:57:53 UTC
(In reply to Katrin Fischer from comment #170)
> If that's the problem - what would be the fix?

What's the value of CACHING_SYSTEM?
If it's 'fastmmap', set it to 'memcached'
Comment 172 Katrin Fischer 2016-03-22 22:33:27 UTC
Changed as suggested in koha-httpd.conf and restarted Apache - not the OPAC is back :)
Comment 173 Julian Maurice 2016-03-23 07:01:26 UTC
Patches pushed to 3.22.x, will be in 3.22.5
Comment 174 Julian Maurice 2016-03-23 13:13:44 UTC
(In reply to Jonathan Druart from comment #112)
> Created attachment 48727 [details] [review] [review]
> Bug 11998: Make HoldsQueue.t tests pass [TO REVERT]
> 
> This patch should not be the way to make these tests pass.
> The prefs should not be updated using set_preference, it will modify the
> values in the cache.
> 
> I have no idea how to make them pass using the correct way, please help
> :)

Maybe it's because the mocked preference sub is case sensitive (not verified)
A patch fixes that on bug 16134
Comment 175 Jonathan Druart 2016-03-23 15:33:50 UTC
(In reply to Julian Maurice from comment #174)
> (In reply to Jonathan Druart from comment #112)
> > Created attachment 48727 [details] [review] [review] [review]
> > Bug 11998: Make HoldsQueue.t tests pass [TO REVERT]
> > 
> > This patch should not be the way to make these tests pass.
> > The prefs should not be updated using set_preference, it will modify the
> > values in the cache.
> > 
> > I have no idea how to make them pass using the correct way, please help
> > :)
> 
> Maybe it's because the mocked preference sub is case sensitive (not verified)
> A patch fixes that on bug 16134

No, I have just checked and reverted the commit makes the tests failing on top of bug 16134.
Comment 176 Srdjan Jankovic 2016-03-30 00:59:31 UTC
Jonathan, I'm having problems with t/db_dependent/Context.t test 35 (line 97) where you reverted logic. Can you please double check it for me, cause it looks to me that the previous version was ok.
Much obliged.
Comment 177 Jonathan Druart 2016-03-30 06:26:49 UTC
(In reply to Srdjan Jankovic from comment #176)
> Jonathan, I'm having problems with t/db_dependent/Context.t test 35 (line
> 97) where you reverted logic. Can you please double check it for me, cause
> it looks to me that the previous version was ok.
> Much obliged.

Are you talking about
 99 C4::Context->enable_syspref_cache();
100 is(C4::Context->preference("SillyPreference"), 'thing3', "Retrieved syspref (value='thing3') successfully from cache");
101 isnt( $trace_read, q{}, 'The pref should be retrieved from the database if the cache has been enabled');
102 $trace_read = q{};
?

C4::Context->enable_syspref_cache will clear the syspref cache, so we will need to access the DB to retrieve the value of the pref.
Comment 178 Srdjan Jankovic 2016-03-31 00:56:36 UTC
Thanks Jonathan, a rebase issue, an extra C4::Context->preference
("SillyPreference") call got in between somehow.
Cheers, Srdjan
Comment 179 Jacek Ablewicz 2016-04-25 09:46:20 UTC
(In reply to Jacek Ablewicz from comment #166)

> Regarding possible circular dependencies issues mentioned in the whole lot
> of the previous comments: there is 1+ circular dependency somewhere in
> current master (perl -wc C4/Context.pm test does not pass), but it's not
> caused by the final patch set from this report. Can't exclude the posibility
> that this patch set may be adding another one (it certainly has some
> potential), but it's hard to judge at the moment - let's deal with it later
> if necessary.

Looks like there are 2+ circular dependencies in Context.pm currently (unrelated to this bug raport)

1) C4::Context -> Koha::Config::SysPref[s] -> Koha::Database -> C4::Context
2) C4::Context -> Koha::Config::SysPref[s] -> C4::Log -> C4::Context