The shadowlog table has lots of issues, and it would be better to deal with those by using replication instead. However, since mysql won't replicate between two servers on the same port, we need to allow for the shadowdb and the real db to be on different servers/ports. We should also support different sockets, otherwise mysql will ignore the port and use the default socket for localhost (which would be the wrong one). This means an extra (optional) param in localconfig for the main db, and params for host/port/socket path for the shadow stuff, and the appropriate code to open a second db connection if required. syncshadowdb would be removed, and the shadowlog table dropped from the db. Docs describing this would also be added. Note that this is obviously post-2.16. However, supporting the shadowdb post-pgsql and transaction support is likely to mean a lot of pain. MattyT pointed out that we'd need to obsolete this before removing it. I'm not sure if this is necessarily the case, though - we plan on upping the mysql requirements for 2.18 anyway, and the shadowlog isn't exactly the most reliable part of bugzilla anyway. Do we have any idea how many sites run with a shadowdb?

OK, I got board and hacked this up. The new params for port/host/socket have been added, and I've also added a param for whether or not bugzilla should do the db syncing. I started hacking at syncshadowdb to support the shadowdb being on a different host to the main db, but decided that it wasn't worth the effort. I did make syncshadowdb use the host/port options for the main db though. I don't think anyone had complained about those being missing before. (It also now uses the new socket param) I'll also attach my my.cnf file I've using - I basically changed the redhat startup scripts to use mysqld_multi to do the database starting. Anyway, I'd like to get this in for 2.16, so that we can deprecate it and then remove Param('updateshadowdb') after we branch. Comments?

Here we go. I created the replication user as documented in the mysql manual, shutdown mysql, changed the config file, did cp -pR /var/lib/mysql /var/lib/mysql2, and restarted. Do note that you'll probably want to flush and purge the update logs once a week or so. Doing so is probably semi-scriptable, I guess. If the slave is running, you can't purge a log which is in use anyway.

->2.16, but bump if you don't agree.

This makes fundamental changes in the way we connect to the database. I don't want to mess with it this close to release.

Fari enough. In that case, can I remove all the shadowlog stuff, or do you want one release where both methods are supported, but this is deprecated?

My personal opinion would be to just switch outright, since we all know shadowlog is busted. We'll need to make sure it gets discussed on mozilla-webtools a few weeks in advance of getting checked in, just so anyone who's actually using it doesn't get caught off-guard. On the other hand, maybe that's a good reason to support both temporarily... So people get a warning when running checksetup.pl that they're set up to use the old method and it'll be going away in a future release...

I'm not sure how busted shadowlog really is, given that the problems we have encountered are quite likely to have resulted from the very basic lack of locking in post_bug.cgi. It doesn't make a lot of sense to have our own replication mechanism when mysql has one already built-in, but it may take some installations a while to switch over, so while I think we can deprecate as soon as replication support is in, we should give installations a while to catch up to the times before we remove the code entirely.

No, it seems to rely on the bugs table being locked while writing to the shadowlog. I suspect that this is not the case for everything. Besides, that still doesn't explain the off by one, does it - the two entries would be swapped in that case. In any event, I posted to npm.webtools asking for users of this feature to comment. Lets see how many people respond.

I've had exactly 0 responses. bugzilla.gnome.org is the only public place I know of who may have enough bugs to be using this. Anyone know a contact address to email them on?

Oh, and this patch is broken. The reason is that ConnectToDatabase() currently does nothing if we're connected, but with my patch it will connect us to the main db. Only problem is that we call ConnectToDatabase() in all kinds of places, which will have this side effect. This function should only be called at the top of a .cgi script, noowhere else. I'll file a bug on that - its causing problems in cvs currently, so we need to do this for 2.16.

Comment on attachment 68767 [details] [diff] [review] v1 This is broken

