297098 - Optimize multiple email searches

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement, P2)

Description

[Joel Peshkin]

Bug 127200 added an optimization to look up profiles first and then do the
queries for longdescs.  This needs to be extended to other types of email joins.

Comment 1

[Joel Peshkin]

Attachment: Patch - query profiles first

Comment 2

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

Comment on attachment 185924 [details] [diff] [review]
Patch - query profiles first

Mmm, yummy.

Nit: you should update the comment above ListIDsForEmail to reflect that it no
longer "only supports anyexact and substring matches."


>+                 $term = "bugs.$f IN($list)"; 

Nit: put a space between IN and its succeeding opening parenthesis per usage
elsewhere in Bugzilla (and because IN is an operator, not a function).


>+    } elsif ($type eq 'regexp') {
>+        &::SendSQL("SELECT userid FROM profiles WHERE " .
>+            "login_name " . $dbh->sql_regexp() . ::SqlQuote($email) .
>+            " " . $dbh->sql_limit(51));
>+        while (&::MoreSQLData()) {
>+            my ($id) = &::FetchSQLData();
>+            push(@list, $id);
>+        }
>+        if (@list < 50) {
>+            $list = join(',', @list);
>+        }
>     }

Nit: this conditional would be clearer as: "if (scalar(@list) <= 50) {...}".

Nit: you would also benefit from converting this to use the new database
handling mechanisms, since then you could do something like:

$list = $dbh->selectcol_arrayref(...);
$list = @$list < 50 ? join(',', @$list) : undef;

Nit: you could also prepare a statement for each type of query, run the
appropriate statement, and then factor out the code for conditionally
populating the list if it doesn't contain too many items.

Speaking of too many items, is it really the case that running a query with >
50 items in an IN clause is slower than doing the join?

Comment 3

[Joel Peshkin]

I'll fix the first 3 nits on checkin.

Removing SendSQL from Search.pm is something I'd rather do as a distinct bug
covering the whole file at once.

It is actually rare that a query needs more than one of these statements, so I
am better off not pre-preparing them.

The limit of 50 is so that the list does not get long enough to make the SQL
statement fail.  I have seen problems with extremely long lists before (> 2K
characters).  If a user  really wants to search for all of the bmo users with
regexp "@.*\.com$" we would rather not generate a monster IN clause.  Perhaps I
could get away with 100 or even 200, but there is little added benefit in that.

Comment 4

[Frédéric Buclin]

Note that this patch does not apply cleanly:

patching file Bugzilla/Search.pm
Hunk #1 FAILED at 25.
1 out of 4 hunks FAILED -- saving rejects to file Bugzilla/Search.pm.rej

The reason is that mkanat has changed his email address in the contributor list. ;)

joel, maybe is it a good idea to submit an updated patch with the nits fixed?

Comment 5

[Joel Peshkin]

Attachment: updated patch

Comment 6

[Frédéric Buclin]

Comment on attachment 188792 [details] [diff] [review]
updated patch

I confirm that the first 3 nits have been fixed. The review itself has been
done by myk. r=LpSolit

Comment 7

[Frédéric Buclin]

joel said to do the checkin ourselves if he was AFK when the patch got a+.

Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.101; previous revision: 1.100
done