Bug 10611

Summary: C4::Context->dbh checks if the DB is still running
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Paul Poulain <paul.poulain>
Severity: major    
Priority: P1 - high CC: bgkriegel, fridolin.somers, gmcharlt, katrin.fischer, kyle, paul.poulain, robin
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 11051    
Attachments: Bug 10611: Use mysql_auto_reconnect instead of ping
Bug 10611: Use mysql_auto_reconnect instead of ping
[SIGNED-OFF] Bug 10611: Use mysql_auto_reconnect instead of ping
Bug 10611: Followup: remove useless calls to dbh->disconnect
Bug 10611: add a "new" parameter to C4::Context->dbh
Bug 10611: Followup: remove useless calls to dbh->disconnect
[SIGNED-OFF] Bug 10611: Use mysql_auto_reconnect instead of ping
[SIGNED-OFF] Bug 10611: add a "new" parameter to C4::Context->dbh
[SIGNED-OFF] Bug 10611: Followup: remove useless calls to dbh->disconnect
[PASSED QA]Bug 10611: Use mysql_auto_reconnect instead of ping

Description Jonathan Druart 2013-07-18 13:54:04 UTC
Given that mysql is the only DBMS available for Koha, it seems that it is not necessary to do a ping before returning the handler.
Comment 1 Jonathan Druart 2013-07-18 13:57:03 UTC Comment hidden (obsolete)
Comment 2 Galen Charlton 2013-07-22 22:55:31 UTC
This patch breaks test cases that mock C4::Context->_new_dbh():

t/db_dependent/Circulation_issuingrules.t .. 
1..7
ok 1 - use C4::Circulation;
not ok 2 - first matches

#   Failed test 'first matches'
#   at t/db_dependent/Circulation_issuingrules.t line 55.
#     Structures begin differing at:
#          $got->{issuelength} = '21'
#     $expected->{issuelength} = '10'
ok 3 - none matches
ok 4 - date expiry
ok 5 - date expiry
not ok 6 - date expiry ( 9 + 10 )

#   Failed test 'date expiry ( 9 + 10 )'
#   at t/db_dependent/Circulation_issuingrules.t line 119.
#          got: '2013-03-02T23:59:00'
#     expected: '2013-02-19T23:59:00'
not ok 7 - date expiry ( 9 + 5 )

#   Failed test 'date expiry ( 9 + 5 )'
#   at t/db_dependent/Circulation_issuingrules.t line 123.
#          got: '2013-03-02T23:59:00'
#     expected: '2013-02-14T23:59:00'
# Looks like you failed 3 tests of 7.
Dubious, test returned 3 (wstat 768, 0x300)
Failed 3/7 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation_issuingrules.t (Wstat: 768 Tests: 7 Failed: 3)
  Failed tests:  2, 6-7
  Non-zero exit status: 3