OK, try this. Almost all the ConnectToDatabase calls were removed pre-2.16, so I removed the few (useless) remaining ones. Other changes, mainly to fix other bugs: - Clearing the token table in globals.pl won't work if we're connected to the shadowdb. This is probably a 'regression' from when it was first added in 2.15. - We can't rerun the checker for the shadowdb stuff - see comments in Bugzilla/Config (regression from my config stuff) - The taint checks on doeditparams mean that the CREATE DATABASE call in check_shadowdb would fail. Regression from enabling taint mode in pre-2.14. Its really obvious that noone uses this stuff... Improvments: - The user can set a path for the mysql socket. This is needed because when mysql is told to connect to localhost, it will use the socket in favour of the port, which is a problem when there are multiple databases on the same server (like, say, for replication). All the connection strings were changed to use this. Other notes: - Yes, ConnectToDatabase is ugly. It will get better once I can oo-ify it (See my Bugzilla::DBI patches for how it will end up looking) - Changing from a replicated db to a non-replicated one w/o changing the shadowdb param won't create the db. Basically, don't do this (Instead, drop teh db via mysql, change the params to not use a shadowdb, then change it back). Fixing it is a pain to avoid circular param dependencies, and its not worth the effort for a 'feature' which is to be removed. There are deliberatly no doc updates. The mysql docs are authoriative to set this up. I will attach a copy of my my.cnf file, though.

I've had a look at this patch, and I think it's going to be a big effort for me to work out what's going on here. One thing, however, would make a really big difference - and that's if the new replication system did not have anything called a "shadowdb". Having the new and old systems use the same terminology is _really_ confusing. Can't we call it querydb or seconddb or something? Just make it different. :-) Gerv

Err. It still is a shadwowdb, its just that someone else does the shadowing Yes, the code to handle this is ugly, which is why I'll drop non-replication support ASAP.

