280503 - Replace "LOCK/UNLOCK TABLES" with Bugzilla::DB function call

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Tomas Kopal]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20050110 Firefox/1.0 (Debian package 1.0+dfsg.1-2)
Build Identifier: 

"LOCK TABLES" and "UNLOCK TABLES" are MySQL specific and should be replaced with
DB agnostic call to function in the DB compatibility layer.



Reproducible: Always

Steps to Reproduce:

Comment 1

[Tomas Kopal]

Attachment: V1

First version of the patch converting locking to use the DB compatibility
layer. checksetup.pl has not been updated, as it's currently not using
Bugzilla->dbh interface.

Comment 2

[Max Kanat-Alexander]

Comment on attachment 173723 [details] [diff] [review]
V1

>diff -Nrux Entries ../bugzilla-dbcompat/Bugzilla/Error.pm ./Bugzilla/Error.pm
>--- ../bugzilla-dbcompat/Bugzilla/Error.pm	2005-02-04 17:47:37.000000000 +1030
>+++ ./Bugzilla/Error.pm	2005-02-06 18:30:31.000000000 +1030
>@@ -37,7 +37,7 @@
> 
>     $vars->{error} = $error;
> 
>-    Bugzilla->dbh->do("UNLOCK TABLES") if $unlock_tables;
>+    Bugzilla->dbh->bz_unlock_tables(1) if $unlock_tables;

  OK. Instead of a "magic number," there should be a constant called
UNLOCK_ABORT in Bugzilla::Constants, and it should be 1. (This makes the code
clearer.)

>-    &::SendSQL("LOCK TABLES tokens WRITE");
>+    Bugzilla->dbh->bz_lock_tables('tokens WRITE');

  Instead of doing Bugzilla->dbh everywhere, just add a "my $dbh =
Bugzilla->dbh" at the beginning of functions that require it. We'll (read:
I'll) be doing that eventually, anyway. We might as well do it now.

>diff -Nrux Entries ../bugzilla-dbcompat/editproducts.cgi ./editproducts.cgi
>--- ../bugzilla-dbcompat/editproducts.cgi	2005-02-04 17:54:48.000000000 +1030
>+++ ./editproducts.cgi	2005-02-08 21:00:28.000000000 +1030
>@@ -209,7 +209,6 @@
> sub PutTrailer (@)
> {
>     my (@links) = ("Back to the <A HREF=\"query.cgi\">query page</A>", @_);
>-    SendSQL("UNLOCK TABLES");

  Can you briefly explain why you moved the UNLOCK TABLES code around? (And
perhaps that should be another patch than this one?)

  Everything else looks pretty good. You can modify checksetup based on the
patch on bug 281360. It might require me to modify my other
checksetup-DB-independence-function-moving patch, but I'm OK with that, too.

Comment 3

[Tomas Kopal]

Attachment: V2

(In reply to comment #2)
> >-	Bugzilla->dbh->do("UNLOCK TABLES") if $unlock_tables;
> >+	Bugzilla->dbh->bz_unlock_tables(1) if $unlock_tables;
> 
>   OK. Instead of a "magic number," there should be a constant called
> UNLOCK_ABORT in Bugzilla::Constants, and it should be 1. (This makes the code

> clearer.)

Very good point, thanks. Fixed.

> 
> >-	&::SendSQL("LOCK TABLES tokens WRITE");
> >+	Bugzilla->dbh->bz_lock_tables('tokens WRITE');
> 
>   Instead of doing Bugzilla->dbh everywhere, just add a "my $dbh =
> Bugzilla->dbh" at the beginning of functions that require it. We'll (read:
> I'll) be doing that eventually, anyway. We might as well do it now.

Yes, I was thinking about that, but the patch is pretty long as it is and this
would make it even harder to read. I would prefer to do it as part of the
SendSQL patch (which will be multi-part anyway). So I didn't change it for now,
but if you insist, I will change my mind :-).

> 
> >diff -Nrux Entries ../bugzilla-dbcompat/editproducts.cgi ./editproducts.cgi
> >--- ../bugzilla-dbcompat/editproducts.cgi	2005-02-04 17:54:48.000000000
+1030
> >+++ ./editproducts.cgi	2005-02-08 21:00:28.000000000 +1030
> >@@ -209,7 +209,6 @@
> > sub PutTrailer (@)
> > {
> >	my (@links) = ("Back to the <A HREF=\"query.cgi\">query page</A>", @_);

> >-	SendSQL("UNLOCK TABLES");
> 
>   Can you briefly explain why you moved the UNLOCK TABLES code around? (And
> perhaps that should be another patch than this one?)

I moved it around to prevent the code from calling unlock_table without locking
it first. It didn't cause trouble before, as MySQL probably ignore this, but
because I put checks for this in Bugzilla::DB, it would beat us now.
Yes, we can do it in different bug, but it would have to block this patch. But
I think the change is simple enough to be incuded here. I just took the UNLOCK
statement from inside of the PutTrailer function and pasted it just before
every call to it, and then removed all occurences which were not preceeded by
matching LOCK. It actually makes the code even a bit cleaner :-).

> 
>   Everything else looks pretty good. You can modify checksetup based on the
> patch on bug 281360. It might require me to modify my other
> checksetup-DB-independence-function-moving patch, but I'm OK with that, too.
> 

Done, changes to checksetup.pl included in this patch (requires patch from bug
281360 to work).

Comment 4

[Max Kanat-Alexander]

Comment on attachment 173734 [details] [diff] [review]
V2

  OK. I won't require the "my $dbh" things, then.

  And the UNLOCK TABLES move also sounds fine to me.

>diff -Nrux Entries ../bugzilla-dbcompat/Bugzilla/Constants.pm ./Bugzilla/Constants.pm
>--- ../bugzilla-dbcompat/Bugzilla/Constants.pm	2005-02-04 17:47:37.000000000 +1030
>+++ ./Bugzilla/Constants.pm	2005-02-08 23:36:34.000000000 +1030
>@@ -64,6 +64,8 @@
> 
>     DEFAULT_COLUMN_LIST
>     DEFAULT_QUERY_NAME
>+
>+    UNLOCK_ABORT
> );

  Cool. Do you think that we should also modify Bugzilla::DB::bz_unlock_tables
to check for that specific value? (In case we ever want to pass it a different
value, to have different behavior.)

  I just have to do some testing on this, and then it's review+. Right now it
looks fine, by inspection.

Comment 5

[Tomas Kopal]

(In reply to comment #4)
>   Cool. Do you think that we should also modify Bugzilla::DB::bz_unlock_tables
> to check for that specific value? (In case we ever want to pass it a different
> value, to have different behavior.)

Dunno. I can't imagine any other value which could be passed there, but it
probably can happen. But we can always change it later, right?
Which reminds me - I will add a documentation of this constant to the
bz_unlock_table function description (expected param value).

>   I just have to do some testing on this, and then it's review+. Right now it
> looks fine, by inspection.
> 

Excellent, thanks. These were probably the fastest reviews I have ever seen on
bugzilla ;-).

Comment 6

[Tomas Kopal]

Attachment: V2.1

Added UNLOCK_ABORT constant to bz_unlock_tables method parameter description,
otherwise identical with V2.

Comment 7

[Max Kanat-Alexander]

Comment on attachment 173736 [details] [diff] [review]
V2.1

>- Params:      $abort = true (1) if the operation on locked tables failed
>-              (if transactions are supported, the action will be rolled
>+ Params:      $abort = UNLOCK_ABORT (true, 1) if the operation on locked tables
>+              failed (if transactions are supported, the action will be rolled

  Nit: Callers should not be told the value of a constant. :-) They don't need
to know. They just need to know it's UNLOCK_ABORT. :-)

  Other than that, basic testing on landfill shows review+. :-)

Comment 8

[Max Kanat-Alexander]

justdave (or myk): Note that the dependencies must be resolved before approval
is finally granted.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

I understand from Max that these changes are relatively low risk for MySQL users
and have gone through multiple levels of revision and review, but I'm concerned
about their impact, so please be around in the days after checking this in to
watch out for and respond to reports of regressions caused by the checkins.

Comment 10

[Max Kanat-Alexander]

Comment on attachment 173736 [details] [diff] [review]
V2.1

bitrot.

Also, a few more LOCK statements showed up:

editgroups.cgi:    $dbh->do("LOCK TABLES groups WRITE, group_group_map WRITE,
editgroups.cgi:    $dbh->do("UNLOCK TABLES");

Comment 11

[Tomas Kopal]

Attachment: V2.2

Unbitrotted, and also Bugzilla->dbh-> replaced by $dbh-> for simplicity.

Comment 12

[Max Kanat-Alexander]

Comment on attachment 174566 [details] [diff] [review]
V2.2

This applies cleanly, checksetup runs, and the testsuite passes.

r=mkanat by careful inspection of the location of all "my $dbh = Bugzilla->dbh"
statements.

Comment 13

[Max Kanat-Alexander]

OK, I got verbal approval from justdave in IRC to check-in this version.

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.69; previous revision: 1.68
done
Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.279; previous revision: 1.278
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.343; previous revision: 1.342
done
Checking in editclassifications.cgi;
/cvsroot/mozilla/webtools/bugzilla/editclassifications.cgi,v  <-- 
editclassifications.cgi
new revision: 1.3; previous revision: 1.2
done
Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <--  editcomponents.cgi
new revision: 1.47; previous revision: 1.46
done
Checking in editflagtypes.cgi;
/cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v  <--  editflagtypes.cgi
new revision: 1.14; previous revision: 1.13
done
Checking in editgroups.cgi;
/cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v  <--  editgroups.cgi
new revision: 1.46; previous revision: 1.45
done
Checking in editmilestones.cgi;
/cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v  <--  editmilestones.cgi
new revision: 1.31; previous revision: 1.30
done
Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.69; previous revision: 1.68
done
Checking in editversions.cgi;
/cvsroot/mozilla/webtools/bugzilla/editversions.cgi,v  <--  editversions.cgi
new revision: 1.30; previous revision: 1.29
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.235; previous revision: 1.234
done
Checking in query.cgi;
/cvsroot/mozilla/webtools/bugzilla/query.cgi,v  <--  query.cgi
new revision: 1.140; previous revision: 1.139
done
Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.78; previous revision: 1.77
done
Checking in token.cgi;
/cvsroot/mozilla/webtools/bugzilla/token.cgi,v  <--  token.cgi
new revision: 1.30; previous revision: 1.29
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.65; previous revision: 1.64
done
Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
new revision: 1.27; previous revision: 1.26
done
Checking in whine.pl;
/cvsroot/mozilla/webtools/bugzilla/whine.pl,v  <--  whine.pl
new revision: 1.5; previous revision: 1.4
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.17; previous revision: 1.16
done
Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.20; previous revision: 1.19
done
Checking in Bugzilla/Error.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Error.pm,v  <--  Error.pm
new revision: 1.10; previous revision: 1.9
done
Checking in Bugzilla/Series.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Series.pm,v  <--  Series.pm
new revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/Token.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Token.pm,v  <--  Token.pm
new revision: 1.25; previous revision: 1.24
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.36; previous revision: 1.35
done