From 5938a6efdce92871400a7bf9f686cad775943090 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Tue, 3 Jul 2018 13:42:45 +0100 Subject: [PATCH] rdbms: add more comments and sanity checks for CONN_TRX_AUTOCOMMIT Change-Id: I69992cf2e2ae3ef62125b0bc733a0cb7274f814e --- includes/jobqueue/JobQueueDB.php | 9 +++---- .../libs/rdbms/loadbalancer/ILoadBalancer.php | 26 ++++++++++++------- .../libs/rdbms/loadbalancer/LoadBalancer.php | 7 +++++ .../phpunit/includes/db/LoadBalancerTest.php | 14 ++++++++++ 4 files changed, 41 insertions(+), 15 deletions(-) diff --git a/includes/jobqueue/JobQueueDB.php b/includes/jobqueue/JobQueueDB.php index bc737188f9..689326e8b6 100644 --- a/includes/jobqueue/JobQueueDB.php +++ b/includes/jobqueue/JobQueueDB.php @@ -221,7 +221,7 @@ class JobQueueDB extends JobQueue { $rowSet = []; // (sha1 => job) map for jobs that are de-duplicated $rowList = []; // list of jobs for jobs that are not de-duplicated foreach ( $jobs as $job ) { - $row = $this->insertFields( $job ); + $row = $this->insertFields( $job, $dbw ); if ( $job->ignoreDuplicates() ) { $rowSet[$row['job_sha1']] = $row; } else { @@ -722,11 +722,10 @@ class JobQueueDB extends JobQueue { /** * @param IJobSpecification $job + * @param IDatabase $db * @return array */ - protected function insertFields( IJobSpecification $job ) { - $dbw = $this->getMasterDB(); - + protected function insertFields( IJobSpecification $job, IDatabase $db ) { return [ // Fields that describe the nature of the job 'job_cmd' => $job->getType(), @@ -734,7 +733,7 @@ class JobQueueDB extends JobQueue { 'job_title' => $job->getTitle()->getDBkey(), 'job_params' => self::makeBlob( $job->getParams() ), // Additional job metadata - 'job_timestamp' => $dbw->timestamp(), + 'job_timestamp' => $db->timestamp(), 'job_sha1' => Wikimedia\base_convert( sha1( serialize( $job->getDeduplicationInfo() ) ), 16, 36, 31 diff --git a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php index 0ff6a32980..83ebd5188b 100644 --- a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php @@ -187,7 +187,8 @@ interface ILoadBalancer { /** * Get any open connection to a given server index, local or foreign * - * Use CONN_TRX_AUTOCOMMIT to only look for connections opened with that flag + * Use CONN_TRX_AUTOCOMMIT to only look for connections opened with that flag. + * Avoid the use of begin() or startAtomic() on any such connections. * * @param int $i Server index or DB_MASTER/DB_REPLICA * @param int $flags Bitfield of CONN_* class constants @@ -202,9 +203,10 @@ interface ILoadBalancer { * (e.g. sqlite) in order to avoid deadlocks. ILoadBalancer::getServerAttributes() * can be used to check such flags beforehand. * - * If the caller uses $domain or sets CONN_TRX_AUTOCOMMIT in $flags, then it must also - * call ILoadBalancer::reuseConnection() on the handle when finished using it. + * If the caller uses $domain or sets CONN_TRX_AUTOCOMMIT in $flags, then it must + * also call ILoadBalancer::reuseConnection() on the handle when finished using it. * In all other cases, this is not necessary, though not harmful either. + * Avoid the use of begin() or startAtomic() on any such connections. * * @param int $i Server index (overrides $groups) or DB_MASTER/DB_REPLICA * @param array|string|bool $groups Query group(s), or false for the generic reader @@ -236,7 +238,8 @@ interface ILoadBalancer { * * The CONN_TRX_AUTOCOMMIT flag is ignored for databases with ATTR_DB_LEVEL_LOCKING * (e.g. sqlite) in order to avoid deadlocks. ILoadBalancer::getServerAttributes() - * can be used to check such flags beforehand. + * can be used to check such flags beforehand. Avoid the use of begin() or startAtomic() + * on any CONN_TRX_AUTOCOMMIT connections. * * @see ILoadBalancer::getConnection() for parameter information * @@ -255,7 +258,8 @@ interface ILoadBalancer { * * The CONN_TRX_AUTOCOMMIT flag is ignored for databases with ATTR_DB_LEVEL_LOCKING * (e.g. sqlite) in order to avoid deadlocks. ILoadBalancer::getServerAttributes() - * can be used to check such flags beforehand. + * can be used to check such flags beforehand. Avoid the use of begin() or startAtomic() + * on any CONN_TRX_AUTOCOMMIT connections. * * @see ILoadBalancer::getConnection() for parameter information * @@ -274,7 +278,8 @@ interface ILoadBalancer { * * The CONN_TRX_AUTOCOMMIT flag is ignored for databases with ATTR_DB_LEVEL_LOCKING * (e.g. sqlite) in order to avoid deadlocks. ILoadBalancer::getServerAttributes() - * can be used to check such flags beforehand. + * can be used to check such flags beforehand. Avoid the use of begin() or startAtomic() + * on any CONN_TRX_AUTOCOMMIT connections. * * @see ILoadBalancer::getConnection() for parameter information * @@ -292,13 +297,14 @@ interface ILoadBalancer { * The index must be an actual index into the array. If a connection to the server is * already open and not considered an "in use" foreign connection, this simply returns it. * - * Avoid using CONN_TRX_AUTOCOMMIT for databases with ATTR_DB_LEVEL_LOCKING (e.g. sqlite) in - * order to avoid deadlocks. ILoadBalancer::getServerAttributes() can be used to check + * Avoid using CONN_TRX_AUTOCOMMIT for databases with ATTR_DB_LEVEL_LOCKING (e.g. sqlite) + * in order to avoid deadlocks. ILoadBalancer::getServerAttributes() can be used to check * such flags beforehand. * - * If the caller uses $domain or sets CONN_TRX_AUTOCOMMIT in $flags, then it must also - * call ILoadBalancer::reuseConnection() on the handle when finished using it. + * If the caller uses $domain or sets CONN_TRX_AUTOCOMMIT in $flags, then it must + * also call ILoadBalancer::reuseConnection() on the handle when finished using it. * In all other cases, this is not necessary, though not harmful either. + * Avoid the use of begin() or startAtomic() on any such connections. * * @note This method throws DBAccessError if ILoadBalancer::disable() was called * diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php index f2f9eb0709..92cabca52f 100644 --- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php @@ -926,6 +926,13 @@ class LoadBalancer implements ILoadBalancer { } if ( $autoCommit && $conn instanceof IDatabase ) { + if ( $conn->trxLevel() ) { // sanity + throw new DBUnexpectedError( + $conn, + __METHOD__ . ': CONN_TRX_AUTOCOMMIT handle has a transaction.' + ); + } + $conn->clearFlag( $conn::DBO_TRX ); // auto-commit mode } diff --git a/tests/phpunit/includes/db/LoadBalancerTest.php b/tests/phpunit/includes/db/LoadBalancerTest.php index 5a748ccd0e..2c4e6b421b 100644 --- a/tests/phpunit/includes/db/LoadBalancerTest.php +++ b/tests/phpunit/includes/db/LoadBalancerTest.php @@ -311,6 +311,20 @@ class LoadBalancerTest extends MediaWikiTestCase { $lb->getAnyOpenConnection( $i, $lb::CONN_TRX_AUTOCOMMIT ) ); $this->assertEquals( $conn2, $lb->getConnection( $i, [], false, $lb::CONN_TRX_AUTOCOMMIT ) ); + + $conn2->startAtomic( __METHOD__ ); + try { + $lb->getConnection( $i, [], false, $lb::CONN_TRX_AUTOCOMMIT ); + $conn2->endAtomic( __METHOD__ ); + $this->fail( "No exception thrown." ); + } catch ( DBUnexpectedError $e ) { + $this->assertEquals( + 'Wikimedia\Rdbms\LoadBalancer::openConnection: ' . + 'CONN_TRX_AUTOCOMMIT handle has a transaction.', + $e->getMessage() + ); + } + $conn2->endAtomic( __METHOD__ ); } $lb->closeAll(); -- 2.20.1