Description:
I found in the mtr test file for the parameter 'innodb_parallel_read_threads', when setting it to a negative number, a log message was produced indicating that it should return ER_WRONG_VALUE_FOR_VAR when a negative value is used. Currently, the handling is to convert negative numbers to zero.
Here's the specific information from the file mysql-test/suite/sys_vars/t/innodb_parallel_read_threads_basic.test:
--echo NOTE: The following should fail with ER_WRONG_VALUE_FOR_VAR (BUG#50643)
set global innodb_parallel_read_threads=-3;
How to repeat:
Start the server with innodb enabled.
Using the mysql client run the following:
mysql> show global variables like '%parallel_read%';
+------------------------------+-------+
| Variable_name | Value |
+------------------------------+-------+
| innodb_parallel_read_threads | 4 |
+------------------------------+-------+
1 row in set (0.02 sec)
mysql> set global innodb_parallel_read_threads = -1;
Query OK, 0 rows affected, 1 warning (0.00 sec)
mysql> show warnings;
+---------+------+--------------------------------------------------------------+
| Level | Code | Message |
+---------+------+--------------------------------------------------------------+
| Warning | 1292 | Truncated incorrect innodb_parallel_read_threads value: '-1' |
+---------+------+--------------------------------------------------------------+
1 row in set (0.00 sec)
Suggested fix:
I believe this concerns a scenario where a parameter is given a value outside its valid range. From what I've observed, for unsigned parameters, a negative value is currently adjusted to zero. Similarly, other values are coerced into the valid range using min/max/block_size to ensure they're within bounds.
These logics are reflected in the file sql/sql_plugin_var.cc:
check_func_int/check_func_long/check_func_longlong
if (var->flags & PLUGIN_VAR_UNSIGNED) {
if ((fixed1 = (!value->is_unsigned(value) && val < 0))) val = 0;
*(uint *)save =
(uint)getopt_ull_limit_value((ulonglong)val, &options, &fixed2);
}
The approach mentioned above is indeed appropriate for unsigned parameters. However, this raises a design principle question: should we implicitly convert negative values to zero? In bug#50643(https://bugs.mysql.com/bug.php?id=50643), a fix was implemented to prevent bool types from being assigned negative values. Similarly, for unsigned type parameters, we should also return ER_WRONG_VALUE_FOR_VAR upon assigning a negative value, as it is evident that negative values are invalid.
Thus, to address this issue, an error should be returned when an unsigned parameter is assigned a negative value instead of converting it to zero.
Such as:
if (var->flags & PLUGIN_VAR_UNSIGNED) {
if ((fixed1 = (!value->is_unsigned(value) && val < 0))) return 1;
*(uint *)save =
(uint)getopt_ull_limit_value((ulonglong)val, &options, &fixed2);
}
Indeed, the current solution is acceptable and understandable. If we decide to retain this approach, I believe the mtr test file for 'innodb_parallel_read_threads' might need a slight modification. I am more than willing to address the issue, as I am enthusiastic about contributing to MySQL, no matter how insignificant the contribution might seem.
