Merge "rdbms: make selectRowCount() use $var argument to exclude NULLs"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Wed, 21 Mar 2018 16:45:33 +0000 (16:45 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Wed, 21 Mar 2018 16:45:33 +0000 (16:45 +0000)
1  2 
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/IDatabase.php

@@@ -362,6 -362,13 +362,13 @@@ class DBConnRef implements IDatabase 
                return $this->__call( __FUNCTION__, func_get_args() );
        }
  
+       public function buildSelectSubquery(
+               $table, $vars, $conds = '', $fname = __METHOD__,
+               $options = [], $join_conds = []
+       ) {
+               return $this->__call( __FUNCTION__, func_get_args() );
+       }
        public function databasesAreIndependent() {
                return $this->__call( __FUNCTION__, func_get_args() );
        }
                return $this->__call( __FUNCTION__, func_get_args() );
        }
  
 -      public function wasErrorReissuable() {
 +      public function wasConnectionLoss() {
                return $this->__call( __FUNCTION__, func_get_args() );
        }
  
                return $this->__call( __FUNCTION__, func_get_args() );
        }
  
 +      public function wasErrorReissuable() {
 +              return $this->__call( __FUNCTION__, func_get_args() );
 +      }
 +
        public function masterPosWait( DBMasterPos $pos, $timeout ) {
                return $this->__call( __FUNCTION__, func_get_args() );
        }
@@@ -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 ) : [];
+               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 ( $res ) {
-                       $row = $this->fetchRow( $res );
-                       $rows = ( isset( $row['rowcount'] ) ) ? (int)$row['rowcount'] : 0;
+               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)."
 +                              );
                        }
                }
  
                        } elseif ( $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)."
                                );
                        }
                }
@@@ -558,18 -558,24 +558,24 @@@ abstract class DatabaseMysqlBase extend
         * Takes same arguments as Database::select()
         *
         * @param string|array $table
-        * @param string|array $vars
+        * @param string|array $var
         * @param string|array $conds
         * @param string $fname
         * @param string|array $options
         * @param array $join_conds
         * @return bool|int
         */
-       public function estimateRowCount( $table, $vars = '*', $conds = '',
+       public function estimateRowCount( $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";
+               }
                $options['EXPLAIN'] = true;
-               $res = $this->select( $table, $vars, $conds, $fname, $options, $join_conds );
+               $res = $this->select( $table, $var, $conds, $fname, $options, $join_conds );
                if ( $res === false ) {
                        return false;
                }
                return $this->lastErrno() == 1205;
        }
  
 -      public function wasErrorReissuable() {
 -              return $this->lastErrno() == 2013 || $this->lastErrno() == 2006;
 -      }
 -
        /**
         * Determines if the last failure was due to the database being read-only.
         *
@@@ -409,18 -409,24 +409,24 @@@ class DatabasePostgres extends Databas
         * Takes same arguments as Database::select()
         *
         * @param string $table
-        * @param string $vars
+        * @param string $var
         * @param string $conds
         * @param string $fname
         * @param array $options
         * @param array $join_conds
         * @return int
         */
