From: Aaron Schulz Date: Wed, 20 Mar 2019 04:04:35 +0000 (-0700) Subject: rdbms: treat cloned temporary tables as "effective write" targets X-Git-Tag: 1.34.0-rc.0~2332^2 X-Git-Url: http://git.cyclocoop.org/%7B%24www_url%7Dadmin/compta/pie.php?a=commitdiff_plain;h=108fd8b18c1084de7af0bf05831ee9360f595c96;p=lhc%2Fweb%2Fwiklou.git rdbms: treat cloned temporary tables as "effective write" targets Make IDatabase::lastDoneWrites() reflect creation and changes to the cloned temporary unit test tables but not other temporary tables. This effects the LB method hasOrMadeRecentMasterChanges(). Other tables are assumpted to really just be there for temporary calculations rather acting as test-only ephemeral versions of permanent tables. Treating writes to the "fake permanent" temp tables more like real permanent tables means that the tests better align with production. At the moment, temporary tables still have to use DB_MASTER, given the assertIsWritableMaster() check in query(). This restriction can be lifted at some point, when RDBMs compatibility is robust. Bug: T218388 Change-Id: I4c0d629da254ac2aaf31aae35bd2efc7bc064ac6 --- diff --git a/includes/libs/rdbms/database/DBConnRef.php b/includes/libs/rdbms/database/DBConnRef.php index 5d80f836fc..5421b28a19 100644 --- a/includes/libs/rdbms/database/DBConnRef.php +++ b/includes/libs/rdbms/database/DBConnRef.php @@ -238,7 +238,7 @@ class DBConnRef implements IDatabase { return $this->__call( __FUNCTION__, func_get_args() ); } - public function query( $sql, $fname = __METHOD__, $tempIgnore = false ) { + public function query( $sql, $fname = __METHOD__, $flags = 0 ) { return $this->__call( __FUNCTION__, func_get_args() ); } diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index 9ce3086c38..dea7aab27d 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -280,6 +280,11 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware /** @var int No transaction is active */ const STATUS_TRX_NONE = 3; + /** @var int Writes to this temporary table do not affect lastDoneWrites() */ + const TEMP_NORMAL = 1; + /** @var int Writes to this temporary table effect lastDoneWrites() */ + const TEMP_PSEUDO_PERMANENT = 2; + /** * @note exceptions for missing libraries/drivers should be thrown in initConnection() * @param array $params Parameters passed from Database::factory() @@ -1135,47 +1140,55 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware /** * @param string $sql A SQL query - * @return bool Whether $sql is SQL for TEMPORARY table operation + * @param bool $pseudoPermanent Treat any table from CREATE TEMPORARY as pseudo-permanent + * @return int|null A self::TEMP_* constant for temp table operations or null otherwise */ - protected function registerTempTableOperation( $sql ) { + protected function registerTempTableWrite( $sql, $pseudoPermanent ) { + static $qt = '[`"\']?(\w+)[`"\']?'; // quoted table + if ( preg_match( - '/^CREATE\s+TEMPORARY\s+TABLE\s+(?:IF\s+NOT\s+EXISTS\s+)?[`"\']?(\w+)[`"\']?/i', + '/^CREATE\s+TEMPORARY\s+TABLE\s+(?:IF\s+NOT\s+EXISTS\s+)?' . $qt . '/i', $sql, $matches ) ) { - $this->sessionTempTables[$matches[1]] = 1; + $type = $pseudoPermanent ? self::TEMP_PSEUDO_PERMANENT : self::TEMP_NORMAL; + $this->sessionTempTables[$matches[1]] = $type; - return true; + return $type; } elseif ( preg_match( - '/^DROP\s+(?:TEMPORARY\s+)?TABLE\s+(?:IF\s+EXISTS\s+)?[`"\']?(\w+)[`"\']?/i', + '/^DROP\s+(?:TEMPORARY\s+)?TABLE\s+(?:IF\s+EXISTS\s+)?' . $qt . '/i', $sql, $matches ) ) { - $isTemp = isset( $this->sessionTempTables[$matches[1]] ); + $type = $this->sessionTempTables[$matches[1]] ?? null; unset( $this->sessionTempTables[$matches[1]] ); - return $isTemp; + return $type; } elseif ( preg_match( - '/^TRUNCATE\s+(?:TEMPORARY\s+)?TABLE\s+(?:IF\s+EXISTS\s+)?[`"\']?(\w+)[`"\']?/i', + '/^TRUNCATE\s+(?:TEMPORARY\s+)?TABLE\s+(?:IF\s+EXISTS\s+)?' . $qt . '/i', $sql, $matches ) ) { - return isset( $this->sessionTempTables[$matches[1]] ); + return $this->sessionTempTables[$matches[1]] ?? null; } elseif ( preg_match( - '/^(?:INSERT\s+(?:\w+\s+)?INTO|UPDATE|DELETE\s+FROM)\s+[`"\']?(\w+)[`"\']?/i', + '/^(?:(?:INSERT|REPLACE)\s+(?:\w+\s+)?INTO|UPDATE|DELETE\s+FROM)\s+' . $qt . '/i', $sql, $matches ) ) { - return isset( $this->sessionTempTables[$matches[1]] ); + return $this->sessionTempTables[$matches[1]] ?? null; } - return false; + return null; } - public function query( $sql, $fname = __METHOD__, $tempIgnore = false ) { + public function query( $sql, $fname = __METHOD__, $flags = 0 ) { $this->assertTransactionStatus( $sql, $fname ); $this->assertHasConnectionHandle(); + $flags = (int)$flags; // b/c; this field used to be a bool + $ignoreErrors = $this->hasFlags( $flags, self::QUERY_SILENCE_ERRORS ); + $pseudoPermanent = $this->hasFlags( $flags, self::QUERY_PSEUDO_PERMANENT ); + $priorTransaction = $this->trxLevel; $priorWritesPending = $this->writesOrCallbacksPending(); $this->lastQuery = $sql; @@ -1184,8 +1197,10 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware # In theory, non-persistent writes are allowed in read-only mode, but due to things # like https://bugs.mysql.com/bug.php?id=33669 that might not work anyway... $this->assertIsWritableMaster(); - # Avoid treating temporary table operations as meaningful "writes" - $isEffectiveWrite = !$this->registerTempTableOperation( $sql ); + # Do not treat temporary table writes as "meaningful writes" that need committing. + # Profile them as reads. Integration tests can override this behavior via $flags. + $tableType = $this->registerTempTableWrite( $sql, $pseudoPermanent ); + $isEffectiveWrite = ( $tableType !== self::TEMP_NORMAL ); } else { $isEffectiveWrite = false; } @@ -1240,12 +1255,12 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $this->trxStatus = self::STATUS_TRX_ERROR; $this->trxStatusCause = $this->getQueryExceptionAndLog( $lastError, $lastErrno, $sql, $fname ); - $tempIgnore = false; // cannot recover + $ignoreErrors = false; // cannot recover $this->trxStatusIgnoredCause = null; } } - $this->reportQueryError( $lastError, $lastErrno, $sql, $fname, $tempIgnore ); + $this->reportQueryError( $lastError, $lastErrno, $sql, $fname, $ignoreErrors ); } return $this->resultObject( $ret ); @@ -1514,17 +1529,17 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware /** * Report a query error. Log the error, and if neither the object ignore - * flag nor the $tempIgnore flag is set, throw a DBQueryError. + * flag nor the $ignoreErrors flag is set, throw a DBQueryError. * * @param string $error * @param int $errno * @param string $sql * @param string $fname - * @param bool $tempIgnore + * @param bool $ignoreErrors * @throws DBQueryError */ - public function reportQueryError( $error, $errno, $sql, $fname, $tempIgnore = false ) { - if ( $tempIgnore ) { + public function reportQueryError( $error, $errno, $sql, $fname, $ignoreErrors = false ) { + if ( $ignoreErrors ) { $this->queryLogger->debug( "SQL ERROR (ignored): $error\n" ); } else { $exception = $this->getQueryExceptionAndLog( $error, $errno, $sql, $fname ); @@ -4684,6 +4699,15 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $this->indexAliases = $aliases; } + /** + * @param int $field + * @param int $flags + * @return bool + */ + protected function hasFlags( $field, $flags ) { + return ( ( $field & $flags ) === $flags ); + } + /** * Get the underlying binding connection handle * diff --git a/includes/libs/rdbms/database/DatabaseMysqlBase.php b/includes/libs/rdbms/database/DatabaseMysqlBase.php index 25d78da18d..891325bf77 100644 --- a/includes/libs/rdbms/database/DatabaseMysqlBase.php +++ b/includes/libs/rdbms/database/DatabaseMysqlBase.php @@ -1427,7 +1427,7 @@ abstract class DatabaseMysqlBase extends Database { $oldName = $this->addIdentifierQuotes( $oldName ); $query = "CREATE $tmp TABLE $newName (LIKE $oldName)"; - return $this->query( $query, $fname ); + return $this->query( $query, $fname, $this::QUERY_PSEUDO_PERMANENT ); } /** diff --git a/includes/libs/rdbms/database/DatabasePostgres.php b/includes/libs/rdbms/database/DatabasePostgres.php index 8aec1acaea..01184bb113 100644 --- a/includes/libs/rdbms/database/DatabasePostgres.php +++ b/includes/libs/rdbms/database/DatabasePostgres.php @@ -819,8 +819,12 @@ __INDEXATTR__; $temporary = $temporary ? 'TEMPORARY' : ''; - $ret = $this->query( "CREATE $temporary TABLE $newNameE " . - "(LIKE $oldNameE INCLUDING DEFAULTS INCLUDING INDEXES)", $fname ); + $ret = $this->query( + "CREATE $temporary TABLE $newNameE " . + "(LIKE $oldNameE INCLUDING DEFAULTS INCLUDING INDEXES)", + $fname, + $this::QUERY_PSEUDO_PERMANENT + ); if ( !$ret ) { return $ret; } @@ -842,7 +846,10 @@ __INDEXATTR__; $fieldE = $this->addIdentifierQuotes( $field ); $newSeqE = $this->addIdentifierQuotes( $newSeq ); $newSeqQ = $this->addQuotes( $newSeq ); - $this->query( "CREATE $temporary SEQUENCE $newSeqE OWNED BY $newNameE.$fieldE", $fname ); + $this->query( + "CREATE $temporary SEQUENCE $newSeqE OWNED BY $newNameE.$fieldE", + $fname + ); $this->query( "ALTER TABLE $newNameE ALTER COLUMN $fieldE SET DEFAULT nextval({$newSeqQ}::regclass)", $fname diff --git a/includes/libs/rdbms/database/DatabaseSqlite.php b/includes/libs/rdbms/database/DatabaseSqlite.php index f2bc01d5b6..82a7e35a31 100644 --- a/includes/libs/rdbms/database/DatabaseSqlite.php +++ b/includes/libs/rdbms/database/DatabaseSqlite.php @@ -1018,7 +1018,7 @@ class DatabaseSqlite extends Database { } } - $res = $this->query( $sql, $fname ); + $res = $this->query( $sql, $fname, self::QUERY_PSEUDO_PERMANENT ); // Take over indexes $indexList = $this->query( 'PRAGMA INDEX_LIST(' . $this->addQuotes( $oldName ) . ')' ); diff --git a/includes/libs/rdbms/database/IDatabase.php b/includes/libs/rdbms/database/IDatabase.php index 6f58cc66a3..eac9baed0e 100644 --- a/includes/libs/rdbms/database/IDatabase.php +++ b/includes/libs/rdbms/database/IDatabase.php @@ -106,6 +106,14 @@ interface IDatabase { /** @var int Enable compression in connection protocol */ const DBO_COMPRESS = 512; + /** @var int Ignore query errors and return false when they happen */ + const QUERY_SILENCE_ERRORS = 1; // b/c for 1.32 query() argument; note that (int)true = 1 + /** + * @var int Treat the TEMPORARY table from the given CREATE query as if it is + * permanent as far as write tracking is concerned. This is useful for testing. + */ + const QUERY_PSEUDO_PERMANENT = 2; + /** * A string describing the current software version, and possibly * other details in a user-friendly way. Will be listed on Special:Version, etc. @@ -527,13 +535,13 @@ interface IDatabase { * @param string $sql SQL query * @param string $fname Name of the calling function, for profiling/SHOW PROCESSLIST * comment (you can use __METHOD__ or add some extra info) - * @param bool $tempIgnore Whether to avoid throwing an exception on errors... - * maybe best to catch the exception instead? + * @param int $flags Bitfield of IDatabase::QUERY_* constants. Note that suppression + * of errors is best handled by try/catch rather than using one of these flags. * @return bool|IResultWrapper True for a successful write query, IResultWrapper object - * for a successful read query, or false on failure if $tempIgnore set + * for a successful read query, or false on failure if QUERY_SILENCE_ERRORS is set. * @throws DBError */ - public function query( $sql, $fname = __METHOD__, $tempIgnore = false ); + public function query( $sql, $fname = __METHOD__, $flags = 0 ); /** * Free a result object returned by query() or select(). It's usually not diff --git a/tests/phpunit/includes/db/DatabaseSqliteTest.php b/tests/phpunit/includes/db/DatabaseSqliteTest.php index e61bd05a67..0818adccf8 100644 --- a/tests/phpunit/includes/db/DatabaseSqliteTest.php +++ b/tests/phpunit/includes/db/DatabaseSqliteTest.php @@ -531,7 +531,7 @@ class DatabaseSqliteMock extends DatabaseSqlite { return Database::factory( 'SqliteMock', $p ); } - function query( $sql, $fname = '', $tempIgnore = false ) { + function query( $sql, $fname = '', $flags = 0 ) { return true; } diff --git a/tests/phpunit/includes/db/DatabaseTestHelper.php b/tests/phpunit/includes/db/DatabaseTestHelper.php index 9679c6cec2..fb4041dd92 100644 --- a/tests/phpunit/includes/db/DatabaseTestHelper.php +++ b/tests/phpunit/includes/db/DatabaseTestHelper.php @@ -133,10 +133,10 @@ class DatabaseTestHelper extends Database { return $s; } - public function query( $sql, $fname = '', $tempIgnore = false ) { + public function query( $sql, $fname = '', $flags = 0 ) { $this->checkFunctionName( $fname ); - return parent::query( $sql, $fname, $tempIgnore ); + return parent::query( $sql, $fname, $flags ); } public function tableExists( $table, $fname = __METHOD__ ) { diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php index 40c260cc94..c0d25553dc 100644 --- a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php +++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php @@ -1343,7 +1343,7 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { } /** - * @covers Wikimedia\Rdbms\Database::registerTempTableOperation + * @covers Wikimedia\Rdbms\Database::registerTempTableWrite */ public function testSessionTempTables() { $temp1 = $this->database->tableName( 'tmp_table_1' );