Page MenuHomePhabricator

LoadBalancer.php uses $wgDBname even though $wgDBservers is defined
Closed, ResolvedPublic

Description

My LocalSettings.php looks like this:

$wgDBservers = array(
    array(
        'host' => 'blah',
        'dbname' = > 'mediawiki',
       ...
    ),
);

I was not setting $wgDBname and this was working in MediaWiki 1.30.0. After upgrading to MediaWiki 1.32.0 I got DB connection errors because LoadBalancer.php was trying to connect to dbname "my_wiki". This is the default value from includes/DefaultSettings.php. This seems like incorrect behavior to me. Based on my reading of https://www.mediawiki.org/wiki/Manual:$wgDBservers I would expect $wgDBname to be ignored if $wgDBservers is specified.

As a workaround, I can confirm that setting $wgDBname = "mediawiki" in my LocalSettings.php causes things to work again.

I did only a small amount of debugging and it appears that on this line: https://phabricator.wikimedia.org/source/mediawiki/browse/master/includes/libs/rdbms/loadbalancer/LoadBalancer.php;de383fe4e41aff1463264bc420be055e257b1397$924
$server contains the good values that I want to use (the correct DB name--"mediawiki").
And $this->localDomain contains the incorrect value that I think should not be used ("my_wiki").

Related but different: I had a similar situation using $wgDBservers in 1.30.0, where I was forced to define $wgLocalDatabases even though $wgDBservers was defined: T193832
And maybe this line is overwriting the good value with the bad value: https://phabricator.wikimedia.org/source/mediawiki/browse/master/includes/libs/rdbms/loadbalancer/LoadBalancer.php;de383fe4e41aff1463264bc420be055e257b1397$1113

I'm not familiar with the MediaWiki source code at all and I don't understand what the intent is here, but it seems fishy.

It's not immediately obvious to me what changed between MediaWiki 1.30.0 and 1.32.0 that might have caused this, but these two commits might be relevant:

Assigning to aaron because it looks like he has worked on this code recently and might be familiar with it.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 12 2019, 12:10 AM
Markdoliner updated the task description. (Show Details)Feb 12 2019, 12:11 AM
Peachey88 updated the task description. (Show Details)Feb 12 2019, 10:18 AM
Peachey88 updated the task description. (Show Details)
aaron added a comment.Feb 15 2019, 1:02 AM

$wgDBname and $wgDBprefix are used by methods like wfWikiId() and "DB domain" methods from WikiMap. The former is a very old method. They are assumed to contain the DB name/prefix of the current wiki (wiki farms will set this depending on the vhost or URL or something).

One problem is that some code assumed that they and wfGetDB(DB_*)->getDBname() and wfWikiId() matched any $wgDBservers/$wgLBFactoryConf/$wgDBname value (if 'dbname' was set at all) but the LoadBalancer level used to ignore the current wiki DB name/prefix and just go off the arrays in $wgDBservers /$wgLBFactoryConf. An extension or core code storing or using getDBname() as a sanity check would fail if $wgDBname/wfWikiId() != <'dbname' value>.

Also, the LoadBalancer::getConnection() method would (and does) override anything in the server configuration array if a DB name (now "DB domain") is passed to that method. This meant that "foreign" wiki access (in farms) always used the specified foreign wiki ID to get the database to connect to, but the "local" wiki (e.g. no wiki ID argument) connections would use the server array fields (if set, $wgDBname otherwise). If the 'dbname' field was in the server config array then foreign wiki access would be broken, unless that field was dynamically set based on the current wiki (alongside $wgDBname to avoid other issues). So, it could fail to do something like:

wfGetDB( DB_* );  // use 'dbname' and connects to real 'mediawiki' DB

$currentWiki = wfGetDB(DB_*)->getDBname(); // my_wiki (some default value that has no corresponding DB)
wfGetDB( DB_*, [], $currentWiki ); // fails, no database called my_wiki

$currentWiki  = wfWikiId(); // my_wiki
wfGetDB( DB_*, [], $currentWiki ); // fails, no database called my_wiki

