301020 - Remove useless locked tables

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Frédéric Buclin]

Some tables were locked because Throw*Error() did not automatically unlocked
tables, see bug 277782. Now that this problem has been fixed, some tables do not
need to be locked anymore.

Some other locked tables are also useless due to code rewrite.

Let's hope this will improve Bugzilla performance a bit.

Comment 1

[Frédéric Buclin]

Attachment: patch, v1

Comment 2

[Frédéric Buclin]

Comment on attachment 189532 [details] [diff] [review]
patch, v1

asking wurblzap too for the tiny optimisation in editusers.cgi, as discussed
yesterday per IRC.

Comment 3

[Marc Schumann [:Wurblzap]]

Comment on attachment 189532 [details] [diff] [review]
patch, v1

Only one reason for r-.

>RCS file: /cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v

Tested; works.

>RCS file: /cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v

Tested; works.

>@@ -282,7 +277,7 @@
>         }
>         if (@changedFields) {
>             push (@values, $otherUserID);
>-            $logoutNeeded && Bugzilla->logout_user_by_id($otherUserID);
>+            $logoutNeeded && Bugzilla->logout_user($otherUser);
>             $dbh->do('UPDATE profiles SET ' .
>                      join(' = ?,', @changedFields).' = ? ' .
>                      'WHERE userid = ?',
>@@ -502,7 +497,7 @@
>     @{$otherUser->product_responsibilities()}
>         && ThrowUserError('user_has_responsibility');
> 
>-    Bugzilla->logout_user_by_id($otherUserID);
>+    Bugzilla->logout_user($otherUser);
> 
>     # Reference removals.
>     $dbh->do('UPDATE flags set requestee_id = NULL WHERE requestee_id = ?',

Good optimizations. Let's take them into this bug while we're here.

>RCS file: /cvsroot/mozilla/webtools/bugzilla/votes.cgi,v

Not tested yet.

>@@ -347,15 +342,28 @@
>     
>     # Update the cached values in the bugs table
>     print $cgi->header();
>+    my @updated_bugs = ();
>     foreach my $id (keys %affected) {
>-        SendSQL("SELECT sum(vote_count) FROM votes WHERE bug_id = $id");
>-        my $v = FetchOneColumn() || 0;
>-        SendSQL("UPDATE bugs SET votes = $v WHERE bug_id = $id");
>+        my $v = $dbh->selectrow_array("SELECT sum(vote_count) FROM votes
>+                                       WHERE bug_id = ?", undef, $id) || 0;
>+
>+        $dbh->do("UPDATE bugs SET votes = ? WHERE bug_id = ?",
>+                 undef, ($v, $id));
>+
>         my $confirmed = CheckIfVotedConfirmed($id, $who);
>-        $vars->{'header_done'} = 1 if $confirmed;
>+        push (@updated_bugs, $id) if $confirmed;
>     }
>-
>     $dbh->bz_unlock_tables();
> 
>+    $vars->{'type'} = "votes";
>+    $vars->{'mailrecipients'} = { 'changer' => $who };
>+
>+    foreach my $bug_id (@updated_bugs) {
>+        $vars->{'id'} = $bug_id;
>+        $template->process("bug/process/results.html.tmpl", $vars)
>+          || ThrowTemplateError($template->error());
>+        # Set header_done to 1 only after the first bug.
>+        $vars->{'header_done'} = 1;
>+    }
>     $vars->{'votes_recorded'} = 1;
> }

I don't understand why you didn't handle this in a separate bug :(
But while it's here... I agree very much that this optimization needs to be
done. The reason for r- is that the two SQL commands within the first loop need
to be moved out of the loop into prepared statement handles. The loop should
use $sth->execute.

>RCS file: /cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v

Tested (including vote count checks); works for me.

Again, an optimization which does make sense but is not part of this bug :(

>         # 3. enough votes to confirm

It seems to me that the part following this should be put into a table lock
bracket of its own, ending before the mail sending loop. If you agree, please
open a bug for it.

>RCS file: /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v

Tested by random bug changes (multiple, too); works for me.

>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v

:(

Comment 4

[Frédéric Buclin]

Attachment: patch, v2

This patch requires a careful testing... ;)

Comment 5

[Frédéric Buclin]

Attachment: patch, v2.5

This patch removes the two changes from "logout_user_by_id" to "logout_user" which are added in bug 314039. This way, this bug won't prevent the checkin of bug 314039. ;)

Comment 6

[Marc Schumann [:Wurblzap]]

Comment on attachment 204492 [details] [diff] [review]
patch, v2.5

This has rotted slightly, but unrotting is trivial – r=wurblzap on an unrotted patch.

Tested; works. I hope I caught all code paths... It's a good thing we're going to check this in near the beginning of a release cycle.

Comment 7

[Frédéric Buclin]

Attachment: unrotted patch, v2.6

carrying forward Marc's r+

Comment 8

[Frédéric Buclin]

Comment on attachment 216565 [details] [diff] [review]
unrotted patch, v2.6

err.. there is a typo

Comment 9

[Frédéric Buclin]

Attachment: fixed typo, v2.7

Comment 10

[Frédéric Buclin]

Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <--  editcomponents.cgi
new revision: 1.71; previous revision: 1.70
done
Checking in editgroups.cgi;
/cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v  <--  editgroups.cgi
new revision: 1.69; previous revision: 1.68
done
Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.117; previous revision: 1.116
done
Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.117; previous revision: 1.116
done
Checking in editversions.cgi;
/cvsroot/mozilla/webtools/bugzilla/editversions.cgi,v  <--  editversions.cgi
new revision: 1.46; previous revision: 1.45
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.311; previous revision: 1.310
done
Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
new revision: 1.36; previous revision: 1.35
done