Merge "rdbms: clean up DBO_TRX behavior for onTransactionPreCommitOrIdle()"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Thu, 22 Mar 2018 01:07:06 +0000 (01:07 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Thu, 22 Mar 2018 01:07:06 +0000 (01:07 +0000)
1  2 
includes/libs/rdbms/database/Database.php

@@@ -1061,7 -1061,7 +1061,7 @@@ abstract class Database implements IDat
                $ret = $this->doProfiledQuery( $sql, $commentedSql, $isNonTempWrite, $fname );
  
                # Try reconnecting if the connection was lost
 -              if ( false === $ret && $this->wasErrorReissuable() ) {
 +              if ( false === $ret && $this->wasConnectionLoss() ) {
                        $recoverable = $this->canRecoverFromDisconnect( $sql, $priorWritesPending );
                        # Stash the last error values before anything might clear them
                        $lastError = $this->lastError();
        }
  
        public function estimateRowCount(
 -              $table, $vars = '*', $conds = '', $fname = __METHOD__, $options = [], $join_conds = []
 +              $table, $var = '*', $conds = '', $fname = __METHOD__, $options = [], $join_conds = []
        ) {
 +              $conds = $this->normalizeConditions( $conds, $fname );
 +              $column = $this->extractSingleFieldFromList( $var );
 +              if ( is_string( $column ) && !in_array( $column, [ '*', '1' ] ) ) {
 +                      $conds[] = "$column IS NOT NULL";
 +              }
 +
                $res = $this->select(
                        $table, [ 'rowcount' => 'COUNT(*)' ], $conds, $fname, $options, $join_conds
                );
        }
  
        public function selectRowCount(
 -              $tables, $vars = '*', $conds = '', $fname = __METHOD__, $options = [], $join_conds = []
 +              $tables, $var = '*', $conds = '', $fname = __METHOD__, $options = [], $join_conds = []
        ) {
 -              $rows = 0;
 -              $sql = $this->selectSQLText( $tables, '1', $conds, $fname, $options, $join_conds );
 -              // The identifier quotes is primarily for MSSQL.
 -              $rowCountCol = $this->addIdentifierQuotes( "rowcount" );
 -              $tableName = $this->addIdentifierQuotes( "tmp_count" );
 -              $res = $this->query( "SELECT COUNT(*) AS $rowCountCol FROM ($sql) $tableName", $fname );
 +              $conds = $this->normalizeConditions( $conds, $fname );
 +              $column = $this->extractSingleFieldFromList( $var );
 +              if ( is_string( $column ) && !in_array( $column, [ '*', '1' ] ) ) {
 +                      $conds[] = "$column IS NOT NULL";
 +              }
 +
 +              $res = $this->select(
 +                      [
 +                              'tmp_count' => $this->buildSelectSubquery(
 +                                      $tables,
 +                                      '1',
 +                                      $conds,
 +                                      $fname,
 +                                      $options,
 +                                      $join_conds
 +                              )
 +                      ],
 +                      [ 'rowcount' => 'COUNT(*)' ],
 +                      [],
 +                      $fname
 +              );
 +              $row = $res ? $this->fetchRow( $res ) : [];
  
 -              if ( $res ) {
 -                      $row = $this->fetchRow( $res );
 -                      $rows = ( isset( $row['rowcount'] ) ) ? (int)$row['rowcount'] : 0;
 +              return isset( $row['rowcount'] ) ? (int)$row['rowcount'] : 0;
 +      }
 +
 +      /**
 +       * @param array|string $conds
 +       * @param string $fname
 +       * @return array
 +       */
 +      final protected function normalizeConditions( $conds, $fname ) {
 +              if ( $conds === null || $conds === false ) {
 +                      $this->queryLogger->warning(
 +                              __METHOD__
 +                              . ' called from '
 +                              . $fname
 +                              . ' with incorrect parameters: $conds must be a string or an array'
 +                      );
 +                      $conds = '';
 +              }
 +
 +              if ( !is_array( $conds ) ) {
 +                      $conds = ( $conds === '' ) ? [] : [ $conds ];
                }
  
 -              return $rows;
 +              return $conds;
 +      }
 +
 +      /**
 +       * @param array|string $var Field parameter in the style of select()
 +       * @return string|null Column name or null; ignores aliases
 +       * @throws DBUnexpectedError Errors out if multiple columns are given
 +       */
 +      final protected function extractSingleFieldFromList( $var ) {
 +              if ( is_array( $var ) ) {
 +                      if ( !$var ) {
 +                              $column = null;
 +                      } elseif ( count( $var ) == 1 ) {
 +                              $column = isset( $var[0] ) ? $var[0] : reset( $var );
 +                      } else {
 +                              throw new DBUnexpectedError( $this, __METHOD__ . ': got multiple columns.' );
 +                      }
 +              } else {
 +                      $column = $var;
 +              }
 +
 +              return $column;
        }
  
        /**
                return 'CAST( ' . $field . ' AS INTEGER )';
        }
  
 +      public function buildSelectSubquery(
 +              $table, $vars, $conds = '', $fname = __METHOD__,
 +              $options = [], $join_conds = []
 +      ) {
 +              return new Subquery(
 +                      $this->selectSQLText( $table, $vars, $conds, $fname, $options, $join_conds )
 +              );
 +      }
 +
        public function databasesAreIndependent() {
                return false;
        }
        }
  
        public function tableName( $name, $format = 'quoted' ) {
 +              if ( $name instanceof Subquery ) {
 +                      throw new DBUnexpectedError(
 +                              $this,
 +                              __METHOD__ . ': got Subquery instance when expecting a string.'
 +                      );
 +              }
 +
                # Skip the entire process when we have a string quoted on both ends.
                # Note that we check the end so that we will still quote any use of
                # use of `database`.table. But won't break things if someone wants
                # any remote case where a word like on may be inside of a table name
                # surrounded by symbols which may be considered word breaks.
                if ( preg_match( '/(^|\s)(DISTINCT|JOIN|ON|AS)(\s|$)/i', $name ) !== 0 ) {
 +                      $this->queryLogger->warning(
 +                              __METHOD__ . ": use of subqueries is not supported this way.",
 +                              [ 'trace' => ( new RuntimeException() )->getTraceAsString() ]
 +                      );
 +
                        return $name;
                }
  
  
        /**
         * Get an aliased table name
 -       * e.g. tableName AS newTableName
         *
 -       * @param string $name Table name, see tableName()
 -       * @param string|bool $alias Alias (optional)
 +       * This returns strings like "tableName AS newTableName" for aliased tables
 +       * and "(SELECT * from tableA) newTablename" for subqueries (e.g. derived tables)
 +       *
 +       * @see Database::tableName()
 +       * @param string|Subquery $table Table name or object with a 'sql' field
 +       * @param string|bool $alias Table alias (optional)
         * @return string SQL name for aliased table. Will not alias a table to its own name
         */
 -      protected function tableNameWithAlias( $name, $alias = false ) {
 -              if ( !$alias || $alias == $name ) {
 -                      return $this->tableName( $name );
 +      protected function tableNameWithAlias( $table, $alias = false ) {
 +              if ( is_string( $table ) ) {
 +                      $quotedTable = $this->tableName( $table );
 +              } elseif ( $table instanceof Subquery ) {
 +                      $quotedTable = (string)$table;
 +              } else {
 +                      throw new InvalidArgumentException( "Table must be a string or Subquery." );
 +              }
 +
 +              if ( !strlen( $alias ) || $alias === $table ) {
 +                      if ( $table instanceof Subquery ) {
 +                              throw new InvalidArgumentException( "Subquery table missing alias." );
 +                      }
 +
 +                      return $quotedTable;
                } else {
 -                      return $this->tableName( $name ) . ' ' . $this->addIdentifierQuotes( $alias );
 +                      return $quotedTable . ' ' . $this->addIdentifierQuotes( $alias );
                }
        }
  
                return false;
        }
  
 -      public function wasErrorReissuable() {
 -              return false;
 +      public function wasConnectionLoss() {
 +              return $this->wasConnectionError( $this->lastErrno() );
        }
  
        public function wasReadOnlyError() {
                return false;
        }
  
 +      public function wasErrorReissuable() {
 +              return (
 +                      $this->wasDeadlock() ||
 +                      $this->wasLockTimeout() ||
 +                      $this->wasConnectionLoss()
 +              );
 +      }
 +
        /**
         * Do not use this method outside of Database/DBError classes
         *
                                $msg = "$fname: Explicit transaction already active (from {$this->trxFname}).";
                                throw new DBUnexpectedError( $this, $msg );
                        } else {
 -                              // @TODO: make this an exception at some point
                                $msg = "$fname: Implicit transaction already active (from {$this->trxFname}).";
 -                              $this->queryLogger->error( $msg );
 -                              return; // join the main transaction set
 +                              throw new DBUnexpectedError( $this, $msg );
                        }
                } elseif ( $this->getFlag( self::DBO_TRX ) && $mode !== self::TRANSACTION_INTERNAL ) {
 -                      // @TODO: make this an exception at some point
                        $msg = "$fname: Implicit transaction expected (DBO_TRX set).";
 -                      $this->queryLogger->error( $msg );
 -                      return; // let any writes be in the main transaction
 +                      throw new DBUnexpectedError( $this, $msg );
                }
  
                // Avoid fatals if close() was called
                                        "$fname: No transaction to commit, something got out of sync." );
                                return; // nothing to do
                        } elseif ( $this->trxAutomatic ) {
 -                              // @TODO: make this an exception at some point
 -                              $msg = "$fname: Explicit commit of implicit transaction.";
 -                              $this->queryLogger->error( $msg );
 -                              return; // wait for the main transaction set commit round
 +                              throw new DBUnexpectedError(
 +                                      $this,
 +                                      "$fname: Expected mass commit of all peer transactions (DBO_TRX set)."
 +                              );
                        }
                }
  
        }
  
        final public function rollback( $fname = __METHOD__, $flush = '' ) {
-               if ( $flush === self::FLUSHING_INTERNAL || $flush === self::FLUSHING_ALL_PEERS ) {
-                       if ( !$this->trxLevel ) {
-                               return; // nothing to do
-                       }
-               } else {
-                       if ( !$this->trxLevel ) {
-                               $this->queryLogger->error(
-                                       "$fname: No transaction to rollback, something got out of sync." );
-                               return; // nothing to do
-                       } elseif ( $this->getFlag( self::DBO_TRX ) ) {
+               $trxActive = $this->trxLevel;
+               if ( $flush !== self::FLUSHING_INTERNAL && $flush !== self::FLUSHING_ALL_PEERS ) {
+                       if ( $this->getFlag( self::DBO_TRX ) ) {
                                throw new DBUnexpectedError(
                                        $this,
 -                                      "$fname: Expected mass rollback of all peer databases (DBO_TRX set)."
 +                                      "$fname: Expected mass rollback of all peer transactions (DBO_TRX set)."
                                );
                        }
                }
  
-               // Avoid fatals if close() was called
-               $this->assertOpen();
+               if ( $trxActive ) {
+                       // Avoid fatals if close() was called
+                       $this->assertOpen();
  
-               $this->doRollback( $fname );
-               $this->trxAtomicLevels = [];
-               if ( $this->trxDoneWrites ) {
-                       $this->trxProfiler->transactionWritingOut(
-                               $this->server,
-                               $this->dbName,
-                               $this->trxShortId
-                       );
+                       $this->doRollback( $fname );
+                       $this->trxAtomicLevels = [];
+                       if ( $this->trxDoneWrites ) {
+                               $this->trxProfiler->transactionWritingOut(
+                                       $this->server,
+                                       $this->dbName,
+                                       $this->trxShortId
+                               );
+                       }
                }
  
-               $this->trxIdleCallbacks = []; // clear
-               $this->trxPreCommitCallbacks = []; // clear
-               try {
-                       $this->runOnTransactionIdleCallbacks( self::TRIGGER_ROLLBACK );
-               } catch ( Exception $e ) {
-                       // already logged; finish and let LoadBalancer move on during mass-rollback
-               }
-               try {
-                       $this->runTransactionListenerCallbacks( self::TRIGGER_ROLLBACK );
-               } catch ( Exception $e ) {
-                       // already logged; let LoadBalancer move on during mass-rollback
-               }
+               // Clear any commit-dependant callbacks. They might even be present
+               // only due to transaction rounds, with no SQL transaction being active
+               $this->trxIdleCallbacks = [];
+               $this->trxPreCommitCallbacks = [];
  
-               $this->affectedRowCount = 0; // for the sake of consistency
+               if ( $trxActive ) {
+                       try {
+                               $this->runOnTransactionIdleCallbacks( self::TRIGGER_ROLLBACK );
+                       } catch ( Exception $e ) {
+                               // already logged; finish and let LoadBalancer move on during mass-rollback
+                       }
+                       try {
+                               $this->runTransactionListenerCallbacks( self::TRIGGER_ROLLBACK );
+                       } catch ( Exception $e ) {
+                               // already logged; let LoadBalancer move on during mass-rollback
+                       }
+                       $this->affectedRowCount = 0; // for the sake of consistency
+               }
        }
  
        /**