Maniphest T207223

getDBLoadBalancerFactory()->getExternalLB() returns a LB with the wrong database.
Closed, DeclinedPublic
Actions

Assigned To

Authored By

	Alexia
	Oct 16 2018, 9:59 PM

Project Tags

Referenced Files

	F26616440: mw1-31.png
	Oct 16 2018, 9:59 PM

	F26616439: mw1-29.png
	Oct 16 2018, 9:59 PM

Subscribers

Aklapper

Alexia

Legoktm

Description

When calling in MW 1.31.1:

\MediaWiki\MediaWikiServices::getInstance()->getDBLoadBalancerFactory()->getExternalLB('master')->getConnection(DB_MASTER);

It returns a load balancer with the database of the local wiki instead of the correct(global/central) database defined in $wgExternalServers. The attached images are annotated to show the issue. This functionality works correctly in MW 1.29, but something has happened since then that causes it to get the incorrect database.

I spoke with legoktm on the #mediawiki IRC channel. He referenced a selectDB() issue that was fixed in the mw-1.32-wmf branches. I back ported the MW 1.32 "includes/libs/rdbms/" to test and unfortunately that did not solve the issue.

The scope is that it breaks internal extensions on Gamepedia/Hydra in addition to the popular Extension:AbuseFilter and Extension:Echo(EchoDbFactory).

$wgExternalServers:

$wgExternalServers	= [
	'master'	=> [
		[
			'host'		=> '127.0.0.1',
			'dbname'	=> 'wikiauth',
			'user'		=> 'bobdole',
			'password'	=> 'bobdole',
			'type'		=> $wgDBtype,
			'load'		=> 1
		]
	]
];

Relevant code:

/**
 * Get the global database connection.
 *
 * @access	public
 * @param	integer	$dbtype either DB_MASTER or DB_SLAVE.
 * @return	mixed	DatabaseBase or false on connection error.
 */
static public function getMasterDatabase($dbtype = DB_MASTER) {
	global $wgGlobalBlockingDatabase;

	if (self::$db !== null) {
		return self::$db;
	}

	try {
		self::$db = \MediaWiki\MediaWikiServices::getInstance()->getDBLoadBalancerFactory()->getExternalLB($wgGlobalBlockingDatabase)->getConnection($dbtype);
		return self::$db;
	} catch (Exception $e) {
		return false;
	}
}

[6682e743eeada4afb76ad73f] /Applejack_Wiki Wikimedia\Rdbms\DBQueryError from line 1458 of includes/libs/rdbms/database/Database.php: A database query error has occurred. Did you forget to run your application's database schema updater after upgrading?
Query: SELECT * FROM `global_block` WHERE global_id = '87049'
Function: GlobalBlock::load
Error: 1146 Table 'applejack_gamepedia.global_block' doesn't exist (127.0.0.1)

Backtrace:

#0 includes/libs/rdbms/database/Database.php(1428): Wikimedia\Rdbms\Database->makeQueryException(string, integer, string, string)
#1 includes/libs/rdbms/database/Database.php(1198): Wikimedia\Rdbms\Database->reportQueryError(string, integer, string, string, boolean)
#2 includes/libs/rdbms/database/Database.php(1655): Wikimedia\Rdbms\Database->query(string, string)
#3 extensions/GlobalBlock/classes/GlobalBlock.php(212): Wikimedia\Rdbms\Database->select(array, array, array, string)
#4 extensions/GlobalBlock/classes/GlobalBlock.php(147): GlobalBlock->load()
#5 extensions/GlobalBlock/GlobalBlock.hooks.php(89): GlobalBlock::newFromUser(User)
#6 includes/Hooks.php(177): GlobalBlockHooks::onGetBlockedStatus(User)
#7 includes/Hooks.php(205): Hooks::callHook(string, array, array, NULL)
#8 includes/user/User.php(1886): Hooks::run(string, array)
#9 includes/user/User.php(2224): User->getBlockedStatus(boolean)
#10 includes/user/User.php(2214): User->getBlock(boolean)
#11 includes/Title.php(2572): User->isBlocked()
#12 includes/Title.php(2737): Title->checkUserBlock(string, User, array, string, boolean)
#13 includes/Title.php(2114): Title->getUserPermissionsErrorsInternal(string, User, string, boolean)
#14 extensions/ApprovedRevs/includes/ApprovedRevs_body.php(165): Title->userCan(string, User)
#15 extensions/ApprovedRevs/includes/ApprovedRevs.hooks.php(476): ApprovedRevs::checkPermission(User, Title, string)
#16 includes/Hooks.php(177): ApprovedRevsHooks::setSubtitle(Article, string)
#17 includes/Hooks.php(205): Hooks::callHook(string, array, array, NULL)
#18 includes/page/Article.php(1322): Hooks::run(string, array)
#19 includes/page/Article.php(566): Article->setOldSubtitle(string)
#20 includes/actions/ViewAction.php(68): Article->view()
#21 includes/MediaWiki.php(500): ViewAction->show()
#22 includes/MediaWiki.php(294): MediaWiki->performAction(Article, Title)
#23 includes/MediaWiki.php(861): MediaWiki->performRequest()
#24 includes/MediaWiki.php(524): MediaWiki->main()
#25 index.php(42): MediaWiki->run()
#26 {main}

Event Timeline

Alexia created this task.Oct 16 2018, 9:59 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 16 2018, 9:59 PM

Krinkle moved this task from Untriaged to Rdbms library on the MediaWiki-libs-Rdbms board.Oct 16 2018, 10:01 PM

Krinkle added projects: MW-1.31-release, MW-1.32-release, Performance-Team.

