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.
Created attachment 19765 [details] [review] 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;
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
Created attachment 20273 [details] [review] 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;
(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!
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
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
Created attachment 21264 [details] [review] [SIGNED-OFF] 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 t/db_dependent/Circulation_issuingrules.t .. ok All tests successful. Test: run twice sample script with more iterations first local, then remote 1) without patch a) local real 0m2.461s user 0m0.848s sys 0m1.052s b) local real 0m2.427s user 0m1.072s sys 0m0.852s 2) with patch a) local real 0m0.213s user 0m0.196s sys 0m0.012s b) local real 0m0.208s user 0m0.200s sys 0m0.008s 3) without patch a) remote real 0m14.957s user 0m2.048s sys 0m2.012s b) remote real 0m15.472s user 0m1.816s sys 0m2.728s 4) with patch a) remote real 0m0.221s user 0m0.208s sys 0m0.012s b) remote real 0m0.216s user 0m0.192s sys 0m0.020s
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?
Created attachment 21655 [details] [review] 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.
Created attachment 21656 [details] [review] 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.
Created attachment 21657 [details] [review] 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.
Created attachment 21667 [details] [review] [SIGNED-OFF] 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
Created attachment 21668 [details] [review] [SIGNED-OFF] 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>
Created attachment 21669 [details] [review] [SIGNED-OFF] 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>
sorry, missed one patch. re-uploading
(In reply to Bernardo Gonzalez Kriegel from comment #15) > sorry, missed one patch. re-uploading oops, wrong Bug. No problem here
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
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>
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
Pushed to master, along with a follow-up that keeps the caching of the DB driver code within the object, not the package.
Thanks Jonathan!
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.
Pushed to 3.12.x will be in 3.12.13