ddl: wrong warning messages when running some DDL jobs.

[bb7133]

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

set sql_mode='';
drop table if exists t; 
create table t(a int);
insert into t values (1000000000), (2000000);
alter table t modify a tinyint;
show warnings;

2. What did you expect to see? (Required)

tidb> drop table if exists t;
Query OK, 0 rows affected (0.02 sec)

tidb> create table t(a int);
Query OK, 0 rows affected (0.01 sec)

tidb> insert into t values (1000000000), (2000000);
Query OK, 2 rows affected (0.00 sec)
Records: 2  Duplicates: 0  Warnings: 0

tidb> alter table t modify a tinyint;
Query OK, 0 rows affected, 2 warnings (2.53 sec)

tidb> show warnings;
+---------+------+---------------------------------------+
| Level   | Code | Message                               |
+---------+------+---------------------------------------+
| Warning | 1690 | constant 1000000000 overflows tinyint |
| Warning | 1690 | constant 2000000 overflows tinyint |
+---------+------+---------------------------------------+
2 rows in set (0.00 sec)

3. What did you see instead (Required)

tidb> drop table if exists t;
Query OK, 0 rows affected (0.02 sec)

tidb> create table t(a int);
Query OK, 0 rows affected (0.01 sec)

tidb> insert into t values (1000000000), (2000000);
Query OK, 2 rows affected (0.00 sec)
Records: 2  Duplicates: 0  Warnings: 0

tidb> alter table t modify a tinyint;
Query OK, 0 rows affected, 2 warnings (2.53 sec)

tidb> show warnings;
+---------+------+---------------------------------------+
| Level   | Code | Message                               |
+---------+------+---------------------------------------+
| Warning | 1690 | constant 1000000000 overflows tinyint |
| Warning | 1690 | constant 1000000000 overflows tinyint |
+---------+------+---------------------------------------+
2 rows in set (0.00 sec)

4. What is your TiDB version? (Required)

I believe it can be reproduced in all TiDB versions that support modifying the column as above.

Also, this issue should be reproduced for all other types of DDLs as long as multiple warnings might be raised.

[mjonss]

Since we are only storing one warning per warning code and a counter per unique warning code, I think we can change the warnings to something like:

+---------+------+---------------------------------------+
| Level   | Code | Message                               |
+---------+------+---------------------------------------+
| Warning | 1690 | 2 warning(s), first warning: constant 1000000000 overflows tinyint |
+---------+------+---------------------------------------+

[wxbty]

/assign

[wxbty]

There may be 10,000 rows of data, such a row will definitely not fit
Can we refer to mysql, or just fix the bug?

mysql5.7/8.0:
mysql> alter table t modify a tinyint;

Query OK, 2 rows affected, 2 warnings (0.02 sec)

Records: 2 Duplicates: 0 Warnings: 2

mysql> show warnings;

+---------+------+--------------------------------------------+

| Level | Code | Message |

+---------+------+--------------------------------------------+

| Warning | 1264 | Out of range value for column 'a' at row 1 |

| Warning | 1264 | Out of range value for column 'a' at row 2 |

+---------+------+--------------------------------------------+

[bb7133]

There may be 10,000 rows of data, such a row will definitely not fit Can we refer to mysql, or just fix the bug?

mysql5.7/8.0: mysql> alter table t modify a tinyint;

Query OK, 2 rows affected, 2 warnings (0.02 sec)

Records: 2 Duplicates: 0 Warnings: 2

mysql> show warnings;

+---------+------+--------------------------------------------+

| Level | Code | Message |

+---------+------+--------------------------------------------+

| Warning | 1264 | Out of range value for column 'a' at row 1 |

| Warning | 1264 | Out of range value for column 'a' at row 2 |

+---------+------+--------------------------------------------+

@wxbty Thank you for being interested in this issue.

IMHO it's fine for either @mjonss 's suggestion or the behavior of MySQL, but I think @mjonss is easier, that's it.

[wxbty]

ok

[mjonss]

There may be 10,000 rows of data, such a row will definitely not fit Can we refer to mysql, or just fix the bug?

@wxbty what I meant was literally say '$(count of the unique error code) warning(s), first warning: ...', not to concatenate all the warnings into a single column.
Since we store the warnings in a map with only the ErrorID, we only have access to the first warning that was reported for each ErrorID (basically error code) + another map for counting the repetitions. So we currently don't save the actual error messages (like which value was truncated, or which row had a warning).

And since we currently don't store each warning, just repeating the first warning over and over again is bad.

[wxbty]

There may be 10,000 rows of data, such a row will definitely not fit Can we refer to mysql, or just fix the bug?

@wxbty what I meant was literally say '$(count of the unique error code) warning(s), first warning: ...', not to concatenate all the warnings into a single column. Since we store the warnings in a map with only the ErrorID, we only have access to the first warning that was reported for each ErrorID (basically error code) + another map for counting the repetitions. So we currently don't save the actual error messages (like which value was truncated, or which row had a warning).

And since we currently don't store each warning, just repeating the first warning over and over again is bad.

I already understand. Will there be two different types of warnings? I haven't thought of it yet. This format needs to be changed, otherwise it will not be able to prompt the user.

[mjonss]

Will there be two different types of warnings? I haven't thought of it yet. This format needs to be changed, otherwise it will not be able to prompt the user.

@wxbty how do you mean?

To simplify I think we can just use what already exists (i.e. two different maps of ErrorID, one to the first warning string and the second to a counter). If the counter ReorgMeta.WarningsCount[ErrorID] is 1, then just append the warning as is (so no change), but if the counter is > 1 then instead of just multiplying the warnings (which is useless and confusing for the user, since it does not give any information for the actual warnings, just a copy of the first warning of that ErrorID) change the warning string to have " warnings of this error code, first warning: " prepended to the error message.

I think the related code is here

Also I think we should update the documenation, clarifying that our warnings for DDLs may differ from MySQLs

[wxbty]

ok