124587 - lock the bugs table for writing when inserting a new bug

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P1)

Description

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

bbbaetz makes a good point in bug 87006, comment 14.  post_bug.cgi does not lock
the bugs table before inserting a bug.  For installations that use the shadow
database, this could result in misordered (or perhaps even missing) shadow
database records, because SendSQL first inserts the record into the bugs table
and then inserts a record into the shadowlog table that will later create the
bug in the shadow database.  If two bugs are created at the same time, the four
inserts could happen in the wrong order, switching bug numbers or overwriting
one bug with the other (and potentially making everything off by one) in the
shadow database.

Comment 1

[Bradley Baetz (:bbaetz)]

As I discussed on IRC, I really hate the shadowdb ;)

A patch for this should only lock if we're using the shadowdb. (I'm not sure if
select or update takes higher priority. a write lock could swap the order round,
leading to other perf problems, although the frequency of INSERT is probably not
enough to be concerned)

Also, we need to be careful. When adding ccs, for example, do you lock the cc
table?. bugzilla's locking is a mess ATM, and cleaning it up is one of my 2.18
goals.

Comment 2

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

Attachment: patch v1: locks the tables

Comment 3

[Bradley Baetz (:bbaetz)]

Comment on attachment 68718 [details] [diff] [review]
patch v1: locks the tables

OK. r=bbaetz. Not that I can test if this reduces the corruption risk, mind
you.

Comment 4

[Dave Miller [:justdave]]

See my comments on bug 87006...  I think syncshadowdb needs to obtain a lock
before reading stuff from the shadowlog table, too.

Comment 5

[Bradley Baetz (:bbaetz)]

dave: possibly, but see my comments in bug 124589. ;)

Are we concerned about that in this late stage? Anyone who already uses the
shadwowdb stuff knows how unreliable it is, and we're already planning to scrap
it anyway.

Comment 6

[Matthew Tuck [:CodeMachine]]

Since this is marked critical, moving to 2.16.

Comment 7

[Dave Miller [:justdave]]

Attachment: Myk's patch - no changes except applies to tip

Comment 8

[Dave Miller [:justdave]]

Comment on attachment 80206 [details] [diff] [review]
Myk's patch - no changes except applies to tip

inheriting r=bbaetz from previous patch.
adding r=justdave

Comment 9

[Gervase Markham [:gerv]]

Someone appears to have checked this in.

Gerv