DBIx::Connector doc about $conn->dbh() indicates he will manage database handler cache and reconnection : http://search.cpan.org/perldoc?DBIx%3A%3AConnector#dbh So C4::Context should not store dbh object but DBIx::Connector object and get handler from it. We experimented issues with SIP server, since its database connections will run all day long. Looks like DBIx::Connector manages very well the reconnection.
Created attachment 40063 [details] [review] Bug 14375 - DBIx::Connector should be stored in C4::Context instead of dbh DBIx::Connector doc about $conn->dbh() indicates he will manage database handler cache and reconnection : http://search.cpan.org/perldoc?DBIx%3A%3AConnector#dbh So C4::Context should not store dbh object but DBIx::Connector object and get handler from it. We experimented issues with SIP server, since its database connections will run all day long. Looks like DBIx::Connector manages very well the reconnection. Test plan : - Check database connection does not fail for pages - Check database connection does not fail for SIP Server - One may check its ok with Plack
Created attachment 40065 [details] [review] Bug 14375 - DBIx::Connector should be stored in C4::Context instead of dbh DBIx::Connector doc about $conn->dbh() indicates he will manage database handler cache and reconnection : http://search.cpan.org/perldoc?DBIx%3A%3AConnector#dbh So C4::Context should not store dbh object but DBIx::Connector object and get handler from it. We experimented issues with SIP server, since its database connections will run all day long. Looks like DBIx::Connector manages very well the reconnection. This patch has to drop methods set_dbh() and restore_dbh() because they seem to be impossible to implement with this changes. Test plan : - Check database connection does not fail for pages - Check database connection does not fail for SIP Server - One may check its ok with Plack
This patch has to drop methods set_dbh() and restore_dbh() because they seem to be impossible to implement with this changes. Are they even used ? I found nothing in sources.
Created attachment 40244 [details] [review] Bug 14375 - DBIx::Connector should be stored in C4::Context instead of dbh DBIx::Connector doc about $conn->dbh() indicates he will manage database handler cache and reconnection : http://search.cpan.org/perldoc?DBIx%3A%3AConnector#dbh So C4::Context should not store dbh object but DBIx::Connector object and get handler from it. We experimented issues with SIP server, since its database connections will run all day long. Looks like DBIx::Connector manages very well the reconnection. Note that mysql_enable_utf8 option is passed in DBIx::Connector creation so it can be set again when the connector recreates a dbh. This patch has to drop methods set_dbh() and restore_dbh() because they seem to be impossible to implement with this changes. Test plan : - Check database connection does not fail for pages - Check database connection does not fail for SIP Server - One may check its ok with Plack
I've found a mandatory change : mysql_enable_utf8 option must be passed in DBIx::Connector creation so it can be set again when the connector recreates a dbh.
(In reply to Fridolin SOMERS from comment #0) > We experimented issues with SIP server, since its database connections will > run all day long. > Looks like DBIx::Connector manages very well the reconnection. Does it mean you are running this patch in production yet?
There is a lot of occurrences of _new_dbh in tests: git grep _new_dbh | wc -l 17 I'd say keep the original name "_new_dbh", why do you want to change it?
(In reply to Jonathan Druart from comment #6) > (In reply to Fridolin SOMERS from comment #0) > > We experimented issues with SIP server, since its database connections will > > run all day long. > > Looks like DBIx::Connector manages very well the reconnection. > > Does it mean you are running this patch in production yet? Not in production but in test for it
Created attachment 41267 [details] [review] Bug 14375 - DBIx::Connector should be stored in C4::Context instead of dbh DBIx::Connector doc about $conn->dbh() indicates he will manage database handler cache and reconnection : http://search.cpan.org/perldoc?DBIx%3A%3AConnector#dbh So C4::Context should not store dbh object but DBIx::Connector object and get handler from it. We experimented issues with SIP server, since its database connections will run all day long. Looks like DBIx::Connector manages very well the reconnection. Note that mysql_enable_utf8 option is passed in DBIx::Connector creation so it can be set again when the connector recreates a dbh. This patch has to drop methods set_dbh() and restore_dbh() because they seem to be impossible to implement with this changes. Test plan : - Check database connection does not fail for pages - Check database connection does not fail for SIP Server - One may check its ok with Plack
My patch suppressed a few obsolete lines in C4:Context::preference. Those line have nothing to do directly with the problem of this bug so I restore them
Hi there! I like your approach, you are doing the right thing. DBIx::Connector is said to be more robust and I am happy to hear it has solved your issues. However there is an issue with this patch that needs addressing if you want a sign off from me. First of all the naming convention is changed which can cause a lot of issues for everybody. Some tests, custom modifications etc. can depend on $context->{dbh}. The new name doesn't help increase clarity IMHO and we should stick to the old "dbh". - return $context->{"dbh"}; + return $context->{'DBconn'}->dbh; Same issue here -sub _new_dbh +sub _new_DBconn _new_dbh is addressed directly from many places (atleast I do) regardless of it being a private method. This is to circumvent bugs/limitations of the C4::Context->dbh()-system. No reason to change it. Because of these things a small and beautiful, yet invaluably important fix is drowned into a large commit changing too many things for reasons I do not agree of.
Created attachment 41437 [details] [review] Bug 14375 - Use mock on dbh instead of _new_dbh This patch changes in test modules the mock on database connexion. Instead of mocking method _new_dbh (removed by main patch), mock directly method dbh. Always use t::lib::Mocks::mock_dbh() for that. To test, just run prove on modified test modules.
I've corrected test modules that mock database connexion.
Created attachment 41439 [details] test module Test module. It saves a record and tries to get it each 15 minutes 4 times. Without patch, it fails with : database has run away.
(In reply to Fridolin SOMERS from comment #14) > Created attachment 41439 [details] > test module > > Test module. > > It saves a record and tries to get it each 15 minutes 4 times. > Without patch, it fails with : database has run away. Why 900sec?
(In reply to Jonathan Druart from comment #15) > (In reply to Fridolin SOMERS from comment #14) > > Created attachment 41439 [details] > > test module > > > > Test module. > > > > It saves a record and tries to get it each 15 minutes 4 times. > > Without patch, it fails with : database has run away. > > Why 900sec? It is to loose the DB connexion. Maybe a few minutes is enought.
Fridolin, Could you have a look at the patch I have just submitted on bug 14778?
Please see http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14778 The Author of DBIx::Class recommends we shouldn't use DBIx::Connector and I tend to agree
(In reply to Chris Cormack from comment #18) > Please see > > http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14778 > > The Author of DBIx::Class recommends we shouldn't use DBIx::Connector and I > tend to agree Oki, this is even better than this patch, great.
*** This bug has been marked as a duplicate of bug 14778 ***