Files=1, Tests=7,  0 wallclock secs ( 0.03 usr  0.01 sys +  0.78 cusr  0.08 csys =  0.90 CPU)
Result: FAIL
Comment 3 Jonathan Druart 2013-08-12 13:20:24 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2013-08-12 13:21:08 UTC
(In reply to Galen Charlton from comment #2)
> This patch breaks test cases that mock C4::Context->_new_dbh()

Good catch, the db_driver variable was not defined!
Comment 5 Jonathan Druart 2013-08-29 14:49:38 UTC
Benchmarks with misc/load_testing/benchmark_staff.pl

== With a remote DB ==

= Without the patch =
Benchmarking with 20 occurences of each operation and 30 concurrent sessions 
Step 1: staff client main page          11512ms 1.737 pages/sec
Step 2: catalog detail page             15142ms 1.32 biblios/sec
Step 3: catalogue search                26549ms 0.753 biblios/sec
Step 4: patron detail page              12248ms 1.632 borrowers/sec
Step 5: patron search page              26901ms 1.486 borrowers/sec
Step 6a circulation (checkouts)         8697ms  2.299 checkouts/sec
Step 6b circulation (checkins)          8779ms  2.278 checkins/sec

= With the patch =
Benchmarking with 20 occurences of each operation and 30 concurrent sessions 
Step 1: staff client main page          11404ms 1.753 pages/sec
Step 2: catalog detail page             13286ms 1.505 biblios/sec
Step 3: catalogue search                19946ms 1.002 biblios/sec
Step 4: patron detail page              10962ms 1.824 borrowers/sec
Step 5: patron search page              24669ms 1.621 borrowers/sec
Step 6a circulation (checkouts)         8856ms  2.258 checkouts/sec
Step 6b circulation (checkins)          8623ms  2.319 checkins/sec
Comment 6 Paul Poulain 2013-09-10 13:53:55 UTC
I consider that this patch is not an ENH, but a bugfix needed for libraries that want to put their mySQL server outside from their web one.

So i've upgraded the severity & priority
Comment 7 Bernardo Gonzalez Kriegel 2013-09-20 05:29:06 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2013-09-27 22:50:40 UTC
I am not entirely sure how to test this, but I have started with running the automated and db_dependent tests and it looks to me like the change breaks t/db_dependent/Context.t.

On master:
[~/kohaclone (master)]> prove t/db_dependent/Context.t 
t/db_dependent/Context.t .. # Note: The overall number of tests may vary by configuration.
# First we need to check your environmental variables
t/db_dependent/Context.t .. 1/? # Number of keys in %$koha: 13
# Examining defined key values.
# Testing syspref caching.
t/db_dependent/Context.t .. ok    
All tests successful.
Files=1, Tests=38,  0 wallclock secs ( 0.03 usr  0.01 sys +  0.16 cusr  0.01 csys =  0.21 CPU)
Result: PASS

With patch applied:
[~/kohaclone (71-10611-contextdb)]> prove t/db_dependent/Context.t 
t/db_dependent/Context.t .. # Note: The overall number of tests may vary by configuration.
# First we need to check your environmental variables
t/db_dependent/Context.t .. 1/? # Number of keys in %$koha: 13
# Examining defined key values.
# Testing syspref caching.

#   Failed test 'Retrieved syspref (value='thing1') successfully with default behavior'
#   at t/db_dependent/Context.t line 74.
#          got: undef
#     expected: 'thing1'
Can't use an undefined value as an ARRAY reference at t/db_dependent/Context.t line 76.
# Tests were run but no plan was declared and done_testing() was not seen.
t/db_dependent/Context.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 1/25 subtests 

Test Summary Report
-------------------
t/db_dependent/Context.t (Wstat: 65280 Tests: 25 Failed: 1)
  Failed test:  25
  Non-zero exit status: 255
  Parse errors: No plan found in TAP output
Files=1, Tests=25,  0 wallclock secs ( 0.04 usr  0.01 sys +  0.17 cusr  0.00 csys =  0.22 CPU)
Result: FAIL

Jonathan, can you please take a look?
Comment 9 Jonathan Druart 2013-09-30 11:48:11 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2013-09-30 11:48:32 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2013-09-30 11:48:36 UTC Comment hidden (obsolete)
Comment 12 Bernardo Gonzalez Kriegel 2013-09-30 17:28:36 UTC Comment hidden (obsolete)
Comment 13 Bernardo Gonzalez Kriegel 2013-09-30 17:28:47 UTC Comment hidden (obsolete)
Comment 14 Bernardo Gonzalez Kriegel 2013-09-30 17:28:56 UTC Comment hidden (obsolete)
Comment 15 Bernardo Gonzalez Kriegel 2013-09-30 17:45:24 UTC
sorry, missed one patch. re-uploading
Comment 16 Bernardo Gonzalez Kriegel 2013-09-30 17:46:20 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #15)
> sorry, missed one patch. re-uploading
oops, wrong Bug. No problem here
Comment 17 Paul Poulain 2013-10-20 19:39:40 UTC
I made some tests with NYTProf, on opac-main.pl

* without the patch
 if (defined($context->{"dbh"}) && $context->{"dbh"}->ping()) {
 # spent 13.5ms making 120 calls to DBI::db::ping, avg 112µs/call

=> 120 pings are sent to mysql

* with the patch
121	1.10ms	if ( defined $db_driver && $db_driver eq 'mysql' && $context->{"dbh"} ) {
			return $context->{"dbh"};
		} elsif ( defined $db_driver && defined($context->{"dbh"}) && $context->{"dbh"}->ping()) {
			return $context->{"dbh"};
		}

=> no pings sent, even if the 120 calls are still here.

We spent on ly 1.10ms instead of 13.5 in those checks. And the lag will be much larger in case of a "slow" network mySQL
Comment 18 Paul Poulain 2013-10-20 19:42:24 UTC
Created attachment 22103 [details] [review]
[PASSED QA]Bug 10611: Use mysql_auto_reconnect instead of ping

DBD::Mysql provides a mysql_auto_reconnect flag. Using it avoid to do a
ping.

Benchmarks:

use Modern::Perl;
use C4::Context;
for ( 1 .. 1000 ) {
    $dbh = C4::Context->dbh;
}

* without this patch on a local DB:
perl t.pl  0,49s user 0,02s system 98% cpu 0,525 total
* without this patch on a remote DB:
perl t.pl  0,52s user 0,05s system 1% cpu 37,358 total
* with this patch on a local DB:
perl t.pl  0,46s user 0,04s system 99% cpu 0,509 total
* with this patch on a remote DB:
perl t.pl  0,49s user 0,02s system 56% cpu 0,892 total

Testing the auto reconnect:
use Modern::Perl;
use C4::Context;
my $ping = $dbh->ping;
say $ping;
$dbh->disconnect;
$ping = $dbh->ping;
say $ping;

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment: Real improvement. No koha-qa errors

prove t/db_dependent/Circulation_issuingrules.t produces no error
prove t/db_dependent/Context.t produces no error

Test
1) dumped Koha DB, load it on a non-local server
2) run sample script whit and without patch, local and remote

