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.

These patches will need to be treated as a series, they depend on the previous ones.

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 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 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. :)

(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.

Can someone please sign this off, ... (I can't as its from catalyst)

I don't use Plack but here's a *bump* anyway...

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?

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.

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.

QA Comment: Should these db action in Context.pm use the dbic variation now days?

Other than that, the code looks good to me.

QA Comment: I also get qa script failure.

 FAIL   C4/Context.pm
   FAIL   critic
                # Variables::ProhibitConditionalDeclarations: Got 1 violation(s).

I also get all Bernardo's failures too..

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.

(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.

I bet it's a circular dependency.

This might be problematic.

(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

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).

(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.

(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.

OK, implemented in a good-enough-for-now sorta way that improves things overall.

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.

BTW, please take a look at t/db_dependent/Context.t failing with this patches.

(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.

(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?

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 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.

(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.

(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.

After all these patches applied, there is an unused variable $logtype in C4::Context::set_preference.

(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.

(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.

(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.

(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.

(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.

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 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.

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

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.

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.

So... I'm a tad confused, are we back at a point where I can test these patches for signoff..?

(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.

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?

Could be "solved" by bug 13805.

We need to get moving on this, but there are conflicts in Context.pm.
Please rebase!

I am not sure about the current state of this - are the current patches still valid?

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.

Please note I have tested this under CGI, caching works fine, but it still needs testing under plack.

(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 :)

It looks like it is just 1 or 2 seconds max, so it is absolutely OK ;)

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 ;)

(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.

(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!

(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.

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

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.

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).

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();

(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'

Sorry, forget this last comment (I was using master... :D)

(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!

And also this one!!

Actually it's certainly caused by the circular dependency.

(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?

(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).

(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.

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

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.

(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).

(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)

(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.)

(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.

(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'.

*** Bug 15970 has been marked as a duplicate of this bug. ***

*** Bug 15973 has been marked as a duplicate of this bug. ***

Jacek, I am interesting by your signoff if you think I am in the right direction with the L1 cache.

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

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>

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>

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>

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>

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

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>

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>

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>

(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)?

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).

(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 ;)

Pushed to Master - Should be in the May 2016 release.  Thanks!

(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.

(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

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

(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 ?

If that's the problem - what would be the fix?

(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'

Changed as suggested in koha-httpd.conf and restarted Apache - not the OPAC is back :)

Patches pushed to 3.22.x, will be in 3.22.5

(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

(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.

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.

(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.

Thanks Jonathan, a rebase issue, an extra C4::Context->preference
("SillyPreference") call got in between somehow.
Cheers, Srdjan

(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