Closed Bug 399163 Opened 17 years ago Closed 17 years ago

Bugzilla/* should use transactions for database interaction

Categories

(Bugzilla :: Database, enhancement)

3.1.2
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 3.2

People

(Reporter: emmanuel, Assigned: emmanuel)

References

Details

Attachments

(1 file, 3 obsolete files)

Bugzilla's modules should use transactions instead of bz_lock_tables and bz_unlock_tables.
Attached patch cvs diff -u of the replacements (obsolete) — Splinter Review
Attachment #284435 - Flags: review?(mkanat)
Comment on attachment 284435 [details] [diff] [review]
cvs diff -u of the replacements

>Index: Bugzilla/Bug.pm
>@@ -758,7 +753,7 @@
>     $dbh->do("DELETE FROM attachments WHERE bug_id = ?", undef, $bug_id);
>     $dbh->do("DELETE FROM bugs WHERE bug_id = ?", undef, $bug_id);
> 
>-    $dbh->bz_unlock_tables();
>+    $dbh->bz_commit_transaction();

  You have to move longdescs outside the transaction. (Put it after the commit.) Anything that modifies longdescs has to happen outside a transaction.

>Index: Bugzilla/Token.pm
> sub CleanTokenTable {
>     my $dbh = Bugzilla->dbh;
>-    $dbh->bz_lock_tables('tokens WRITE');
>+    $dbh->bz_start_transaction();
>     $dbh->do('DELETE FROM tokens
>               WHERE ' . $dbh->sql_to_days('NOW()') . ' - ' .
>                         $dbh->sql_to_days('issuedate') . ' >= ?',
>               undef, MAX_TOKEN_AGE);
>-    $dbh->bz_unlock_tables();
>+    $dbh->bz_commit_transaction();

  That doesn't need a lock or a transaction, just remove the locks.

>-    $dbh->bz_lock_tables('tokens WRITE');
>+    $dbh->bz_start_transaction();
>     $dbh->do("DELETE FROM tokens WHERE token = ?", undef, $token);
>-    $dbh->bz_unlock_tables();
>+    $dbh->bz_commit_transaction();

  Nor does that. Single statements don't benefit from transactions, really.

>Index: Bugzilla/Install/DB.pm
> [snip[
>-        $dbh->bz_lock_tables('bugs write', 'longdescs write', 'profiles write',
>-                             'bz_schema WRITE');
>+        $dbh->bz_start_transaction();

  We need a comment above this:
  # On MySQL, longdescs doesn't benefit from transactions, but this doesn't hurt.


>Index: Bugzilla/Search/Saved.pm
>     # Right now you can only create a Saved Search for the current user.
>     $params->{userid} = Bugzilla->user->id;
> 
>-    $dbh->bz_lock_tables('namedqueries WRITE',
>-                         'namedqueries_link_in_footer WRITE');
>+    $dbh->bz_start_transaction();

  Move start_transaction above run_create_validators.
Attachment #284435 - Flags: review?(mkanat) → review-
Attachment #284435 - Attachment is obsolete: true
Attachment #288295 - Flags: review?(mkanat)
Comment on attachment 288295 [details] [diff] [review]
Taking into account mkanat's comments on the previous patch

This looks OK. I'm a little concerned about the possibility of some things in Install::DB auto-committing the transaction (like doing drop_column), and then COMMIT throwing an error, but it's hard to know if that will happen unless we check it in and see what happens in the checksetup regression test.
Attachment #288295 - Flags: review?(mkanat) → review+
  Manu, do you have a CVS account yet? You really should get one so that we don't have to check in all your patches.
Severity: normal → enhancement
Flags: approval+
Target Milestone: --- → Bugzilla 3.2
Comment on attachment 288295 [details] [diff] [review]
Taking into account mkanat's comments on the previous patch

>Index: Bugzilla/Bug.pm
>-    $dbh->bz_unlock_tables();
>+    $dbh->bz_commit_transaction();
>+
>+    $dbh->do("DELETE FROM attachments WHERE bug_id = ?", undef, $bug_id);

  Why did you move that outside the transaction? It should stay inside the transaction.

  It's longdescs that should be outside, not attachments.
Attachment #288295 - Flags: review+ → review-
Flags: approval+
(In reply to comment #6)
>
>   It's longdescs that should be outside, not attachments.

Sorry. I have no idea why I did that.
Attachment #288295 - Attachment is obsolete: true
Attachment #288638 - Flags: review?(mkanat)
Comment on attachment 288638 [details] [diff] [review]
move longdescs out of the transaction, not attachements

>Index: Bugzilla/Series.pm
>@@ -170,7 +170,7 @@
>     my $self = shift;
> 
>     my $dbh = Bugzilla->dbh;
>-    $dbh->bz_lock_tables('series_categories WRITE', 'series WRITE');
>+    $dbh->bz_commit_transaction();

  That should be start.

>Index: Bugzilla/Install/DB.pm

  What happened to the comment in this file?

> Index: Bugzilla/Search/Saved.pm

  And you reverted the fixes in this file.

  I think you updated the wrong patch.
Attachment #288638 - Flags: review?(mkanat) → review-
Attached patch v3Splinter Review
Attachment #288638 - Attachment is obsolete: true
Attachment #288732 - Flags: review?(mkanat)
Comment on attachment 288732 [details] [diff] [review]
v3

Yes, this one is correct. Thank you! :-)
Attachment #288732 - Flags: review?(mkanat) → review+
Flags: approval+
Comment on attachment 288732 [details] [diff] [review]
v3

>Index: Bugzilla/Search/Saved.pm

>+    $dbh->bz_start_transaction();
>     my $params = $class->run_create_validators(@_);
> 
>     # Right now you can only create a Saved Search for the current user.
>     $params->{userid} = Bugzilla->user->id;
> 
>-    $dbh->bz_lock_tables('namedqueries WRITE',
>-                         'namedqueries_link_in_footer WRITE');

No need to move bz_start_transaction() before run_create_validators() as it doesn't look at the DB at all. No race condition can occur.
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.216; previous revision: 1.215
done
Checking in Bugzilla/Series.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Series.pm,v  <--  Series.pm
new revision: 1.16; previous revision: 1.15
done
Checking in Bugzilla/Token.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Token.pm,v  <--  Token.pm
new revision: 1.54; previous revision: 1.53
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.162; previous revision: 1.161
done
Checking in Bugzilla/Install/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v  <--  DB.pm
new revision: 1.42; previous revision: 1.41
done
Checking in Bugzilla/Search/Saved.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search/Saved.pm,v  <--  Saved.pm
new revision: 1.6; previous revision: 1.5
done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: