413772 - Eliminate sqlify_criteria in Bugzilla::Flag and replace match() there with Bugzilla::Object::match

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement, P3)

Description

[Max Kanat-Alexander]

Right now bmo has this query about 34 times per second:

SELECT id FROM flags WHERE 1 = 1 AND attach_id IS NULL AND type_id = ? AND bug_id = ?

The first step to fixing that is to remove Bugzilla::Flag::match and replace it with the standard Bugzilla::Object::match, and remove sqlify_criteria in the process.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Okay, here we go. This was pretty straightforward. Combined with the changes I made to match() in bug 372795, this should be a nice DB performance win, or at least allow us to better diagnose where problems are coming from. Also, it helps clean up the architecture!

Comment 2

[Frédéric Buclin]

Comment on attachment 298837 [details] [diff] [review]
v1

>Index: Bugzilla/Flag.pm

>+    return $class->SUPER::match(@_);

This doesn't work. Changing a bug flag throws:

Can't call method "SUPER::match" on unblessed reference at Bugzilla/Flag.pm line 219.



>@@ -1128,51 +1126,6 @@
> # Private Functions
> ######################################################################

Please remove this now useless title.

Comment 3

[Frédéric Buclin]

I'm pretty sure the reason of the failure is that Bugzilla::Flag::snapshot() calls match() directly. So you need to write something like:

    my $invocant = shift;
    my $class = ref($invocant) || $invocant;

in match(). Maybe in count() too, btw, to be safe.

Comment 4

[Max Kanat-Alexander]

Attachment: v2

Okay, there was a limit to how far I was willing to go in converting "::" methods to "->" methods, so for some things here I just used __PACKAGE__ (which makes it hard to subclass Bugzilla::Flag, but you really couldn't do that before anyway).

Comment 5

[Frédéric Buclin]

Comment on attachment 299878 [details] [diff] [review]
v2

>+        my $has_flags = __PACKAGE__->count(

No, that's not how I want to see it implemented, especially for process() and snapshot(). All you have to do is to replace direct calls to count() in Flag.pm by Bugzilla::Flag->count() AFAIK.

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> No, that's not how I want to see it implemented, especially for process() and
> snapshot(). All you have to do is to replace direct calls to count() in Flag.pm
> by Bugzilla::Flag->count() AFAIK.

  That's what I *did*. That's what __PACKAGE__->count means. It just doesn't have to be updated if the name of the package changes for some reason--it's the standard Perl way of doing this.

Comment 7

[Frédéric Buclin]

Comment on attachment 299878 [details] [diff] [review]
v2

Looks good. r=LpSolit

I'm going to commit it myself as I have another patch conflicting with it.

Comment 8

[Frédéric Buclin]

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.141; previous revision: 1.140
done
Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.146; previous revision: 1.145
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.193; previous revision: 1.192
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.402; previous revision: 1.401
done
Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.55; previous revision: 1.54
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.231; previous revision: 1.230
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.90; previous revision: 1.89
done