rdbms: treat cloned temporary tables as "effective write" targets
authorAaron Schulz <aschulz@wikimedia.org>
Wed, 20 Mar 2019 04:04:35 +0000 (21:04 -0700)
committerAaron Schulz <aschulz@wikimedia.org>
Tue, 26 Mar 2019 21:24:42 +0000 (14:24 -0700)
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

includes/libs/rdbms/database/DBConnRef.php
includes/libs/rdbms/database/Database.php
includes/libs/rdbms/database/DatabaseMysqlBase.php
includes/libs/rdbms/database/DatabasePostgres.php
includes/libs/rdbms/database/DatabaseSqlite.php
includes/libs/rdbms/database/IDatabase.php
tests/phpunit/includes/db/DatabaseSqliteTest.php
tests/phpunit/includes/db/DatabaseTestHelper.php
tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php

index 5d80f83..5421b28 100644 (file)
@@ -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() );
        }
 
index 9ce3086..dea7aab 100644 (file)
@@ -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
         *
index 25d78da..891325b 100644 (file)
@@ -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 );
        }
 
        /**
index 8aec1ac..01184bb 100644 (file)
@@ -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
index f2bc01d..82a7e35 100644 (file)
@@ -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 ) . ')' );
index 6f58cc6..eac9bae 100644 (file)
@@ -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
index e61bd05..0818adc 100644 (file)
@@ -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;
        }
 
index 9679c6c..fb4041d 100644 (file)
@@ -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__ ) {
index 40c260c..c0d2555 100644 (file)
@@ -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' );