Index: Bug.pm - &::ConnectToDatabase(); - &::GetVersionTable(); - - # this verification should already have been done by caller - # my $loginok = quietly_check_login(); Does this have anything to do with this patch or was it just unnecessary? Index: checksetup.pl - $drh->func('createdb', $my_db_name, "$my_db_host:$my_db_port", $my_db_user, $my_db_pass, 'admin') + $dbh->func('createdb', $my_db_name, 'admin') This is the only use of $drh, so if it is no longer necessary, then you should eliminate it from line 1215 as well. Index: defparams.pl + name => 'queryagainstshadowdb', + desc => 'If this is on, and the shadowdb is set, then queries will ' . + 'happen against the shadow database.', Nit: "If this is on, and the <tt>shadowdb</tt> parameter is set, then query.cgi will query against the shadow database." + name => 'updateshadowdb', + desc => 'If this is on, and the shadowdb is set, then updates to the ' . + 'main database will be propgated to the shadow database. <b>This ' . + 'option is deprecated, and will be removed <u>BEFORE</u> the next ' . + 'bugzilla release.</b> You should use replication instead if you ' . + 'need this functionality, to avoid the risk of data ' . + 'inconsistancies. If you disable this, make sure that the ' . + 'shadowdb is already set up for replication, or queries will ' . + 'return stale data', Nit: "If this is on, and the <tt>shadowdb</tt> parameter is set, then Bugzilla will use the old style of shadow database in which it propagates changes to the shadow database itself. Otherwise Bugzilla will rely on MySQL replication to do the propagating. WARNING! The old-style is deprecated and is going away soon. You should use replication instead if you want this functionality, since the old style has problems with data consistency. If this parameter is on, and you disable it, make sure the shadow database is already set up for replication, or queries will return stale data." + # This entry must be _after_ updateshadowdb, because check_shadowdbhost uses + # that + { + name => 'shadowdbhost', + desc => 'The host which the shadowdb is on. If blank, then the main ' . + 'database host (defined in localconfig) is used instead. Note ' . + 'using this parameter requries that <tt>updateshadowdb</tt> is ' . + 'off, since bugzilla does not handle manually updating the ' . + 'shadowdb between different connections. Setting this will use a ' . + 'different database connection for the shadowdb, so you should ' . + 'leave this blank rather than set the host, port, and socket to ' . + 'the same as the localconfig settings.', Nit: "The host the shadow database is on. If blank, we assume it's on the main database host (the one defined in localconfig) and ignore the <tt>shadowdbport</tt> and <tt>shadowdbsock</tt> parameters below, which means that this parameter <em>must be filled in</em> if your shadow database is on a different instance of the mysql server, even if that instance runs on the same machine as the main database. Note that <tt>updateshadowdb</tt> must be off if the shadow database is on a different mysql instance, since Bugzilla can't propagate changes between instances itself, and this should be left blank if the shadow database is on the same instance, since Bugzilla can then reuse the same database connection for better performance." + name => 'shadowdbport', + desc => 'The port which the shadowdb is on. This setting will only be ' . + 'used if shadowdbhost is not empty.', Nit: "The port the shadow database is on. Ignored if <tt>shadowdbhost</tt> is blank. Note: if the host is the local machine, then MySQL will ignore this setting, and you must specify a socket below." + name => 'shadowdbsock', + desc => 'The socket which will be used to connect to the shadowdb. This ' . + 'setting will only be used is shadowdbhost is not empty. Note ' . + 'that if shadowdbhost is localhost, mysql will use its ' . + 'compiled-in socket, and ignore the port setting, so it is ' . + 'important to set this if you are running two different ' . + 'mysql instalations on localhost (which you need to do for ' . + 'mysql\'s replication to work.', Nit: "The socket used to connect to the shadow database, if the host is the local machine. Necessary because MySQL ignores the port specified by the client and connects directly to its compiled-in socket when a local client connects to it, so setting the port is insufficient to get Bugzilla to connect to the right server instance." name => 'shadowdb', desc => 'If non-empty, then this is the name of another database in ' . 'which Bugzilla will keep a shadow read-only copy of everything. ' . 'This is done so that long slow read-only operations can be used ' . - 'against this db, and not lock up things for everyone else. ' . + 'against this db, and not lock up things for everyone else.' . 'Turning on this parameter will create the given database ; be ' . - 'careful not to use the name of an existing database with useful ' . - 'data in it!', + 'careful not to use the name of an existing database with useful ' . 'data in it!', Hmm, these don't seem like useful changes :-). This message is also misleading, since the code errors out if the database already exists rather than obliterating it, but it doesn't really matter since this will go away quickly once replication is up and running properly. Index: globals.pl - if ($useshadow && Param("shadowdb") && Param("queryagainstshadowdb")) { - $name = Param("shadowdb"); - $::dbwritesallowed = 0; - } + $::dbwritesallowed = !$useshadow; + $useshadow = ($useshadow && Param("shadowdb") && + Param("queryagainstshadowdb")); This logic (don't allow writes if the user wanted the shadow database, even if they didn't get it) makes sense, but it's different from the previous behavior (don't allow writes if they get the shadow database). Is that going to cause problems? >(rest of code in ConnectToDatabase) Yuck! Please tell me this will get much better looking with either modulization or removal of old shadow database code. sub ReconnectToShadowDatabase { + # This will connect us to the shadowdb if we're not already connected, + # but if we're using the same dbh for both the main db and the shadowdb, + # be sure to USE the correct db "we make sure to USE the correct database." The use of an imperative suggests that the caller is responsible for USEing the correct database. + if (!Param("shadowdbhost")) { + SendSQL("USE " . Param("shadowdb")); + } Why doesn't ConnectToDatabase do this? +sub ReconnectToMainDatabase { + if (Param("shadowdb") && Param("queryagainstshadowdb")) { + ConnectToDatabase(); + if (!Param("shadowdbhost")) { + SendSQL("USE $::db_name"); + } Same question here. Index: syncshadowdb - my $cmd = "$::mysqlpath/mysqladmin -u $::db_user"; - if ($::db_pass) { $cmd .= " -p$::db_pass" } + my $cmd = "$::mysqlpath/mysqladmin -u $::db_user -h $::db_host -P $::db_port"; + if ($::db_pass) { $cmd .= " -p$::db_pass"; } + if ($::db_sock) { $cmd .= " -S$::db_sock"; } Can you use updateshadowdb with a database not running under the same MySQL instance? If not, why do this here?

