rdbms: make select() warn when FOR UPDATE is used with aggregation

Using FOR UPDATE or LOCK IN SHARE MODE with aggregation leads to
query errors with PostgreSQL.

Bug: T160910
Change-Id: Iaed964e7e59468365cbc62cb4bfd3ad44b898452

diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php
index 1779880..31ff3dc 100644 (file)
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -1641,8 +1641,9 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                return '';
        }
 
-       public function select( $table, $vars, $conds = '', $fname = __METHOD__,
-               $options = [], $join_conds = [] ) {
+       public function select(
+               $table, $vars, $conds = '', $fname = __METHOD__, $options = [], $join_conds = []
+       ) {
                $sql = $this->selectSQLText( $table, $vars, $conds, $fname, $options, $join_conds );
 
                return $this->query( $sql, $fname );
@@ -1652,7 +1653,9 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                $options = [], $join_conds = []
        ) {
                if ( is_array( $vars ) ) {
-                       $vars = implode( ',', $this->fieldNamesWithAlias( $vars ) );
+                       $fields = implode( ',', $this->fieldNamesWithAlias( $vars ) );
+               } else {
+                       $fields = $vars;
                }
 
                $options = (array)$options;
@@ -1666,6 +1669,18 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                        ? $options['IGNORE INDEX']
                        : [];
 
+               if (
+                       $this->selectOptionsIncludeLocking( $options ) &&
+                       $this->selectFieldsOrOptionsAggregate( $vars, $options )
+               ) {
+                       // Some DB types (postgres/oracle) disallow FOR UPDATE with aggregate
+                       // functions. Discourage use of such queries to encourage compatibility.
+                       call_user_func(
+                               $this->deprecationLogger,
+                               __METHOD__ . ": aggregation used with a locking SELECT ($fname)."
+                       );
+               }
+
                if ( is_array( $table ) ) {
                        $from = ' FROM ' .
                                $this->tableNamesWithIndexClauseOrJOIN(
@@ -1696,9 +1711,9 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                }
 
                if ( $conds === '' ) {
-                       $sql = "SELECT $startOpts $vars $from $useIndex $ignoreIndex $preLimitTail";
+                       $sql = "SELECT $startOpts $fields $from $useIndex $ignoreIndex $preLimitTail";
                } elseif ( is_string( $conds ) ) {
-                       $sql = "SELECT $startOpts $vars $from $useIndex $ignoreIndex " .
+                       $sql = "SELECT $startOpts $fields $from $useIndex $ignoreIndex " .
                                "WHERE $conds $preLimitTail";
                } else {
                        throw new DBUnexpectedError( $this, __METHOD__ . ' called with incorrect parameters' );
@@ -1783,6 +1798,49 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                return isset( $row['rowcount'] ) ? (int)$row['rowcount'] : 0;
        }
 
+       /**
+        * @param string|array $options
+        * @return bool
+        */
+       private function selectOptionsIncludeLocking( $options ) {
+               $options = (array)$options;
+               foreach ( [ 'FOR UPDATE', 'LOCK IN SHARE MODE' ] as $lock ) {
+                       if ( in_array( $lock, $options, true ) ) {
+                               return true;
+                       }
+               }
+
+               return false;
+       }
+
+       /**
+        * @param array|string $fields
+        * @param array|string $options
+        * @return bool
+        */
+       private function selectFieldsOrOptionsAggregate( $fields, $options ) {
+               foreach ( (array)$options as $key => $value ) {
+                       if ( is_string( $key ) ) {
+                               if ( preg_match( '/^(?:GROUP BY|HAVING)$/i', $key ) ) {
+                                       return true;
+                               }
+                       } elseif ( is_string( $value ) ) {
+                               if ( preg_match( '/^(?:DISTINCT|DISTINCTROW)$/i', $value ) ) {
+                                       return true;
+                               }
+                       }
+               }
+
+               $regex = '/^(?:COUNT|MIN|MAX|SUM|GROUP_CONCAT|LISTAGG|ARRAY_AGG)\s*\\(/i';
+               foreach ( (array)$fields as $field ) {
+                       if ( is_string( $field ) && preg_match( $regex, $field ) ) {
+                               return true;
+                       }
+               }
+
+               return false;
+       }
+
        /**
         * @param array|string $conds
         * @param string $fname

diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
index 3756da2..cfe9ad0 100644 (file)
--- a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
+++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
@@ -46,6 +46,8 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase {
         * @covers Wikimedia\Rdbms\Database::makeSelectOptions
         * @covers Wikimedia\Rdbms\Database::makeOrderBy
         * @covers Wikimedia\Rdbms\Database::makeGroupByWithHaving
+        * @covers Wikimedia\Rdbms\Database::selectFieldsOrOptionsAggregate
+        * @covers Wikimedia\Rdbms\Database::selectOptionsIncludeLocking
         */
        public function testSelect( $sql, $sqlText ) {
                $this->database->select(
@@ -223,9 +225,17 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase {
                                [
                                        'tables' => 'table',
                                        'fields' => [ 'field' ],
-                                       'options' => [ 'DISTINCT', 'LOCK IN SHARE MODE' ],
+                                       'options' => [ 'DISTINCT' ],
                                ],
-                               "SELECT DISTINCT field FROM table      LOCK IN SHARE MODE"
+                               "SELECT DISTINCT field FROM table"
+                       ],
+                       [
+                               [
+                                       'tables' => 'table',
+                                       'fields' => [ 'field' ],
+                                       'options' => [ 'LOCK IN SHARE MODE' ],
+                               ],
+                               "SELECT field FROM table      LOCK IN SHARE MODE"
                        ],
                        [
                                [