124589 - (bz-replication) support replication with mysql

Status: RESOLVED FIXED

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

Description

[Bradley Baetz (:bbaetz)]

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?

Comment 1

[Bradley Baetz (:bbaetz)]

Attachment: v1

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?

Comment 2

[Bradley Baetz (:bbaetz)]

Attachment: my new my.cnf file

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.

Comment 3

[Bradley Baetz (:bbaetz)]

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

Comment 4

[Dave Miller [:justdave]]

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

Comment 5

[Bradley Baetz (:bbaetz)]

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?

Comment 6

[Dave Miller [:justdave]]

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...

Comment 7

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

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.

Comment 8

[Bradley Baetz (:bbaetz)]

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.

Comment 9

[Bradley Baetz (:bbaetz)]

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?

Comment 10

[Bradley Baetz (:bbaetz)]

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 11

[Bradley Baetz (:bbaetz)]

Comment on attachment 68767 [details] [diff] [review]
v1

This is broken

Comment 12

[Bradley Baetz (:bbaetz)]

Attachment: v2

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.

Comment 13

[Bradley Baetz (:bbaetz)]

Attachment: newer my.cnf

Comment 14

[Gervase Markham [:gerv]]

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

Comment 15

[Bradley Baetz (:bbaetz)]

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.

Comment 16

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

Attachment: patch v2 unrotted

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?

Comment 17

[Bradley Baetz (:bbaetz)]

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.

Comment 18

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

>>+        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.

Comment 19

[Bradley Baetz (:bbaetz)]

> 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

Comment 20

[Bradley Baetz (:bbaetz)]

Attachment: nits fixed

Hows this?

Comment 21

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

Comment on attachment 106742 [details] [diff] [review]
nits fixed

r=myk

Comment 22

[Dave Miller [:justdave]]

a= justdave

Comment 23

[Bradley Baetz (:bbaetz)]

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)