Closed
Bug 399163
Opened 17 years ago
Closed 17 years ago
Bugzilla/* should use transactions for database interaction
Categories
(Bugzilla :: Database, enhancement)
Tracking
()
RESOLVED
FIXED
Bugzilla 3.2
People
(Reporter: emmanuel, Assigned: emmanuel)
References
Details
Attachments
(1 file, 3 obsolete files)
7.71 KB,
patch
|
mkanat
:
review+
|
Details | Diff | Splinter Review |
Bugzilla's modules should use transactions instead of bz_lock_tables and bz_unlock_tables.
Assignee | ||
Comment 1•17 years ago
|
||
Attachment #284435 -
Flags: review?(mkanat)
Comment 2•17 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•17 years ago
|
||
Attachment #284435 -
Attachment is obsolete: true
Attachment #288295 -
Flags: review?(mkanat)
Comment 4•17 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•17 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•17 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•17 years ago
|
Flags: approval+
Assignee | ||
Comment 7•17 years ago
|
||
(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•17 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•17 years ago
|
||
Attachment #288638 -
Attachment is obsolete: true
Attachment #288732 -
Flags: review?(mkanat)
Comment 10•17 years ago
|
||
Comment on attachment 288732 [details] [diff] [review] v3 Yes, this one is correct. Thank you! :-)
Attachment #288732 -
Flags: review?(mkanat) → review+
Updated•17 years ago
|
Flags: approval+
Comment 11•17 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•17 years ago
|
Blocks: bz-transactions
Comment 12•17 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
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•