// Site using $wgLBFactoryConf with the LBFactoryMulti option ('mediawiki' belongs in cluster/section 's1').
// The server arrays still have 'dbname' keys with 'mediawiki' set for the cluster for the 'mediawiki' wiki.
// The DEFAULT cluster uses 'dbname' keys with 'wikidb' in its server config arrays.
$currentWiki  = wfWikiId(); // my_wiki
$lb = wfGetLB( $currentWiki ); // cant find the explicit DB cluster to use so uses 'DEFAULT' (not s1)
$lb->getConnection(DB_*); // uses the 'dbname' key of the server config array for a server in the wrong cluster (e.g. db='wikidb')
$lb->getConnection(DB_*,[],$currentWiki); // looks for wrong DB my_wiki on a server from the wrong cluster as well

Basically, they always had to match up to assure everything works, whereas as only some stuff would work otherwise, e.g. an extension storing wfWikiId() in a possibly-global table and using getConnection() using those field values as a parameter would break). Enforcing that local connections agree with wfWikiId() and LoadBalancer using a concept of the local domain (based on $wgDBname) inevitably was a breaking change to configurations that had mismatches.

The $wgLocalDatabases thing as basically the same thing; that JobQueue exception check would look at wgLocalDatabases *or* wfWikiId() in case the former was empty, but the later comes from $wgDBname (which I assume was my_wiki and not mediawiki). Some push() call was probably using ->getDBname() or ->getDomainId() on a local Database connection handle or something.

aaron added a comment.Mar 22 2019, 7:30 AM

From T193832:

I don't see a way to avoid having to set $wgDBname. It was already used to determine things like wfWikiId(), which is used by some things to get DB connections. Having them both set and mismatched has long since been a use case that is not well supported. The correspondence needs better documentation though.

Maybe I could detect this in MWLBFactory and error out with an informative message. Mysql does not require specifying a DB in order to get a connection (unlike postgres) and 'dbname' is only used for when the $domain to wfGetDB() is '' (e.g. unspecified). That means that it's pretty much pointless to have it in $wgDBservers now, so if it exists and does not match $wgDBname, an error could be thrown. The 'schema' is always NULL for mysql. I suppose 'tablePrefix' could be checked against $wgDBprefix too.

Change 498313 had a related patch set uploaded (by Aaron Schulz; owner: Aaron Schulz):
[mediawiki/core@master] rdbms: halt on some common broken $wgDBServers configurations

https://gerrit.wikimedia.org/r/498313

Change 498313 merged by jenkins-bot:
[mediawiki/core@master] rdbms: halt on some common broken $wgDBServers configurations

https://gerrit.wikimedia.org/r/498313

Thanks for working on this, @aaron. I'm not familiar with the Mediawiki source code but your change seems reasonable to me. Assuming I understand this correctly, could I suggest updating https://www.mediawiki.org/wiki/Manual:$wgDBservers and/or https://www.mediawiki.org/wiki/Manual:$wgDBname and mentioning that $wgDBname should be set even if $wgDBservers is used? And that they should have the same value? I think that would have helped me. It looks like I probably have access to update those pages, but I think you'd probably do a better job since you're familiar with the rationale behind the requirement.

aaron added a comment.EditedMar 24 2019, 7:33 AM

You can even just omit the dbname from $wgDBservers. That is also a cleaner config for wiki farms. The servers (masters + X replicas) themselves don't really have a concept of "the table prefix", or even "the database" if it's mysql (rather than sqlite/postgres). Each wiki uses some db/prefix (might be the same DB or per-wiki or grouped or whatever). Calls to LoadBalancer::getConnection() or wfGetDB( DB_REPLICA, [], $wikiDbDomain ) can just use the db/prefix contained in $wikiDBDomain, which is $wgDBname and $wgDBprefix if it's the current wiki.

Before the concept of "local (wiki) DB domain" was added to LoadBalancer, then they had to both exist and happen to match the current wiki (with foreign wiki connections simply ignoring the db/prefix specified in $wgDBservers).

kchapman moved this task from Inbox to Doing on the Performance-Team board.May 13 2019, 7:48 PM
aaron closed this task as Resolved.May 16 2019, 8:01 PM

Thanks for working on this, @aaron. I'm not familiar with the Mediawiki source code but your change seems reasonable to me. Assuming I understand this correctly, could I suggest updating https://www.mediawiki.org/wiki/Manual:$wgDBservers and/or https://www.mediawiki.org/wiki/Manual:$wgDBname and mentioning that $wgDBname should be set even if $wgDBservers is used? And that they should have the same value? I think that would have helped me. It looks like I probably have access to update those pages, but I think you'd probably do a better job since you're familiar with the rationale behind the requirement.

I've updated those pages.