Bug 374004: Enable transaction code and use it in some installation places

Patch By Max Kanat-Alexander <mkanat@bugzilla.org> (module owner) a=mkanat

5 files changed, 64 insertions, 45 deletions

diff --git a/Bugzilla.pm b/Bugzilla.pm
index f278adb2e..473d959fc 100644
--- a/Bugzilla.pm
+++ b/Bugzilla.pm
@@ -412,14 +412,15 @@ sub request_cache {
 
 # Private methods
 
-# Per process cleanup
+# Per-process cleanup
 sub _cleanup {
-
-    # When we support transactions, need to ->rollback here
     my $main   = request_cache()->{dbh_main};
     my $shadow = request_cache()->{dbh_shadow};
-    $main->disconnect if $main;
-    $shadow->disconnect if $shadow && Bugzilla->params->{"shadowdb"};
+    foreach my $dbh ($main, $shadow) {
+        next if !$dbh;
+        $dbh->bz_rollback_transaction() if $dbh->bz_in_transaction;
+        $dbh->disconnect;
+    }
     undef $_request_cache;
 }
 
diff --git a/Bugzilla/DB.pm b/Bugzilla/DB.pm
index 22c6bbafa..095ba27a6 100644
--- a/Bugzilla/DB.pm
+++ b/Bugzilla/DB.pm
@@ -859,39 +859,51 @@ sub bz_table_list_real {
 # Transaction Methods
 #####################################################################
 
+sub bz_in_transaction {
+    return $_[0]->{private_bz_transaction_count} ? 1 : 0;
+}
+
 sub bz_start_transaction {
     my ($self) = @_;
 
-    if ($self->{private_bz_in_transaction}) {
-        ThrowCodeError("nested_transaction");
+    if ($self->bz_in_transaction) {
+        $self->{private_bz_transaction_count}++;
     } else {
         # Turn AutoCommit off and start a new transaction
         $self->begin_work();
-        $self->{private_bz_in_transaction} = 1;
+        # REPEATABLE READ means "We work on a snapshot of the DB that
+        # is created when we execute our first SQL statement." It's
+        # what we need in Bugzilla to be safe, for what we do.
+        # Different DBs have different defaults for their isolation
+        # level, so we just set it here manually.
+        $self->do('SET TRANSACTION ISOLATION LEVEL REPEATABLE READ');
+        $self->{private_bz_transaction_count} = 1;
     }
 }
 
 sub bz_commit_transaction {
     my ($self) = @_;
-
-    if (!$self->{private_bz_in_transaction}) {
-        ThrowCodeError("not_in_transaction");
-    } else {
+    
+    if ($self->{private_bz_transaction_count} > 1) {
+        $self->{private_bz_transaction_count}--;
+    } elsif ($self->bz_in_transaction) {
         $self->commit();
-
-        $self->{private_bz_in_transaction} = 0;
+        $self->{private_bz_transaction_count} = 0;
+    } else {
+       ThrowCodeError('not_in_transaction');
     }
 }
 
 sub bz_rollback_transaction {
     my ($self) = @_;
 
-    if (!$self->{private_bz_in_transaction}) {
+    # Unlike start and commit, if we rollback at any point it happens
+    # instantly, even if we're in a nested transaction.
+    if (!$self->bz_in_transaction) {
         ThrowCodeError("not_in_transaction");
     } else {
         $self->rollback();
-
-        $self->{private_bz_in_transaction} = 0;
+        $self->{private_bz_transaction_count} = 0;
     }
 }
 
@@ -928,9 +940,6 @@ sub db_new {
     # above "die" condition.
     $self->{RaiseError} = 1;
 
-    # class variables
-    $self->{private_bz_in_transaction} = 0;
-
     bless ($self, $class);
 
     return $self;
@@ -2212,20 +2221,33 @@ in the database.
 
 =over
 
+=item C<bz_in_transaction>
+
+Returns C<1> if we are currently in the middle of an uncommitted transaction,
+C<0> otherwise.
+
 =item C<bz_start_transaction>
 
-Starts a transaction if supported by the database being used. Returns nothing
-and takes no parameters.
+Starts a transaction.
+
+It is OK to call C<bz_start_transaction> when you are already inside of
+a transaction. However, you must call L</bz_commit_transaction> as many
+times as you called C<bz_start_transaction>, in order for your transaction
+to actually commit.
+
+Bugzilla uses C<REPEATABLE READ> transactions.
+
+Returns nothing and takes no parameters.
 
 =item C<bz_commit_transaction>
 
-Ends a transaction, commiting all changes, if supported by the database 
-being used. Returns nothing and takes no parameters.
+Ends a transaction, commiting all changes. Returns nothing and takes
+no parameters.
 
 =item C<bz_rollback_transaction>
 
-Ends a transaction, rolling back all changes, if supported by the database 
-being used. Returns nothing and takes no parameters.
+Ends a transaction, rolling back all changes. Returns nothing and takes 
+no parameters.
 
 =back
 
diff --git a/Bugzilla/DB/Mysql.pm b/Bugzilla/DB/Mysql.pm
index 582a2a7b8..582df3d54 100644
--- a/Bugzilla/DB/Mysql.pm
+++ b/Bugzilla/DB/Mysql.pm
@@ -207,14 +207,23 @@ sub bz_lock_tables {
         ThrowCodeError("already_locked", { current => $self->{private_bz_tables_locked},
                                            new => $list });
     } else {
+        $self->bz_start_transaction();
         $self->do('LOCK TABLE ' . $list); 
-    
         $self->{private_bz_tables_locked} = $list;
     }
 }
 
 sub bz_unlock_tables {
     my ($self, $abort) = @_;
+
+    if ($self->bz_in_transaction) {
+        if ($abort) {
+            $self->bz_rollback_transaction();
+        }
+        else {
+            $self->bz_commit_transaction();
+        }
+    }
     
     # Check first if there was previous matching lock
     if (!$self->{private_bz_tables_locked}) {
@@ -223,28 +232,10 @@ sub bz_unlock_tables {
         ThrowCodeError("no_matching_lock");
     } else {
         $self->do("UNLOCK TABLES");
-    
         $self->{private_bz_tables_locked} = "";
     }
 }
 
-# As Bugzilla currently runs on MyISAM storage, which does not support
-# transactions, these functions die when called.
-# Maybe we should just ignore these calls for now, but as we are not
-# using transactions in MySQL yet, this just hints the developers.
-sub bz_start_transaction {
-    die("Attempt to start transaction on DB without transaction support");
-}
-
-sub bz_commit_transaction {
-    die("Attempt to commit transaction on DB without transaction support");
-}
-
-sub bz_rollback_transaction {
-    die("Attempt to rollback transaction on DB without transaction support");
-}
-
-
 sub _bz_get_initial_schema {
     my ($self) = @_;
     return $self->_bz_build_schema_from_disk();
diff --git a/Bugzilla/Install/DB.pm b/Bugzilla/Install/DB.pm
index 6ddca06bd..7d4939877 100644
--- a/Bugzilla/Install/DB.pm
+++ b/Bugzilla/Install/DB.pm
@@ -2205,6 +2205,9 @@ sub _migrate_email_prefs_to_new_table {
         my %requestprefs = ("FlagRequestee" => EVT_FLAG_REQUESTED,
                             "FlagRequester" => EVT_REQUESTED_FLAG);
 
+        # We run the below code in a transaction to speed things up.
+        $dbh->bz_start_transaction();
+
         # Select all emailflags flag strings
         my $sth = $dbh->prepare("SELECT userid, emailflags FROM profiles");
         $sth->execute();
@@ -2271,6 +2274,7 @@ sub _migrate_email_prefs_to_new_table {
         # EVT_ATTACHMENT.
         _clone_email_event(EVT_ATTACHMENT, EVT_ATTACHMENT_DATA);
 
+        $dbh->bz_commit_transaction();
         $dbh->bz_drop_column("profiles", "emailflags");
     }
 }
diff --git a/mod_perl.pl b/mod_perl.pl
index cb2f635f9..f88c398e1 100644
--- a/mod_perl.pl
+++ b/mod_perl.pl
@@ -102,6 +102,7 @@ use Apache2::Const -compile => qw(OK);
 sub handler {
     my $r = shift;
 
+    Bugzilla::_cleanup();
     # Sometimes mod_perl doesn't properly call DESTROY on all
     # the objects in pnotes()
     foreach my $key (keys %{$r->pnotes}) {