Bug 121069: Remove $dbh->bz_(un)lock_tables from process_bug.cgi and Error.pm in favor of DB transactions. These methods are no longer used and are completely removed now - Patch by Frédéric Buclin <LpSolit@gmail.com> r/a=mkanat

6 files changed, 13 insertions, 208 deletions

diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm
index ee064bb85..31427bc82 100644
--- a/Bugzilla/Constants.pm
+++ b/Bugzilla/Constants.pm
@@ -91,7 +91,6 @@ use File::Basename;
     CMT_POPULAR_VOTES
     CMT_MOVED_TO
 
-    UNLOCK_ABORT
     THROW_ERROR
     
     RELATIONSHIPS
@@ -269,10 +268,6 @@ use constant CMT_HAS_DUPE => 2;
 use constant CMT_POPULAR_VOTES => 3;
 use constant CMT_MOVED_TO => 4;
 
-# used by Bugzilla::DB to indicate that tables are being unlocked
-# because of error
-use constant UNLOCK_ABORT => 1;
-
 # Determine whether a validation routine should return 0 or throw
 # an error when the validation fails.
 use constant THROW_ERROR => 1;
diff --git a/Bugzilla/DB.pm b/Bugzilla/DB.pm
index 2a71bcd75..4a0303287 100644
--- a/Bugzilla/DB.pm
+++ b/Bugzilla/DB.pm
@@ -273,8 +273,7 @@ EOT
 # List of abstract methods we are checking the derived class implements
 our @_abstract_methods = qw(REQUIRED_VERSION PROGRAM_NAME DBD_VERSION
                             new sql_regexp sql_not_regexp sql_limit sql_to_days
-                            sql_date_format sql_interval
-                            bz_lock_tables bz_unlock_tables);
+                            sql_date_format sql_interval);
 
 # This overridden import method will check implementation of inherited classes
 # for missing implementation of abstract methods
@@ -301,62 +300,6 @@ sub import {
     $Exporter::ExportLevel-- if $is_exporter;
 }
 
