rdbms: clean up return values of IDatabase write methods
authorAaron Schulz <aschulz@wikimedia.org>
Fri, 26 Oct 2018 20:17:34 +0000 (13:17 -0700)
committerAaron Schulz <aschulz@wikimedia.org>
Tue, 30 Oct 2018 03:34:52 +0000 (03:34 +0000)
Also improved the atomicity and affected row count logic for
insert/replace with sqlite.

Also remove unused "fileHandle" code from insert().

Change-Id: If7b9148fd44f3a958899885753c7c86ba66bf193

includes/db/DatabaseOracle.php
includes/libs/rdbms/database/Database.php
includes/libs/rdbms/database/DatabaseMssql.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/DatabaseTestHelper.php

index 6d921b9..696e81f 100644 (file)
@@ -390,13 +390,12 @@ class DatabaseOracle extends Database {
                foreach ( $a as &$row ) {
                        $this->insertOneRow( $table, $row, $fname );
                }
-               $retVal = true;
 
                if ( in_array( 'IGNORE', $options ) ) {
                        $this->ignoreDupValOnIndex = false;
                }
 
-               return $retVal;
+               return true;
        }
 
        private function fieldBindStatement( $table, $col, &$val, $includeCol = false ) {
@@ -580,13 +579,11 @@ class DatabaseOracle extends Database {
                        $this->ignoreDupValOnIndex = true;
                }
 
-               $retval = $this->query( $sql, $fname );
+               $this->query( $sql, $fname );
 
                if ( in_array( 'IGNORE', $insertOptions ) ) {
                        $this->ignoreDupValOnIndex = false;
                }
-
-               return $retval;
        }
 
        public function upsert( $table, array $rows, array $uniqueIndexes, array $set,
index 83b9660..2c40c72 100644 (file)
@@ -2038,7 +2038,6 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                        $options = [ $options ];
                }
 
-               $fh = $options['fileHandle'] ?? null;
                $options = $this->makeInsertOptions( $options );
 
                if ( isset( $a[0] ) && is_array( $a[0] ) ) {
@@ -2066,13 +2065,9 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                        $sql .= '(' . $this->makeList( $a ) . ')';
                }
 
-               if ( $fh !== null && false === fwrite( $fh, $sql ) ) {
-                       return false;
-               } elseif ( $fh !== null ) {
-                       return true;
-               }
+               $this->query( $sql, $fname );
 
-               return (bool)$this->query( $sql, $fname );
+               return true;
        }
 
        /**
@@ -2116,7 +2111,9 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                        $sql .= " WHERE " . $this->makeList( $conds, self::LIST_AND );
                }
 
-               return (bool)$this->query( $sql, $fname );
+               $this->query( $sql, $fname );
+
+               return true;
        }
 
        public function makeList( $a, $mode = self::LIST_COMMA ) {
@@ -2830,8 +2827,6 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
         * @param string $table Table name
         * @param array|string $rows Row(s) to insert
         * @param string $fname Caller function name
-        *
-        * @return ResultWrapper
         */
        protected function nativeReplace( $table, $rows, $fname ) {
                $table = $this->tableName( $table );
@@ -2854,7 +2849,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                        $sql .= '(' . $this->makeList( $row ) . ')';
                }
 
-               return $this->query( $sql, $fname );
+               $this->query( $sql, $fname );
        }
 
        public function upsert( $table, array $rows, array $uniqueIndexes, array $set,
@@ -2890,13 +2885,11 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                        $this->startAtomic( $fname, self::ATOMIC_CANCELABLE );
                        # Update any existing conflicting row(s)
                        if ( $where !== false ) {
-                               $ok = $this->update( $table, $set, $where, $fname );
+                               $this->update( $table, $set, $where, $fname );
                                $affectedRowCount += $this->affectedRows();
-                       } else {
-                               $ok = true;
                        }
                        # Now insert any non-conflicting row(s)
-                       $ok = $this->insert( $table, $rows, $fname, [ 'IGNORE' ] ) && $ok;
+                       $this->insert( $table, $rows, $fname, [ 'IGNORE' ] );
                        $affectedRowCount += $this->affectedRows();
                        $this->endAtomic( $fname );
                        $this->affectedRowCount = $affectedRowCount;
@@ -2905,7 +2898,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                        throw $e;
                }
 
