Closed Bug 313695 Opened 19 years ago Closed 19 years ago

Buglist does not use shadowdb

Categories

(Bugzilla :: Query/Bug List, defect, P1)

2.20

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: bugreport, Assigned: LpSolit)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Flags: blocking2.20.1?
Priority: -- → P1
Attached patch use shadow for reads (obsolete) — Splinter Review
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?
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 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
And whining does the same phony thing :-)
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.
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
Attached patch LpSolit's patch, v1 (obsolete) — Splinter Review
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)
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?
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.
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)
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)
Comment on attachment 200971 [details] [diff] [review]
patch for tip, v2

maybe joel will be faster...
Attachment #200971 - Flags: review?(bugreport)
Attachment #200972 - Flags: review?(bugreport)
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+
Whiteboard: [attachment 200972 applied to b.m.o]
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+
Attachment #200971 - Flags: review?(bugreport)
Attachment #200972 - Flags: review?(bugreport)
Flags: approval?
Flags: approval2.20?
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+
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
Keywords: relnote
Whiteboard: [attachment 200972 applied to b.m.o] → [attachment 200972 applied to b.m.o] [relnote 2.20.1]
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]
Whiteboard: [attachment 200972 applied to b.m.o]
You need to log in before you can comment on or make changes to this bug.