-sub bz_lock_tables {
-    my ($self, @tables) = @_;
-
-    my $list = join(', ', @tables);
-    # Check first if there was no lock before
-    if ($self->{private_bz_tables_locked}) {
-        ThrowCodeError("already_locked", { current => $self->{private_bz_tables_locked},
-                                           new => $list });
-    } else {
-        my %read_tables;
-        my %write_tables;
-        foreach my $table (@tables) {
-            $table =~ /^([\d\w]+)([\s]+AS[\s]+[\d\w]+)?[\s]+(WRITE|READ)$/i;
-            my $table_name = $1;
-            if ($3 =~ /READ/i) {
-                if (!exists $read_tables{$table_name}) {
-                    $read_tables{$table_name} = undef;
-                }
-            }
-            else {
-                if (!exists $write_tables{$table_name}) {
-                    $write_tables{$table_name} = undef;
-                }
-            }
-        }
-
-        # Begin Transaction
-        $self->bz_start_transaction();
-
-        Bugzilla->dbh->do('LOCK TABLE ' . join(', ', keys %read_tables) .
-                          ' IN ROW SHARE MODE') if keys %read_tables;
-        Bugzilla->dbh->do('LOCK TABLE ' . join(', ', keys %write_tables) .
-                          ' IN ROW EXCLUSIVE MODE') if keys %write_tables;
-        $self->{private_bz_tables_locked} = $list;
-    }
-}
-
-sub bz_unlock_tables {
-    my ($self, $abort) = @_;
-
-    # Check first if there was previous matching lock
-    if (!$self->{private_bz_tables_locked}) {
-        # Abort is allowed even without previous lock for error handling
-        return if $abort;
-        ThrowCodeError("no_matching_lock");
-    } else {
-        $self->{private_bz_tables_locked} = "";
-        # End transaction, tables will be unlocked automatically
-        if ($abort) {
-            $self->bz_rollback_transaction();
-        } else {
-            $self->bz_commit_transaction();
-        }
-    }
-}
-
 sub sql_istrcmp {
     my ($self, $left, $right, $op) = @_;
     $op ||= "=";
@@ -1949,63 +1892,6 @@ Formatted SQL for the C<IN> operator.
 
 =back
 
-=item C<bz_lock_tables>
-
-=over
-
-=item B<Description>
-
-Performs a table lock operation on specified tables. If the underlying 
-database supports transactions, it should also implicitly start a new 
-transaction.
-
-Abstract method, should be overridden by database specific code.
-
-=item B<Params>
-
-=over
-
-=item C<@tables> - list of names of tables to lock in MySQL
-notation (ex. 'bugs AS bugs2 READ', 'logincookies WRITE')
-
-=back
-
-=item B<Returns> (nothing)
-
-=back
-
-=item C<bz_unlock_tables>
-
-=over
-
-=item B<Description>
-
-Performs a table unlock operation.
-
-If the underlying database supports transactions, it should also implicitly 
-commit or rollback the transaction.
-
-Also, this function should allow to be called with the abort flag
-set even without locking tables first without raising an error
-to simplify error handling.
-
-Abstract method, should be overridden by database specific code.
-
-=item B<Params>
-
-=over
-
-=item C<$abort> - C<UNLOCK_ABORT> if the operation on locked tables
-failed (if transactions are supported, the action will be rolled
-back). No param if the operation succeeded. This is only used by
-L<Bugzilla::Error/throw_error>.
-
-=back
-
-=item B<Returns> (none)
-
-=back
-
 =back
 
 
diff --git a/Bugzilla/DB/Mysql.pm b/Bugzilla/DB/Mysql.pm
index 9e0d25277..720c8ff42 100644
--- a/Bugzilla/DB/Mysql.pm
+++ b/Bugzilla/DB/Mysql.pm
@@ -200,44 +200,6 @@ sub sql_group_by {
 }
 
 
-sub bz_lock_tables {
-    my ($self, @tables) = @_;
-
-    my $list = join(', ', @tables);
-    # Check first if there was no lock before
-    if ($self->{private_bz_tables_locked}) {
-        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}) {
-        # Abort is allowed even without previous lock for error handling
-        return if $abort;
-        ThrowCodeError("no_matching_lock");
-    } else {
-        $self->do("UNLOCK TABLES");
-        $self->{private_bz_tables_locked} = "";
-    }
-}
-
 sub _bz_get_initial_schema {
     my ($self) = @_;
     return $self->_bz_build_schema_from_disk();
diff --git a/Bugzilla/Error.pm b/Bugzilla/Error.pm
index 18f9aae54..3e5688e2a 100644
--- a/Bugzilla/Error.pm
+++ b/Bugzilla/Error.pm
@@ -46,16 +46,15 @@ sub _in_eval {
 
 sub _throw_error {
     my ($name, $error, $vars) = @_;
-
+    my $dbh = Bugzilla->dbh;
     $vars ||= {};
 
     $vars->{error} = $error;
 
-    # Make sure any locked tables are unlocked
-    # and the transaction is rolled back (if supported)
-    # If we are within an eval(), do not unlock tables as we are
+    # Make sure any transaction is rolled back (if supported).
+    # If we are within an eval(), do not roll back transactions as we are
     # eval'uating some test on purpose.
-    Bugzilla->dbh->bz_unlock_tables(UNLOCK_ABORT) unless _in_eval();
+    $dbh->bz_rollback_transaction() if ($dbh->bz_in_transaction() && !_in_eval());
 
     my $datadir = bz_locations()->{'datadir'};
     # If a writable $datadir/errorlog exists, log error details there.
@@ -124,10 +123,10 @@ sub ThrowCodeError {
 
 sub ThrowTemplateError {
     my ($template_err) = @_;
+    my $dbh = Bugzilla->dbh;
 
-    # Make sure any locked tables are unlocked
-    # and the transaction is rolled back (if supported)
-    Bugzilla->dbh->bz_unlock_tables(UNLOCK_ABORT);
+    # Make sure the transaction is rolled back (if supported).
+    $dbh->bz_rollback_transaction() if $dbh->bz_in_transaction();
 
     my $vars = {};
     if (Bugzilla->error_mode == ERROR_MODE_DIE) {
diff --git a/process_bug.cgi b/process_bug.cgi
index 912440ce2..8eb7aaf33 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -398,17 +398,7 @@ if ($move_action eq Bugzilla->params->{'move-button-text'}) {
     $user->is_mover || ThrowUserError("auth_failure", {action => 'move',
                                                        object => 'bugs'});
 
-    my @multi_select_locks  = map {'bug_' . $_->name . " WRITE"}
-        Bugzilla->get_fields({ custom => 1, type => FIELD_TYPE_MULTI_SELECT,
-                               obsolete => 0 });
-
-    $dbh->bz_lock_tables('bugs WRITE', 'bugs_activity WRITE', 'duplicates WRITE',
-                         'longdescs WRITE', 'profiles READ', 'groups READ',
-                         'bug_group_map READ', 'group_group_map READ',
-                         'user_group_map READ', 'classifications READ',
-                         'products READ', 'components READ', 'votes READ',
-                         'cc READ', 'fielddefs READ', 'bug_status READ',
-                         'status_workflow READ', 'resolution READ', @multi_select_locks);
+    $dbh->bz_start_transaction();
 
     # First update all moved bugs.
     foreach my $bug (@bug_objects) {
@@ -430,7 +420,7 @@ if ($move_action eq Bugzilla->params->{'move-button-text'}) {
         $bug->_clear_dup_id;
     }
     $_->update() foreach @bug_objects;
-    $dbh->bz_unlock_tables();
+    $dbh->bz_commit_transaction();
 
     # Now send emails.
     foreach my $bug (@bug_objects) {
@@ -510,28 +500,9 @@ foreach my $b (@bug_objects) {
 # Do Actual Database Updates #
 ##############################
 foreach my $bug (@bug_objects) {
-    my $write = "WRITE";        # Might want to make a param to control
-                                # whether we do LOW_PRIORITY ...
-
-    my @multi_select_locks  = map {'bug_' . $_->name . " $write"}
-        Bugzilla->get_fields({ custom => 1, type => FIELD_TYPE_MULTI_SELECT,
-                               obsolete => 0 });
-
-    $dbh->bz_lock_tables("bugs $write", "bugs_activity $write", "cc $write",
-            "profiles READ", "dependencies $write", "votes $write",
-            "products READ", "components READ", "milestones READ",
-            "keywords $write", "longdescs $write", "fielddefs READ",
-            "bug_group_map $write", "flags $write", "duplicates $write",
-            "user_group_map READ", "group_group_map READ", "flagtypes READ",
-            "flaginclusions AS i READ", "flagexclusions AS e READ",
-            "keyworddefs READ", "groups READ", "attachments READ",
-            "bug_status READ", "group_control_map AS oldcontrolmap READ",
-            "group_control_map AS newcontrolmap READ",
-            "group_control_map READ", "email_setting READ", 
-            "classifications READ", @multi_select_locks);
-    
-    my $timestamp = $dbh->selectrow_array(q{SELECT NOW()});
+    $dbh->bz_start_transaction();
 
+    my $timestamp = $dbh->selectrow_array(q{SELECT NOW()});
     my $changes = $bug->update($timestamp);
 
     my %notify_deps;
@@ -581,7 +552,7 @@ foreach my $bug (@bug_objects) {
     # Set and update flags.
     Bugzilla::Flag::process($bug, undef, $timestamp, $cgi, $vars);
 
-    $dbh->bz_unlock_tables();
+    $dbh->bz_commit_transaction();
 
     ###############
     # Send Emails #
diff --git a/template/en/default/global/code-error.html.tmpl b/template/en/default/global/code-error.html.tmpl
index 1e5581e13..7e0e1cd70 100644
--- a/template/en/default/global/code-error.html.tmpl
+++ b/template/en/default/global/code-error.html.tmpl
@@ -416,14 +416,6 @@
   [% ELSIF error == "not_in_transaction" %]
     Attempted to end transaction without starting one first.
 
-  [% ELSIF error == "already_locked" %]
-    Attempted to lock a table without releasing previous lock first:
-    <p>Tables already locked:<br>[% current FILTER html %]
-    <p>Tables requesting locking:<br>[% new FILTER html %]
-
-  [% ELSIF error == "no_matching_lock" %]
-    Attempted to unlock tables without locking them first.
-
   [% ELSIF error == "comma_operator_deprecated" %]
     [% title = "SQL query generator internal error" %]
     There is an internal error in the SQL query generation code,