Selecting all the entries in an listbox on the query page is equivalent to selecting none of them, but about twice as slow. buglist.cgi should spot when all entries have been selected, and turn it into the "none" case. Gerv

This does the job for status and resolution, which I suspect are the common cases. Gerv

You could have query.cgi pass a num_status and num_resolution to buglist.cgi giving the max number of entries to get rid of the need to hardcode 7 and 9 into buglist.cgi. I know some, me for example, who will alter the number of entries displayed in the list boxes in the templates and would need to change the numbers. I also know that this will be solved when customizable resolutions is implemented but passing the number of elements should solve it in the meantime.

Hmm. That's rather a hacky solution, really (like mine, in fact :-). Bugzilla should be able to work this out for itself without needing that. Myk, Dave - any ideas? Would unconditionally doing GetVersionTable() be a bigger slowdown than this is a speed up? Is calling GetVersionTable() expensive? Can we do a cheap query to find these numbers? Gerv

GetVersionTable is currently fairly cheap. It will need to vanish, eventually, though. In this case, though, custres will break this hack. We would, of course, like the db to do this for us :)

So is doing GetVersionTable unconditionally worth it? Having been the happy recipient of the mysqld-watcher "I killed this query because it took too long" emails for the past 24 hours, a significant proportion (half?) have all the statuses selected. So I think it is worth it. New patch coming up. Gerv

Try this. Gerv

An analysis of 17 killed queries showed this problem in 5 of them. Gerv

Comment on attachment 102514 [details] [diff] [review] Patch v.2 Doing the tests with |scalar| is nicer. You need to test that the contents are the same, since we never validate that the status/resolutions selected are actually valid. Alos, the GetVersionTable call is fragile - I know that report.cgi does this unconditionally as well, but maybe Bugzilla::Search should, instead?

bbaetz: > Doing the tests with |scalar| is nicer. OK. > You need to test that the contents are the same, since we never validate that > the status/resolutions selected are actually valid. If people are hacking the search form, they deserve everything they get, including incorrect results. > Alos, the GetVersionTable call is fragile - I know that report.cgi does this > unconditionally as well, but maybe Bugzilla::Search should, instead? "Fragile"? Maybe Bugzilla::Search should be calling GetVersionTable(), but this works, is a minimal change, and I want to get it in before b.m.o. upgrades. We can open a new bug to think about exactly where we should be calling it if you like. Gerv

No, I mean that relying on the caller having loaded the versiontable is fragile. If Bugzilla::Search needs to ensure that the version table has been loaded, then it should do so itsself.

Nits picked; Search.pm now calls GetVersionTable() for itself. Looking for r=bbaetz; are you going to make me file a review request? :-) gerv

No, it means you find somsone who doesn't have a thesis due in two weeks :)

Wouldn't this work better on the client side as a JS script that unselected the items on form submission?

myk: what advantages would that have? It would mean it wouldn't work for people with no JS, and would increase the size of the page (and the query page is already pretty big.) Also, can you get the number of items in a select list from JS? (You probably can; I'm just asking.) Gerv

Yeah, I don't think we want this done via js. Speaking of JS, though, what does the ALL for quicksearch do? Anyway, I just thought of the reason why mysql can't do this - because it accepts 'invalid' enum values, which are hidden by what we currently do, but wouldn't be with the removal of the clause. I don't think we care about that distinction, do we?

No - if there are invalid enum values, too bad. :-) If people add new states in a way that makes them show up in GetVersionTable, then this code will work. And that's perfectly good enough. :-) Gerv

The main advantage to using JS is that we don't have to depend on the version cache, so when it goes away we don't have to change any code. Minor advantages include cleaner search URLs and probably a small performance improvement for buglist.cgi. The fact that some people don't have or use JS is a non-issue. Almost everybody does have and use it, and the search form will still work for those who don't.

> The main advantage to using JS is that we don't have to depend on the version > cache, so when it goes away we don't have to change any code. If there's a backwards-compatible interface, this won't be a problem - and if there isn't, we have a load of code to change, and a couple more lines won't make a difference. > Minor advantages include cleaner search URLs and probably a small performance > improvement for buglist.cgi. ...which would be cancelled out by even one person doing a stupid search that the JS didn't prevent because they had it switched off. Now I've changed the search defaults, this is a very common feature of the remaining mysqld-watcher mails. Gerv

Comment on attachment 103907 [details] [diff] [review] Patch v.3 r=justdave

Fixed, after conversion to CGI.pm syntax. Checking in Bugzilla/Search.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v <-- Search.pm new revision: 1.25; previous revision: 1.24 done Gerv

fixing target