Skip Menu |

This queue is for tickets about the DBI CPAN distribution.

Report information
The Basics
Id: 17289
Status: resolved
Priority: 0/
Queue: DBI

People
Owner: Nobody in particular
Requestors: dom [...] idealx.com
Cc:
AdminCc:

Bug Information
Severity: Normal
Broken in: 1.50
Fixed in: (no value)



Subject: rollback() at db::DESTROY = cargo cult?
I see a highly suspicious comment around line 343 of Driver.xst: Show quoted text
> The application has not explicitly disconnected. That's bad. > To ensure integrity we *must* issue a rollback. [...] if we don't > rollback, the server may automatically commit! Bham! > Corrupt database!
Are there actually eyewitnesses of any database doing this? It would definitely be a critical bug on its behalf. E.g. PostgreSQL will never do this; it waits for the client app to issue an *explicit* commit statement, as it should. If the Pg socket goes down the tubes halfway through a transaction, the server will simply rollback it (it's easy to verify with psql: start a transaction, insert some tuples, then killall -9 psql; restart psql, the tuples are gone). Please remove this preventive rollback, or at least move it into the DBD's of the buggy databases if any. For a properly ACID-compliant DBMS, the rollback-on-DESTROY behavior is useless, if not harmful (if the rollback itself fails, e.g. because the database itself went down before DESTROY, entire provinces of heck may break loose). Pursuant to the Perl way of things, it seems desirable that one be able to safely forget about a DBI handle that is no longer needed, as is already the case with all other kinds of Perl objects, XS or not (e.g. filehandles, sockets, GTK widgets...)
I've updated the comment in the code to read: if (!DBIc_has(imp_dbh,DBIcf_AutoCommit)) { /* Application is using transactions and hasn't explicitly disconnected. Some databases will automatically commit on graceful disconnect. Since we're about to gracefully disconnect as part of the DESTROY we want to be sure we're not about to implicitly commit changes that are incomplete and should be rolled back. (The DESTROY may be due to a RaiseError, for example.) So we rollback here. This will be harmless if the application has issued a commit, XXX Could add an attribute flag to indicate that the driver doesn't have this problem. Patches welcome. XXX or could just move the DBIc_is(imp_dbh, DBIcf_Executed) test to cover the rollback as well. That just needs sanity checking that DBIcf_Executed is set by any/all possible way to execute a statement that might start a transaction. */ if (DBIc_WARN(imp_dbh) && DBIc_is(imp_dbh, DBIcf_Executed) /* has not just called commit/rollback */ && (!dirty || DBIc_DBISTATE(imp_dbh)->debug >= 3) ) warn("Issuing rollback() for database handle being DESTROY'd without explicit disconnect()"); dbd_db_rollback(dbh, imp_dbh); /* ROLLBACK! */ }