-               return $ok;
+               return true;
        }
 
        public function deleteJoin( $delTable, $joinTable, $delVar, $joinVar, $conds,
@@ -2958,7 +2951,9 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                        $sql .= ' WHERE ' . $conds;
                }
 
-               return $this->query( $sql, $fname );
+               $this->query( $sql, $fname );
+
+               return true;
        }
 
        final public function insertSelect(
@@ -2973,7 +2968,18 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                if ( $this->cliMode && $this->isInsertSelectSafe( $insertOptions, $selectOptions ) ) {
                        // For massive migrations with downtime, we don't want to select everything
                        // into memory and OOM, so do all this native on the server side if possible.
-                       return $this->nativeInsertSelect(
+                       $this->nativeInsertSelect(
+                               $destTable,
+                               $srcTable,
+                               $varMap,
+                               $conds,
+                               $fname,
+                               array_diff( $insertOptions, $hints ),
+                               $selectOptions,
+                               $selectJoinConds
+                       );
+               } else {
+                       $this->nonNativeInsertSelect(
                                $destTable,
                                $srcTable,
                                $varMap,
@@ -2985,16 +2991,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                        );
                }
 
-               return $this->nonNativeInsertSelect(
-                       $destTable,
-                       $srcTable,
-                       $varMap,
-                       $conds,
-                       $fname,
-                       array_diff( $insertOptions, $hints ),
-                       $selectOptions,
-                       $selectJoinConds
-               );
+               return true;
        }
 
        /**
@@ -3020,7 +3017,6 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
         * @param array $insertOptions
         * @param array $selectOptions
         * @param array $selectJoinConds
-        * @return bool
         */
        protected function nonNativeInsertSelect( $destTable, $srcTable, $varMap, $conds,
                $fname = __METHOD__,
@@ -3038,7 +3034,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                        $srcTable, implode( ',', $fields ), $conds, $fname, $selectOptions, $selectJoinConds
                );
                if ( !$res ) {
-                       return false;
+                       return;
                }
 
                try {
@@ -3071,7 +3067,6 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                        } else {
                                $this->cancelAtomic( $fname );
                        }
-                       return $ok;
                } catch ( Exception $e ) {
                        $this->cancelAtomic( $fname );
                        throw $e;
@@ -3091,7 +3086,6 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
         * @param array $insertOptions
         * @param array $selectOptions
         * @param array $selectJoinConds
-        * @return bool
         */
        protected function nativeInsertSelect( $destTable, $srcTable, $varMap, $conds,
                $fname = __METHOD__,
@@ -3118,7 +3112,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                        " INTO $destTable (" . implode( ',', array_keys( $varMap ) ) . ') ' .
                        $selectSql;
 
-               return $this->query( $sql, $fname );
+               $this->query( $sql, $fname );
        }
 
        /**
index de102a7..93bb5d3 100644 (file)
@@ -513,6 +513,8 @@ class DatabaseMssql extends Database {
                        throw $e;
                }
                $this->scrollableCursor = true;
+
+               return true;
        }
 
        /**
@@ -741,7 +743,7 @@ class DatabaseMssql extends Database {
 
                $this->ignoreDupKeyErrors = false;
 
-               return $ret;
+               return true;
        }
 
        /**
@@ -757,15 +759,14 @@ class DatabaseMssql extends Database {
         * @param array $insertOptions
         * @param array $selectOptions
         * @param array $selectJoinConds
-        * @return bool
         * @throws Exception
         */
-       public function nativeInsertSelect( $destTable, $srcTable, $varMap, $conds, $fname = __METHOD__,
+       protected function nativeInsertSelect( $destTable, $srcTable, $varMap, $conds, $fname = __METHOD__,
                $insertOptions = [], $selectOptions = [], $selectJoinConds = []
        ) {
                $this->scrollableCursor = false;
                try {
-                       $ret = parent::nativeInsertSelect(
+                       parent::nativeInsertSelect(
                                $destTable,
                                $srcTable,
                                $varMap,
@@ -780,8 +781,6 @@ class DatabaseMssql extends Database {
                        throw $e;
                }
                $this->scrollableCursor = true;
-
-               return $ret;
        }
 
        /**
index cc9e98f..eeedff7 100644 (file)
@@ -493,10 +493,9 @@ abstract class DatabaseMysqlBase extends Database {
         * @param array $uniqueIndexes
         * @param array $rows
         * @param string $fname
-        * @return ResultWrapper
         */
        public function replace( $table, $uniqueIndexes, $rows, $fname = __METHOD__ ) {
-               return $this->nativeReplace( $table, $rows, $fname );
+               $this->nativeReplace( $table, $rows, $fname );
        }
 
        protected function isInsertSelectSafe( array $insertOptions, array $selectOptions ) {
@@ -1303,7 +1302,6 @@ abstract class DatabaseMysqlBase extends Database {
         * @param array|string $conds
         * @param bool|string $fname
         * @throws DBUnexpectedError
-        * @return bool|ResultWrapper
         */
        public function deleteJoin(
                $delTable, $joinTable, $delVar, $joinVar, $conds, $fname = __METHOD__
@@ -1320,7 +1318,7 @@ abstract class DatabaseMysqlBase extends Database {
                        $sql .= ' AND ' . $this->makeList( $conds, self::LIST_AND );
                }
 
-               return $this->query( $sql, $fname );
+               $this->query( $sql, $fname );
        }
 
        /**
@@ -1353,7 +1351,9 @@ abstract class DatabaseMysqlBase extends Database {
                $sql .= implode( ',', $rowTuples );
                $sql .= " ON DUPLICATE KEY UPDATE " . $this->makeList( $set, self::LIST_SET );
 
-               return (bool)$this->query( $sql, $fname );
+               $this->query( $sql, $fname );
+
+               return true;
        }
 
        /**
index e1cd764..adcd899 100644 (file)
@@ -670,9 +670,8 @@ __INDEXATTR__;
         * @param array $insertOptions
         * @param array $selectOptions
         * @param array $selectJoinConds
-        * @return bool
         */
-       public function nativeInsertSelect(
+       protected function nativeInsertSelect(
                $destTable, $srcTable, $varMap, $conds, $fname = __METHOD__,
                $insertOptions = [], $selectOptions = [], $selectJoinConds = []
        ) {
@@ -697,18 +696,18 @@ __INDEXATTR__;
                                $sql = "INSERT INTO $destTable (" . implode( ',', array_keys( $varMap ) ) . ') ' .
                                        $selectSql . ' ON CONFLICT DO NOTHING';
 
-                               return $this->query( $sql, $fname );
+                               $this->query( $sql, $fname );
                        } else {
                                // IGNORE and we don't have ON CONFLICT DO NOTHING, so just use the non-native version
-                               return $this->nonNativeInsertSelect(
+                               $this->nonNativeInsertSelect(
                                        $destTable, $srcTable, $varMap, $conds, $fname,
                                        $insertOptions, $selectOptions, $selectJoinConds
                                );
                        }
+               } else {
+                       parent::nativeInsertSelect( $destTable, $srcTable, $varMap, $conds, $fname,
+                               $insertOptions, $selectOptions, $selectJoinConds );
                }
-
-               return parent::nativeInsertSelect( $destTable, $srcTable, $varMap, $conds, $fname,
-                       $insertOptions, $selectOptions, $selectJoinConds );
        }
 
        public function tableName( $name, $format = 'quoted' ) {
index 487e122..43776b5 100644 (file)
@@ -25,6 +25,7 @@ namespace Wikimedia\Rdbms;
 
 use PDO;
 use PDOException;
+use Exception;
 use LockManager;
 use FSLockManager;
 use InvalidArgumentException;
@@ -648,17 +649,23 @@ class DatabaseSqlite extends Database {
 
                # SQLite can't handle multi-row inserts, so divide up into multiple single-row inserts
                if ( isset( $a[0] ) && is_array( $a[0] ) ) {
-                       $ret = true;
-                       foreach ( $a as $v ) {
-                               if ( !parent::insert( $table, $v, "$fname/multi-row", $options ) ) {
-                                       $ret = false;
+                       $affectedRowCount = 0;
+                       try {
+                               $this->startAtomic( $fname, self::ATOMIC_CANCELABLE );
+                               foreach ( $a as $v ) {
+                                       parent::insert( $table, $v, "$fname/multi-row", $options );
                                }
+                               $this->endAtomic( $fname );
+                       } catch ( Exception $e ) {
+                               $this->cancelAtomic( $fname );
+                               throw $e;
                        }
+                       $this->affectedRowCount = $affectedRowCount;
                } else {
-                       $ret = parent::insert( $table, $a, "$fname/single-row", $options );
+                       parent::insert( $table, $a, "$fname/single-row", $options );
                }
 
-               return $ret;
+               return true;
        }
 
        /**
@@ -666,26 +673,30 @@ class DatabaseSqlite extends Database {
         * @param array $uniqueIndexes Unused
         * @param string|array $rows
         * @param string $fname
-        * @return bool|ResultWrapper
         */
        function replace( $table, $uniqueIndexes, $rows, $fname = __METHOD__ ) {
                if ( !count( $rows ) ) {
-                       return true;
+                       return;
                }
 
                # SQLite can't handle multi-row replaces, so divide up into multiple single-row queries
                if ( isset( $rows[0] ) && is_array( $rows[0] ) ) {
-                       $ret = true;
-                       foreach ( $rows as $v ) {
-                               if ( !$this->nativeReplace( $table, $v, "$fname/multi-row" ) ) {
-                                       $ret = false;
+                       $affectedRowCount = 0;
+                       try {
+                               $this->startAtomic( $fname, self::ATOMIC_CANCELABLE );
+                               foreach ( $rows as $v ) {
+                                       $this->nativeReplace( $table, $v, "$fname/multi-row" );
+                                       $affectedRowCount += $this->affectedRows();
                                }
+                               $this->endAtomic( $fname );
+                       } catch ( Exception $e ) {
+                               $this->cancelAtomic( $fname );
+                               throw $e;
                        }
+                       $this->affectedRowCount = $affectedRowCount;
                } else {
-                       $ret = $this->nativeReplace( $table, $rows, "$fname/single-row" );
+                       $this->nativeReplace( $table, $rows, "$fname/single-row" );
                }
-
-               return $ret;
        }
 
        /**
index 3d3f855..532d818 100644 (file)
@@ -924,8 +924,7 @@ interface IDatabase {
         * @param array $a Array of rows to insert
         * @param string $fname Calling function name (use __METHOD__) for logs/profiling
         * @param array $options Array of options
-        *
-        * @return bool
+        * @return bool Return true if no exception was thrown (deprecated since 1.33)
         * @throws DBError
         */
        public function insert( $table, $a, $fname = __METHOD__, $options = [] );
@@ -948,7 +947,7 @@ interface IDatabase {
         * @param array $options An array of UPDATE options, can be:
         *   - IGNORE: Ignore unique key conflicts
         *   - LOW_PRIORITY: MySQL-specific, see MySQL manual.
-        * @return bool
+        * @return bool Return true if no exception was thrown (deprecated since 1.33)
         * @throws DBError
         */
        public function update( $table, $values, $conds, $fname = __METHOD__, $options = [] );
@@ -1262,7 +1261,7 @@ interface IDatabase {
         *   things like "field = field + 1" or similar computed values.
         * @param string $fname Calling function name (use __METHOD__) for logs/profiling
         * @throws DBError
-        * @return bool
+        * @return bool Return true if no exception was thrown (deprecated since 1.33)
         */
        public function upsert(
                $table, array $rows, array $uniqueIndexes, array $set, $fname = __METHOD__
@@ -1300,7 +1299,7 @@ interface IDatabase {
         *   for the format. Use $conds == "*" to delete all rows
         * @param string $fname Name of the calling function
         * @throws DBUnexpectedError
-        * @return bool|IResultWrapper
+        * @return bool Return true if no exception was thrown (deprecated since 1.33)
         * @throws DBError
         */
        public function delete( $table, $conds, $fname = __METHOD__ );
@@ -1338,7 +1337,7 @@ interface IDatabase {
         * @param array $selectJoinConds Join conditions for the SELECT part of the query, see
         *    IDatabase::select() for details.
         *
-        * @return bool
+        * @return bool Return true if no exception was thrown (deprecated since 1.33)
         * @throws DBError
         */
        public function insertSelect( $destTable, $srcTable, $varMap, $conds,
index 936bee0..65b82ab 100644 (file)
@@ -148,7 +148,7 @@ class DatabaseTestHelper extends Database {
 
        // Redeclare parent method to make it public
        public function nativeReplace( $table, $rows, $fname ) {
-               return parent::nativeReplace( $table, $rows, $fname );
+               parent::nativeReplace( $table, $rows, $fname );
        }
 
        function getType() {