Quick comments, although there is absolutely no way I am going to get to this before Fri 8/11^WNov, and very very unlikly that I will do so in the week after that. [ConnectToDatabase calls] Those do need to be removed; because of the changed CtD logic, we shouldn't be calling it more than once per script. The old code gave back the current handle, the new code gives back a non-shadowdb handle (because there was no argument). In practice, we don't use Bug.pm with a shadowdb, so noone will notice. > This logic (don't allow writes if the user wanted the shadow database, > even if they didn't get it) makes sense, but it's different from the > previous behavior (don't allow writes if they get the shadow database). > Is that going to cause problems? It shouldn't cause problems, since theres no difference to bmo. What it will do is cause an error on non-shadow db systems if the equivalent code would have errored out on a shadowdb. This makes sense from a development pov. The only problem I noticed was that Token::ClearTokenTable could end up being run even with a shadowdb; my patch fixes that. >>(rest of code in ConnectToDatabase) >Yuck! Please tell me this will get much better looking with either >modulization or removal of old shadow database code. Heh. Yeah, with a bit of both, really. scripts will get teh dbh handle, and use it directly, as required, so we won't have to have the 'what dbh do I need to use now' issue. >+ if (!Param("shadowdbhost")) { >+ SendSQL("USE " . Param("shadowdb")); >+ } Its an optimisation, sort of. If we already have a handle, then with mysql we can USE to change dbs rather than needing a new connection. This code is slated to die when syncshadowdb does, though, since replication requires teh separate host, and USE won't be of any use. >+ if ($::db_pass) { $cmd .= " -p$::db_pass"; } >+ if ($::db_sock) { $cmd .= " -S$::db_sock"; } >Can you use updateshadowdb with a database not running under the same >MySQL instance? If not, why do this here? You can't. However, theres no reason that the main db couldn't be running using a socket. If we accept the option (Which we do) it should be used everywhere.

>>+ if (!Param("shadowdbhost")) { >>+ SendSQL("USE " . Param("shadowdb")); >>+ } > >Its an optimisation, sort of. If we already have a handle, then with mysql we >can USE to change dbs rather than needing a new connection. This code is slated >to die when syncshadowdb does, though, since replication requires teh separate >host, and USE won't be of any use. I understand why to do this, I just don't understand why it's not part of the ConnectToDatabase function.

> I understand why to do this, I just don't understand why it's not part of the > ConnectToDatabase function. Because if we're already connected, then CtD will leave early. IF we weren't connected, then we don't want to USE. Its a bit ugly, but its vanishing as soon as bmo gets to use replication and I can pull all of this cruft out. > Nit: > "If this is on, and the <tt>shadowdb</tt> parameter is set, > then query.cgi will query against the shadow database." I added the <tt>, but left the text as is - its not just query.cgi which uses teh shadowdb. (I suspect you meant buglist.cgi, but even that doens't only use the shadowdb) <socket> > machine. Necessary because MySQL ignores the port specified by the client Thats not strictly true - unix sockets are faster than tcp, so its a perf thing too New patch coming in a bit

Hows this?

Comment on attachment 106742 [details] [diff] [review] nits fixed r=myk

a= justdave

FIXED. Bug 180870 filed to kill the old stuff, and write up the docs. Bug 140269 dependancy moved to there, too. (I purposely left the docs out of this patch, because they're just going to change again - I'll do them in bug 180870)