Re: Ima::DBI patch for mod_perl compatibility

[prev] [thread] [next] [Date index for 2005/06/16]

On Fri, 2005-06-10 at 21:00 +0100, Tony Bowden wrote: 
> Class::DBI and Ima::DBI shouldn't need to know about their
> environment.

I agree with the sentiment, but the problem is that Ima::DBI is a
database connection manager, and thus has to either know enough about
the environment to avoid breaking things, or provide hooks for people to
use to adapt it to their environment.  DBI itself has done this, in the
form of hooks for Apache::DBI and special-purpose calls to use in
applications that fork.  Ima::DBI breaks some of this by keeping its own
connection cache in closures.  If the storage of db handles in closures
was taken out, and Ima::DBI stopped acting as a connection cache, none
of this would be necessary.

> The 'storm of protests' doesn't carry very much weight with me. Mostly
> they seem to be reacting more to a perceived problem that's much more
> philosophical, than to the technical problem, which, AIUI is quite a
> technical one that doesn't only shows up in circumstances that most
> users don't every encounter.
> 
> As I've said, however, this isn't a problem that's bitten me, and I
> haven't really understand it all, so I may well have picked things up 
> incorrectly.

Let me try to explain it clearly, so that we all know what we're
discussing here.

Apache::DBI does a couple of important things, besides maintaining
persistent connections (which DBI->connect_cached can do perfectly well
on its own).  One of them is that whenever you connect to a database
with AutoCommit off, i.e. using transactions, it sets a cleanup handler
to run at the end of the request.  This cleanup handler issues a
rollback on those database handles.  The reason this is important is
that if you use Postgres or MySQL InnoDB or something else with
transactions, and your code dies unexpectedly in the middle somewhere,
you can be left with a bunch of row locks being held open, or with half-
finished data that will get committed the next time some other request
uses this connection.

Using the do_transaction() code from the man page will also prevent this
problem, if you use it religiously in every place where you turn off
AutoCommit.  I like the extra insurance of the cleanup handler, and
personally I don't use the do_transaction() approach, having had too
many problems with the sub ref stuff in Error.pm in the past.  It's also
worth noting that people who connect with AutoCommit on will never get
Apache::DBI's cleanup handler anyway, and have to set it themselves.
This patch won't fix that.

The relevance to Ima::DBI here is that Ima::DBI only calls
connect_cached (which calls Apache::DBI when it's loaded), when it first
connects to the database, and then keeps the db handle in a private
cache inside a closure.  You need to call Apache::DBI the first time you
use the handle on each request in order for the cleanup handler to get
set each time.  That's what my patch does.  Storing the handle in
pnotes, which gets cleared at the end of every request, helps make up
for any performance loss from not keeping the handle in the closure.

The second important thing that Apache::DBI does is prevent you from
opening connections before the server forks and then trying to reuse
them after forking (in a different process).  This can cause segfaults
and scary-looking network errors from DBI.  Because Ima::DBI doesn't
check for this and just caches the handle itself in a closure, it will
result in handles opened before forking being reused.  The main cause of
handles being opened before forking is the set_up_table() call which
needs to talk to the database to determine the table structure.  My
patch makes Ima::DBI avoid caching the db handle in a closure (when
under mod_perl), so it lets Apache::DBI do the right thing.

An annoying side effect of this is that you get a bunch of warnings from
DBI about the handle being DESTROYed without a disconnect.  Without the
caching of the handle in the closure, it gets DESTROYed after every
set_up_table call, and that generates these harmless but annoying
warnings.  I fixed this by keeping a private cache of handles used
before the fork, and making sure they never get used after forking.

Once could argue that Apache;:DBI really needs some changes to play
better with different kinds of AutoCommit behavior, and I would agree,
but that's a larger issue and would probably still require some
cooperation from Ima::DBI.  Also, my knowledge of how DBI deals with
forking continues to evolve, and there may be a nicer way to deal with
this (or maybe the ping check in Ima::DBI should deal with it), but the
approach in this patch solves the known issues now, and can be improved
later if we find a better way.

I respect your choice to try to limit the expansion of the Ima::DBI and
Class::DBI codebases.  It makes sense to me for the maintainer of a
popular module to be conservative about patches.  At the moment though,
the support for adding plugins to either is not very good, and require
some hacky stuff like shoving methods into other classes' namespaces.  I
don't have a better suggestion yet, but I feel like something cleaner
must be possible, and it would encourage more people to go the plugin
route.

I also think it may eventually become necessary to hand off some of the
development chores to a group of trusted others, just as it has been
with large projects like Linux, simply due to your time constraints.
Even when a module isn't getting new features added, a growing userbase
means more support is needed for assistance with extensions, bug fixes,
testing on new Perl releases, and keeping up with modern perl techniques
(e.g. who used Test::More and DBD::SQLite for test suites a few years
ago?).

- Perrin