Bugzilla/* should use transactions for database interaction

RESOLVED FIXED in Bugzilla 3.2

Status

()

Bugzilla
Database
--
enhancement
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Emmanuel Seyman, Assigned: Emmanuel Seyman)

Tracking

3.1.2
Bugzilla 3.2
Bug Flags:
approval +

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

11 years ago
Bugzilla's modules should use transactions instead of bz_lock_tables and bz_unlock_tables.
(Assignee)

Comment 1

11 years ago
Created attachment 284435 [details] [diff] [review]
cvs diff -u of the replacements
Attachment #284435 - Flags: review?(mkanat)

Comment 2

11 years ago
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-
(Assignee)

Comment 3

10 years ago
Created attachment 288295 [details] [diff] [review]
Taking into account mkanat's comments on the previous patch
Attachment #284435 - Attachment is obsolete: true
Attachment #288295 - Flags: review?(mkanat)

Comment 4

10 years ago
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+

Comment 5

10 years ago
  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 6

10 years ago
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-

Updated

10 years ago
Flags: approval+
(Assignee)

Comment 7

10 years ago
Created attachment 288638 [details] [diff] [review]
move longdescs out of the transaction, not attachements

(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 8

10 years ago
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-
(Assignee)

Comment 9

10 years ago
Created attachment 288732 [details] [diff] [review]
v3
Attachment #288638 - Attachment is obsolete: true
Attachment #288732 - Flags: review?(mkanat)

Comment 10

10 years ago
Comment on attachment 288732 [details] [diff] [review]
v3

Yes, this one is correct. Thank you! :-)
Attachment #288732 - Flags: review?(mkanat) → review+

Updated

10 years ago
Flags: approval+

Comment 11

10 years ago
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.

Updated

10 years ago
Blocks: 121069

Comment 12

10 years ago
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
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.