132808 - describekeywords.cgi blocks queries against the bugs database

Status: RESOLVED WORKSFORME

(Bugzilla :: Bugzilla-General, defect, P3)

Description

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

When describekeywords.cgi runs it blocks queries against the bugs database,
causing other script invocations (show_bug.cgi, process_bug.cgi, etc.) to hang
waiting for describekeywords.cgi to finish.  For a large site like b.m.o., where
describekeywords.cgi can take 30-60 seconds to run, this seriously degrades
Bugzilla performance for the duration of the run.  describekeywords.cgi should
not block other queries against the bugs database.

Comment 1

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

Attachment: patch v1: uses the shadow database on b.m.o.

I changed describekeywords.cgi on b.m.o. to use the shadow database, since it
appears that queries against the shadow database are not blocked by
describekeywords.cgi.  I can't be sure of this, because I don't have a server
with enough load on it to test, but b.m.o. will tell us soon enough if this
solution doesn't work.	In the meantime, here's the patch that does it.

Comment 2

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

Attachment: patch v2: against the tip

This patch works against the tip.

Comment 3

[Bradley Baetz (:bbaetz)]

Comment on attachment 75587 [details] [diff] [review]
patch v2: against the tip

>diff -u -r1.7 describekeywords.cgi
>--- describekeywords.cgi	19 Feb 2002 23:32:04 -0000	1.7
>+++ describekeywords.cgi	22 Mar 2002 16:13:17 -0000
>@@ -32,7 +32,8 @@
> 
> ConnectToDatabase();
> 
>-quietly_check_login();
>+# Connect to the shadow database if this installation is using one.
>+ReconnectToShadowDatabase();
> 

Whats wrong with ConnectToDatabase(1) ?

Comment 4

[Dave Miller [:justdave]]

-> patch author.    this still has a needs-work on it, fyi.

Comment 5

[Gervase Markham [:gerv]]

Attachment: Patch v.3

I think this is the right fix.

Gerv

Comment 6

[Bradley Baetz (:bbaetz)]

Comment on attachment 80451 [details] [diff] [review]
Patch v.3

r=bbaetz

Comment 7

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

Comment on attachment 80451 [details] [diff] [review]
Patch v.3

Unfortunately using the shadow database means locking that database up, which
doesn't gain us anything, so I have to needs-work this patch.  The real
solution long-term is to generate a static version of these counts
periodically.

Comment 8

[Bradley Baetz (:bbaetz)]

Hang on. Yes, this query may take a long time. But so may any query. At least
this way you'd only block buglist.cgi, not show/enter/etc.

Even then, are you blocking? We don't lock the keywords table in that page, do
we? Its ok for numbers to be approximate. We should lock keyworddefs, for READ
access, but that hardly every changes.

Comment 9

[Matthew Tuck [:CodeMachine]]

I can't see how we are blocking anything significant.  The shadow database is
designed to be read only, so you can have as many readers as you want, and as
many read locks as you want, on any tables.

The only thing that does get blocked by read locks is the synchronisations
(which are the only writes), but that's the point of the shadow database
tradeoff.

Comment 10

[Bradley Baetz (:bbaetz)]

The select will effectivly result in built-in locking for that particular
statement, because of how mysql works. If we use the shadowdb, then we only
block queries which care about keywords, rather than show_bug and friends which
will use keywords.

myk: What do you get for: |SELECT COUNT(*) FROM keywords;|?

whats the explain output on the describekeywords.cgi sql?

We have to search through every entry on the keywords table - there's no short
cut for this.

Comment 11

[Bradley Baetz (:bbaetz)]

Oh, and just out of interest - whats the time taken for the original query, and
the original query using INNER JOIN instead of LEFT JOIN? It should be roughly
the same. 

Comment 12

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

mysql> SELECT COUNT(*) FROM keywords;
+----------+
| COUNT(*) |
+----------+
|    61404 |
+----------+
1 row in set (0.00 sec)

mysql> EXPLAIN SELECT keyworddefs.name, keyworddefs.description,
COUNT(keywords.bug_id), keywords.bug_id FROM keyworddefs LEFT JOIN keywords ON
keyworddefs.id=keywords.keywordid GROUP BY keyworddefs.id ORDER BY keyworddefs.name;
+-------------+------+---------------+-----------+---------+----------------+------+---------------------------------+
| table       | type | possible_keys | key       | key_len | ref            |
rows | Extra                           |
+-------------+------+---------------+-----------+---------+----------------+------+---------------------------------+
| keyworddefs | ALL  | NULL          | NULL      |    NULL | NULL           | 
130 | Using temporary; Using filesort |
| keywords    | ref  | keywordid     | keywordid |       2 | keyworddefs.id | 
810 |                                 |
+-------------+------+---------------+-----------+---------+----------------+------+---------------------------------+
2 rows in set (0.00 sec)

Comment 13

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

INNER JOIN has no effect on the query.

Comment 14

[Gervase Markham [:gerv]]

So, it seems we have two options for b.m.o for 2.16 - use the shadowdb, or
disable the CGI. Which is it to be?

Gerv

Comment 15

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

I don't think anything needs to be done about this bug for 2.16.  Bug counts are
already disabled on b.m.o, but the permanent solution is likely to be either the
shadow database or (more likely) periodic generation of a static page.  In the
meantime I can live with the local customization.

Pushing out to 2.18.

Comment 16

[Gervase Markham [:gerv]]

Let's update the tip to use the shadowDB. I think that's reasonable, because of
what bbaetz said about the way MySQL works.

Gerv

Comment 17

[Joel Peshkin]

Unloved bugs targetted for 2.18 but untouched since 9-15-2003 are being
retargeted to 2.20
If you plan to act on one immediately, go ahead and pull it back to 2.18.

Comment 18

[Max Kanat-Alexander]

We don't use the shadowdb anymore. Anybody have any other good ideas for patches
for this?

Is it really still a problem?

Comment 19

[Max Kanat-Alexander]

Re-assigning to last patch author.

Comment 20

[Marc Schumann [:Wurblzap]]

Sounds like a WORKSFORME to me...