Previously with MW 1.29 in LoadBalancer::reallyOpenConnection( array $server, $dbNameOverride = false ) $server['dbname'] would only be overwritten if an override was passed. An override was never passed though since LoadBalancer::openConnection() was always passing false for that parameter.

		if ( $dbNameOverride !== false ) {
			$server['dbname'] = $dbNameOverride;
		}

However, in MW 1.31(Possibly MW 1.30 as well) $this->localDomain is now always being passed to reallyOpenConnection(). As well, the behavior in reallyOpenConnection() has changed to always override $server['dbname'] when in the past it would only optionally override.

		// Handle $domainOverride being a specified or an unspecified domain
		if ( $domainOverride->getDatabase() === null ) {
			// Normally, an RDBMS requires a DB name specified on connection and the $server
			// configuration array is assumed to already specify an appropriate DB name.
			if ( $server['type'] === 'mysql' ) {
				// For MySQL, DATABASE and SCHEMA are synonyms, connections need not specify a DB,
				// and the DB name in $server might not exist due to legacy reasons (the default
				// domain used to ignore the local LB domain, even when mismatched).
				$server['dbname'] = null;
			}
		} else {
			$server['dbname'] = $domainOverride->getDatabase();
			$server['schema'] = $domainOverride->getSchema();
		}

We applied this patch as a temporary work around for our MW 1.31 testing.

diff --git a/includes/libs/rdbms/lbfactory/LBFactorySimple.php b/includes/libs/rdbms/lbfactory/LBFactorySimple.php
index 9a6aa3a74d..2a35a9fcdf 100644
--- a/includes/libs/rdbms/lbfactory/LBFactorySimple.php
+++ b/includes/libs/rdbms/lbfactory/LBFactorySimple.php
@@ -133,6 +133,11 @@ class LBFactorySimple extends LBFactory {
		$lb = new LoadBalancer( array_merge(
			$this->baseLoadBalancerParams(),
			[
+				'localDomain' => new DatabaseDomain(
+					$servers[0]['dbname'],
+					isset( $servers[0]['schema'] ) ? $servers[0]['schema'] : null,
+					isset( $servers[0]['tablePrefix'] ) ? $servers[0]['tablePrefix'] : ''
+				),
				'servers' => $servers,
				'maxLag' => $this->maxLag,
				'loadMonitor' => [ 'class' => $this->loadMonitorClass ],

Krinkle moved this task from Backlog to Extension bundling on the MW-1.31-release board.Oct 20 2018, 5:48 AM

Krinkle moved this task from Extension bundling to Core on the MW-1.31-release board.

aaron claimed this task.Oct 22 2018, 8:43 PM

aaron triaged this task as Low priority.

aaron moved this task from Inbox, needs triage to To-do: Goals prioritized current Quarter on the Performance-Team board.

In the getMasterDatabase() method posted above, I noticed that the database domain (e.g. DB/schema/prefix) is missing from getConnection(). Instead that should be:

$lbFactory = \MediaWiki\MediaWikiServices::getInstance()->getDBLoadBalancerFactory();
self::$db = $lbFactory->getExternalLB($wgGlobalBlockingDatabase)->getConnection($dbtype, false, $wgGlobalBlockingDatabase);

If no domain is passed to getConnection(), then it uses the local wiki database domain. This previously was an unenforced expectation in some code. The dbname is the server configuration array is just a default for when '' (the unspecified domain) is passed to getConnection(). GlobalBlocking, Echo, and AbuseFilter already seem to do pass in a config variable for the domain (e.g. wgGlobalBlockingDatabase), at least in git master where I checked.

Echo does not have a $wgEchoDatabase parameter for getEchoDb(), but if you are using a separate cluster, it assumes they are segregated by wiki. getSharedEchoDb() passes in $wgEchoSharedTrackingDB as a domain.

The REL1_31 GlobalBlocking (https://github.com/wikimedia/mediawiki-extensions-GlobalBlocking/blob/REL1_31/includes/GlobalBlocking.class.php) code looks right. What version of the extension are you using? I don't see code like getMasterDatabase(). Any internal code like that should be changed to have the domain passed in.

Extension:GlobalBlock(No 'ing) is an internal extension that works with Gamepedia/Twitch authentication servers. That code example is based on a previous version of Echo.

I am ticketing those changes in our internal Jira to be done and tested.

aaron closed this task as Declined.Oct 25 2018, 5:46 PM

The recommend fix does work for our extension. I apparently had configured Echo properly in the past so it was working properly. For AbuseFilter I had to patch it to use the same pattern since it only specifies the database and not the cluster to use.

In T207223#4703802, @Alexia wrote:

The recommend fix does work for our extension. I apparently had configured Echo properly in the past so it was working properly. For AbuseFilter I had to patch it to use the same pattern since it only specifies the database and not the cluster to use.

What is the patch diff? There shouldn't be any need to change AbuseFilter itself (at least not git master).

In T207223#4703891, @aaron wrote:

In T207223#4703802, @Alexia wrote:

The recommend fix does work for our extension. I apparently had configured Echo properly in the past so it was working properly. For AbuseFilter I had to patch it to use the same pattern since it only specifies the database and not the cluster to use.

What is the patch diff? There shouldn't be any need to change AbuseFilter itself (at least not git master).

I am working on cleaning up the patch to not be rushed so it supports both legacy and new before submitting it through Gerrit. Since it uses the "wfGetDB( DB_REPLICA, [], $wgAbuseFilterCentralDB );" pattern everywhere it will only change the database and not the entire cluster where needed. Previously I patched this and submitted a Gerrit review on it, but that was declined. https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/302608/

getDBLoadBalancerFactory()->getExternalLB() returns a LB with the wrong database.Closed, DeclinedPublicActions

Description

Event Timeline

getDBLoadBalancerFactory()->getExternalLB() returns a LB with the wrong database.
Closed, DeclinedPublic
Actions