bigpresh / Dancer-Plugin-Database

Dancer::Plugin::Database - easy database support for Dancer applications
http://search.cpan.org/dist/Dancer-Plugin-Database
37 stars 36 forks source link

Support of mysql_auto_reconnect #22

Closed knutov closed 11 years ago

knutov commented 12 years ago

Hello again,

is it possible to add mysql_auto_reconnect support to this module?

As for now, I found it is stable and works perfect. It allows to make a connection at the prefork/init state and saving time on making connection to the MySQL on each request. This leads to big speed up if the site. This is noticeable in both cases when the mysql server is near the client and it's very noticeable when mysql server is in different datacenter.

bigpresh commented 12 years ago

As I understand it, mysql_auto_reconnect is just an attribute supplied to DBI upon connection; you should be able to use this module's dbi_params config setting to pass it on, e.g.:

plugins:
    Database:
        # normal driver/host/etc config
        dbi_params:
            mysql_auto_reconnect: 1

See also https://metacpan.org/module/Dancer::Plugin::Database#CONFIGURATION - I think it'd be used in exactly the same way as the RaiseError / AutoCommit attributes illustrated in the example there.

knutov commented 12 years ago

Yes, but..

1) As I understand the code - connection to the DB will be made at the first request, not at the prefork time. What to do with this? 'select 1' at the beginning of the app.pm?

2) I think connection_check_threshold should be set to unlimited in this case, how to do it/disable this feature?

3) I cannot find (in code and documentation) - when $dbh->disconnect happens? Does it happens at all or connection will be alive (when preforking with starman) until [mysql]server idle connection time ends?

bigpresh commented 12 years ago

1) Yep, the connection is established the first time that the database() keyword is called. (This is by design, as in simpler apps, there may well be plenty of pages which have no need for a database connection at all.

If I can think of a clean way to hook in to an app starting up before any requests are handled and make it optional to get a connection there, that might be something I could add (although if the process then forks, that handle would be invalidated anyway for safety - sharing database connections between processes/threads is dodgy).

2) I think just setting connection_check_threshold to zero should disable that.

3) It doesn't happen explictly; the connection should remain open until the database handle goes out of scope when the app closes down; the handle is held onto between requests.

I guess I could add a "disconnect after request" option, for situations where the user doesn't want database handles being kept open between requests but still wants the convenience of the module, but I can't imagine it would be massively useful. If it would be helpful to you, though, I can add it.

knutov commented 12 years ago

1) As for now, as I understand, there is no practical reasons to use Coro, EV and any other non-prefoking servers with dancer, because anything inside dancer app is blocking now. But I agree, it's ok to spend more time on the very first query, if it stay connected later. Anyway, 'select 1' at the startup time is always possible, if it will be hardly needed.

2) hmm. But in this case

$conn_details->{connection_check_threshold} &&
            time - $handle->{last_connection_check}
            < $conn_details->{connection_check_threshold}

will be always false (example: 0 && 2<0?'true':'false') and _check_connection with $dbh->ping or 'select 1' will be done on every query.

3) ok, I think it's better to add description of default behavior to documentation. "disconnect after request" is always possible to do via hook 'after' so I don't think it's important to add this in config/code.

jwieland commented 11 years ago

my 2 cents.. Enabling mysql_auto_reconnect is such a common switch one almost thinks it would happen by default. Not sure why you would not want it, however to help on the commons I would add it into the documentation and as recommend setting.

bigpresh commented 11 years ago

Good point regarding the inability to disable connection checking. I've just committed a fix, so that if you supply e.g. connection_check_threshold: 0, no connectivity checking will be performed.

Personally, I'm not in favour of automatically enabling mysql_auto_reconnect, as it's a surprising change to defaults which could trip users up; it's easy enough to enable if desired. I also seem to recall some vague problems with it not always working correctly, and it still letting a query fail (but meaning that the connection will be re-established if you try the query again), but I could well be imagining that, or it could just be very old DBD::mysql versions. Either way, the current behaviour works well for a lot of people I think,and I don't want to make surprising changes to defaults.