use Modern::Perl;
use C4::Context;
for ( 1 .. 100000 ) {
    my $dbh = C4::Context->dbh;
}

Main difference I note is with remote server
a) without patch
real    0m16.357s
user    0m2.592s
sys     0m2.132s

b) with patch
real    0m0.259s
user    0m0.240s
sys     0m0.012s

I think this could be good for DBs placed on
remote servers

Bug 10611: add a "new" parameter to C4::Context->dbh

When dbh->disconnect is called and the mysql_auto_reconnect flag is set,
the dbh is not recreated: the old one is used.

Adding a new flag, we can now force the C4::Context->dbh method to
return a new dbh.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Bug 10611: Followup: remove useless calls to dbh->disconnect

These 3 calls to disconnect are done at the end of the script, they are
useless.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Comment 19 Paul Poulain 2013-10-20 19:43:55 UTC
QA comments : passes koha-qa.pl, no visible changes in the interface, improve performances.

Linked to mysql, but this will be outdated when DBIx::Class will replace current database management.

passed QA
Comment 20 Galen Charlton 2014-03-10 23:19:29 UTC
Pushed to master, along with a follow-up that keeps the caching of the DB driver code within the object, not the package.
Comment 21 Galen Charlton 2014-03-10 23:19:43 UTC
Thanks Jonathan!
Comment 22 Fridolin Somers 2014-05-16 10:18:01 UTC
Pushed to 3.14.x, will be in 3.14.07.

I had some minor conflicts, but since it is the most important module it would be great if someone tests it before the release.
Comment 23 Kyle M Hall 2014-06-06 13:49:44 UTC
Pushed to 3.12.x will be in 3.12.13