Closed
Bug 313695
Opened 19 years ago
Closed 19 years ago
Buglist does not use shadowdb
Categories
(Bugzilla :: Query/Bug List, defect, P1)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.20
People
(Reporter: bugreport, Assigned: LpSolit)
References
Details
Attachments
(2 files, 2 obsolete files)
6.72 KB,
patch
|
justdave
:
review+
|
Details | Diff | Splinter Review |
3.85 KB,
patch
|
justdave
:
review+
|
Details | Diff | Splinter Review |
at the beginning, $dbh is set to Bugzilla->dbh later, switch_to_shadow() changes what will be returned by Bugzilla->dbh then, it does $dbh->prepare which gets the value it had before it switched to shadow. buglist needs to get a handle for main and one for shadow and use the appropriate ones for the appropriate purposes.
Reporter | ||
Updated•19 years ago
|
Flags: blocking2.20.1?
Priority: -- → P1
Reporter | ||
Comment 1•19 years ago
|
||
To keep thigns simple and safe, this switches to shadow just long enough to get its own $read_dbh and the switches back. This way, we don't have to worry about some library function that does more.
Attachment #200704 -
Flags: review?
Comment 2•19 years ago
|
||
Brendan: you were right, Bugzilla is slower, and this is why. On the trunk, switch_to_shadow() can go away. The only reason we needed it was for the deprecated DB routines such as SendSQL, since it didn't take a database handle as a parameter. Since those deprecated routines are now gone (they are, right?) switch_to_shadow() should just go away, and let the client script decide which database handle it's using on its own. I propose we add a Bugzilla->shadowdbh property to complement Bugzilla->dbh, and let the script just grab whichever handle they need. On a system that doesn't have a shadowdb set up, Bugzilla->shadowdbh would just return Bugzilla->dbh. For 2.20 we're going to need a kludge, since there's still a lot of places using the deprecated routines...
Assignee: query-and-buglist → justdave
Flags: blocking2.20.1? → blocking2.20.1+
Comment 3•19 years ago
|
||
Comment on attachment 200704 [details] [diff] [review] use shadow for reads And this almost looks like a good kludge... Seems like there are other places in Bugzilla that use the shadow database though. We should do a quick audit and make sure none of the other places fell victim to this. Also, to emulate the prior behavior, we should probably not fuss with the $dbh handles used in the code, but instead just make sure we grab a new $dbh any time a switch_to_foo() is called
Reporter | ||
Comment 4•19 years ago
|
||
And whining does the same phony thing :-)
Comment 5•19 years ago
|
||
But where is $dbh coming from? Its not valid to cache $dbh arround calls to the shadow DB. Not is it valid to mix SendSQL and DBI - hence this bug.
Comment 6•19 years ago
|
||
bbaetz: $dbh is coming from Bugzilla->dbh, always, in files where we have an $dbh. So it's a reference to a DBI handle, which should be fine to keep around as long as you're using the same handle. I agree, we should have a Bugzilla->shadow_dbh to get the shadowDB handle.
Assignee: justdave → bugreport
Assignee | ||
Comment 7•19 years ago
|
||
You cannot simply write my $shadow_dbh = Bugzilla->shadow_dbh; and then work with it, and thinking that all subsequent DBI calls will use it. Indeed, each routine redefines its own $dbh variable by using my $dbh = Bugzilla->dbh; as justdave pointed out on IRC, and calling this routine will not make it use "your" $dbh_shadow defined earlier. Depending on the caller, the routine should sometimes use the main DB, sometimes the shadow DB. But passing $dbh as a param is a mess as it would appear for almost all routines we have in the Bugzilla code. Easier is to update $_dbh in Bugzilla.pm itself so that all subsequent calls to Bugzilla->dbh will get the correct handle. I updated Bugzilla->switch_to_foo_db to return the handle so that you can directly write: my $dbh = Bugzilla->switch_to_foo_db; see for instance in collectstats.pl. This way, deprecated DB routines and new DBI methods should be compatible. Note that my patch fixes all concerned files.
Attachment #200944 -
Flags: review?(justdave)
Reporter | ||
Comment 8•19 years ago
|
||
I think we're going a bit nuts with this. In each of these CGIs, there is single query that really needs to be run against the shadow. Rather than trying to change every last operation, why not just run the big slow query against the shadow?
Comment 9•19 years ago
|
||
Technically, we should really be running everything against the shadow, except for authentication and any reads that are loading data which is about to be modified. The master server in MySQL typically can't be scaled any further than the hardware it's running on. The slaves, however, can be split across lots of servers and load balanced. So the more traffic we throw at the shadowdb the better. But for 2.20 purposes, yeah, that's probably overkill. For 2.20 I rather like the type of thing Fred has posted in his last patch, because it preserves the intent of the original code, and we want to make as few changes to the assumptions about how things operate as possible on the stable branch. Fred's patch makes it operate the same way it did in 2.18.x, and the way we intended for it to work in 2.20 before we discovered it was broken.
Assignee | ||
Comment 10•19 years ago
|
||
my previous patch had a bug when the shadow DB was called before the main DB. $_dbh was never updated. This patch fixes it.
Assignee: bugreport → LpSolit
Attachment #200704 -
Attachment is obsolete: true
Attachment #200944 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #200971 -
Flags: review?(justdave)
Attachment #200704 -
Flags: review?
Attachment #200944 -
Flags: review?(justdave)
Assignee | ||
Comment 11•19 years ago
|
||
backport for 2.20. There has been almost no cleanup in 2.20 yet, so much less files are affected. It should applies cleanly on b.m.o too.
Attachment #200972 -
Flags: review?(justdave)
Assignee | ||
Comment 12•19 years ago
|
||
Comment on attachment 200971 [details] [diff] [review] patch for tip, v2 maybe joel will be faster...
Attachment #200971 -
Flags: review?(bugreport)
Assignee | ||
Updated•19 years ago
|
Attachment #200972 -
Flags: review?(bugreport)
Comment 13•19 years ago
|
||
Comment on attachment 200972 [details] [diff] [review] patch for 2.20, v1 seems to work. This is now live on b.m.o after excercising it for a bit on my test install on recluse.
Attachment #200972 -
Flags: review?(justdave) → review+
Updated•19 years ago
|
Whiteboard: [attachment 200972 applied to b.m.o]
Comment 14•19 years ago
|
||
Comment on attachment 200971 [details] [diff] [review] patch for tip, v2 r+ based on inspection and similarity to the known-working 2.20 patch
Attachment #200971 -
Flags: review?(justdave) → review+
Assignee | ||
Updated•19 years ago
|
Attachment #200971 -
Flags: review?(bugreport)
Assignee | ||
Updated•19 years ago
|
Attachment #200972 -
Flags: review?(bugreport)
Assignee | ||
Updated•19 years ago
|
Flags: approval?
Flags: approval2.20?
Comment 15•19 years ago
|
||
ok, nobody's complained on bmo yet, and it seems to be working quite well there. Let's go ahead and check this in.
Flags: approval?
Flags: approval2.20?
Flags: approval2.20+
Flags: approval+
Assignee | ||
Comment 16•19 years ago
|
||
tip: Checking in Bugzilla.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v <-- Bugzilla.pm new revision: 1.24; previous revision: 1.23 done Checking in buglist.cgi; /cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v <-- buglist.cgi new revision: 1.319; previous revision: 1.318 done Checking in collectstats.pl; /cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v <-- collectstats.pl new revision: 1.45; previous revision: 1.44 done Checking in duplicates.cgi; /cvsroot/mozilla/webtools/bugzilla/duplicates.cgi,v <-- duplicates.cgi new revision: 1.51; previous revision: 1.50 done Checking in report.cgi; /cvsroot/mozilla/webtools/bugzilla/report.cgi,v <-- report.cgi new revision: 1.36; previous revision: 1.35 done Checking in showdependencygraph.cgi; /cvsroot/mozilla/webtools/bugzilla/showdependencygraph.cgi,v <-- showdependencygraph.cgi new revision: 1.47; previous revision: 1.46 done Checking in showdependencytree.cgi; /cvsroot/mozilla/webtools/bugzilla/showdependencytree.cgi,v <-- showdependencytree.cgi new revision: 1.38; previous revision: 1.37 done Checking in whine.pl; /cvsroot/mozilla/webtools/bugzilla/whine.pl,v <-- whine.pl new revision: 1.19; previous revision: 1.18 done 2.20: Checking in Bugzilla.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v <-- Bugzilla.pm new revision: 1.17.4.1; previous revision: 1.17 done Checking in buglist.cgi; /cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v <-- buglist.cgi new revision: 1.299.2.5; previous revision: 1.299.2.4 done Checking in collectstats.pl; /cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v <-- collectstats.pl new revision: 1.43.4.2; previous revision: 1.43.4.1 done Checking in whine.pl; /cvsroot/mozilla/webtools/bugzilla/whine.pl,v <-- whine.pl new revision: 1.13.2.3; previous revision: 1.13.2.2 done Checking in Bugzilla/User.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v <-- User.pm new revision: 1.61.2.12; previous revision: 1.61.2.11 done
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
Keywords: relnote
Whiteboard: [attachment 200972 applied to b.m.o] → [attachment 200972 applied to b.m.o] [relnote 2.20.1]
Comment 17•18 years ago
|
||
Added to the Bugzilla 2.20.1 Release Notes in bug 320319.
Keywords: relnote
Whiteboard: [attachment 200972 applied to b.m.o] [relnote 2.20.1] → [attachment 200972 applied to b.m.o]
Updated•18 years ago
|
Whiteboard: [attachment 200972 applied to b.m.o]
You need to log in
before you can comment on or make changes to this bug.
Description
•