-       public function estimateRowCount( $table, $vars = '*', $conds = '',
+       public function estimateRowCount( $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";
+               }
                $options['EXPLAIN'] = true;
-               $res = $this->select( $table, $vars, $conds, $fname, $options, $join_conds );
+               $res = $this->select( $table, $var, $conds, $fname, $options, $join_conds );
                $rows = -1;
                if ( $res ) {
                        $row = $this->fetchRow( $res );
@@@ -799,13 -805,7 +805,13 @@@ __INDEXATTR__
        }
  
        public function wasDeadlock() {
 -              return $this->lastErrno() == '40P01';
 +              // https://www.postgresql.org/docs/8.2/static/errcodes-appendix.html
 +              return $this->lastErrno() === '40P01';
 +      }
 +
 +      public function wasLockTimeout() {
 +              // https://www.postgresql.org/docs/8.2/static/errcodes-appendix.html
 +              return $this->lastErrno() === '55P03';
        }
  
        public function duplicateTableStructure(
@@@ -513,10 -513,6 +513,10 @@@ interface IDatabase 
         * Run an SQL query and return the result. Normally throws a DBQueryError
         * on failure. If errors are ignored, returns false instead.
         *
 +       * If a connection loss is detected, then an attempt to reconnect will be made.
 +       * For queries that involve no larger transactions or locks, they will be re-issued
 +       * for convenience, provided the connection was re-established.
 +       *
         * In new code, the query wrappers select(), insert(), update(), delete(),
         * etc. should be used where possible, since they give much better DBMS
         * independence and automatically quote or validate user input in a variety
         * This includes the user table in the query, with the alias "a" available
         * for use in field names (e.g. a.user_name).
         *
+        * A derived table, defined by the result of selectSQLText(), requires an alias
+        * key and a Subquery instance value which wraps the SQL query, for example:
+        *
+        *    [ 'c' => new Subquery( 'SELECT ...' ) ]
+        *
         * Joins using parentheses for grouping (since MediaWiki 1.31) may be
         * constructed using nested arrays. For example,
         *
         * doing UNION queries, where the SQL text of each query is needed. In general,
         * however, callers outside of Database classes should just use select().
         *
+        * @see IDatabase::select()
+        *
         * @param string|array $table Table name
         * @param string|array $vars Field names
         * @param string|array $conds Conditions
         * @param string $fname Caller function name
         * @param string|array $options Query options
         * @param string|array $join_conds Join conditions
-        *
-        * @return string SQL query string.
-        * @see IDatabase::select()
+        * @return string SQL query string
         */
        public function selectSQLText(
                $table, $vars, $conds = '', $fname = __METHOD__,
         * Takes the same arguments as IDatabase::select().
         *
         * @param string $table Table name
-        * @param string $vars Unused
+        * @param string $var Column for which NULL values are not counted [default "*"]
         * @param array|string $conds Filters on the table
         * @param string $fname Function name for profiling
         * @param array $options Options for select
         * @throws DBError
         */
        public function estimateRowCount(
-               $table, $vars = '*', $conds = '', $fname = __METHOD__, $options = [], $join_conds = []
+               $table, $var = '*', $conds = '', $fname = __METHOD__, $options = [], $join_conds = []
        );
  
        /**
         * @since 1.27 Added $join_conds parameter
         *
         * @param array|string $tables Table names
-        * @param string $vars Unused
+        * @param string $var Column for which NULL values are not counted [default "*"]
         * @param array|string $conds Filters on the table
         * @param string $fname Function name for profiling
         * @param array $options Options for select
         * @throws DBError
         */
        public function selectRowCount(
-               $tables, $vars = '*', $conds = '', $fname = __METHOD__, $options = [], $join_conds = []
+               $tables, $var = '*', $conds = '', $fname = __METHOD__, $options = [], $join_conds = []
        );
  
        /**
         */
        public function buildIntegerCast( $field );
  
+       /**
+        * Equivalent to IDatabase::selectSQLText() except wraps the result in Subqyery
+        *
+        * @see IDatabase::selectSQLText()
+        *
+        * @param string|array $table Table name
+        * @param string|array $vars Field names
+        * @param string|array $conds Conditions
+        * @param string $fname Caller function name
+        * @param string|array $options Query options
+        * @param string|array $join_conds Join conditions
+        * @return Subquery
+        * @since 1.31
+        */
+       public function buildSelectSubquery(
+               $table, $vars, $conds = '', $fname = __METHOD__,
+               $options = [], $join_conds = []
+       );
        /**
         * Returns true if DBs are assumed to be on potentially different servers
         *
        /**
         * Determines if the last failure was due to a deadlock
         *
 +       * Note that during a deadlock, the prior transaction will have been lost
 +       *
         * @return bool
         */
        public function wasDeadlock();
        /**
         * Determines if the last failure was due to a lock timeout
         *
 +       * Note that during a lock wait timeout, the prior transaction will have been lost
 +       *
         * @return bool
         */
        public function wasLockTimeout();
  
        /**
 -       * Determines if the last query error was due to a dropped connection and should
 -       * be dealt with by pinging the connection and reissuing the query.
 +       * Determines if the last query error was due to a dropped connection
 +       *
 +       * Note that during a connection loss, the prior transaction will have been lost
         *
         * @return bool
 +       * @since 1.31
         */
 -      public function wasErrorReissuable();
 +      public function wasConnectionLoss();
  
        /**
         * Determines if the last failure was due to the database being read-only.
         */
        public function wasReadOnlyError();
  
 +      /**
 +       * Determines if the last query error was due to something outside of the query itself
 +       *
 +       * Note that the transaction may have been lost, discarding prior writes and results
 +       *
 +       * @return bool
 +       */
 +      public function wasErrorReissuable();
 +
        /**
         * Wait for the replica DB to catch up to a given master position
         *