Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ddl: fix Incorrect behavior of NO_ZERO_DATE when altering table #21564

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

ddl: fix Incorrect behavior of NO_ZERO_DATE when altering table #21564

wants to merge 7 commits into from

Conversation

7yyo
Copy link
Contributor

@7yyo 7yyo commented Dec 8, 2020

What problem does this PR solve?

Issue Number: close #11952

Problem Summary:

> mysql> select @@session.sql_mode;
> +-------------------------------------------------------------------------------------------------------------------------------------------+
> | @@session.sql_mode                                                                                                                        |
> +-------------------------------------------------------------------------------------------------------------------------------------------+
> | ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION |
> +-------------------------------------------------------------------------------------------------------------------------------------------+
> 1 row in set (0.00 sec)
> create table t(d int);
> insert into t values(1);
> select * from t;
> alter table t add column e date not null;
  • In NO_ZERO_DATE, add column default NOT NULL is not allowed.

  • Not in NO_ZERO_DATE in contrast.

What is changed and how it works?

> mysql> create table t(d int);
> Query OK, 0 rows affected (0.03 sec)
> 
> mysql> insert into t values(1);
> Query OK, 1 row affected (0.01 sec)
> 
> mysql> alter table t add column e date not null;
> ERROR 1292 (22007): Incorrect date value: '0000-00-00' for column 'e' at row 1

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Release note

  • fix Incorrect behavior of NO_ZERO_DATE when altering table

@7yyo 7yyo requested a review from a team as a code owner December 8, 2020 10:28
@7yyo 7yyo requested review from XuHuaiyu and removed request for a team December 8, 2020 10:28
@sre-bot
Copy link
Contributor

sre-bot commented Dec 8, 2020

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

  • *: what's changed

@7yyo 7yyo changed the title fix Incorrect behavior of NO_ZERO_DATE when altering table ddl: fix Incorrect behavior of NO_ZERO_DATE when altering table Dec 8, 2020
@github-actions github-actions bot added sig/sql-infra SIG: SQL Infra sig/execution SIG execution labels Dec 8, 2020
@ichn-hu ichn-hu mentioned this pull request Dec 8, 2020
@7yyo
Copy link
Contributor Author

7yyo commented Dec 8, 2020

/run-tics-test

@7yyo
Copy link
Contributor Author

7yyo commented Dec 9, 2020

@XuHuaiyu Hello,I am a little confused why the execution of tics-test failed, I think this DDL error report is the correct behavior, PTAL, thank you

@morgo
Copy link
Contributor

morgo commented Dec 9, 2020

@XuHuaiyu hello,I am a little confused why the execution of tics-test failed, I think this DDL error report is the correct behavior, PTAL, thank you

We have some internal tests that run in addition to the unit tests. The log seems to show that the test is failing because it relied on the previous behavior:

[2020-12-08T15:35:54.444Z] [2020/12/08 15:35:53.096 +00:00] [INFO] [ddl.go:594] ["[ddl] DDL job is finished"] [jobID=77]

[2020-12-08T15:35:54.444Z] [2020/12/08 15:35:53.096 +00:00] [INFO] [domain.go:660] ["performing DDL change, must reload"]

[2020-12-08T15:35:54.444Z] [2020/12/08 15:35:53.113 +00:00] [INFO] [session.go:2459] ["CRUCIAL OPERATION"] [conn=924937768587493515] [schemaVersion=68] [cur_db=] [sql="alter table test.t add date_a DATE NOT NULL"] [user=root@192.168.96.2]

[2020-12-08T15:35:54.444Z] [2020/12/08 15:35:53.114 +00:00] [INFO] [tidb.go:222] ["rollbackTxn for ddl/autocommit failed"]

[2020-12-08T15:35:54.444Z] [2020/12/08 15:35:53.114 +00:00] [WARN] [session.go:1257] ["run statement failed"] [conn=924937768587493515] [schemaVersion=68] [error="[table:1366]Incorrect date value: '0000-00-00' for column 'date_a' at row 1"] [session="{\n  \"currDBName\": \"\",\n  \"id\": 

I will take a look at this for you.

@7yyo
Copy link
Contributor Author

7yyo commented Dec 9, 2020

@morgo thx😊

@morgo
Copy link
Contributor

morgo commented Dec 9, 2020

Here is a simplified version of the test (it's an end-to-end test, testing tiflash among other things..). I have ran the queries against MySQL 8.0 and commented where they break, but not against your branch yet. I think some of the tests look really helpful though, you may want to include them in your tests:

drop table if exists test.t;
create table test.t(a int);
insert into test.t values (1);
alter table test.t add date_0 DATE NULL DEFAULT '1000-01-01';
alter table test.t add date_1 DATE NULL DEFAULT '9999-12-31';
alter table test.t add date_a DATE NOT NULL; -- fails on MySQL

alter table test.t add time_0 TIME NULL DEFAULT '59';
alter table test.t add time_1 TIME(6) NULL DEFAULT '-838:59:59.000000';
alter table test.t add time_2 TIME(6) NULL DEFAULT '838:59:59.000000';
alter table test.t add time_3 TIME(6) NULL DEFAULT '0';
alter table test.t add time_a TIME NOT NULL;
alter table test.t add time_b TIME(6) NOT NULL;

alter table test.t add datetime_0 DATETIME(6) NULL DEFAULT '1000-01-01 00:00:00.000000';
alter table test.t add datetime_1 DATETIME(6) NULL DEFAULT '9999-12-31 23:59:59.000000';
alter table test.t add datetime_a DATETIME NOT NULL; -- fails on MySQL
alter table test.t add datetime_b DATETIME(6) NOT NULL;  -- fails on MySQL

select * from test.t;
+------+------------+------------+------------+----------+-------------------+------------------+-----------------+----------+-----------------+----------------------------+----------------------------+---------------------+----------------------------+
| a    | date_0     | date_1     | date_a     | time_0   | time_1            | time_2           | time_3          | time_a   | time_b          | datetime_0                 | datetime_1                 | datetime_a          | datetime_b                 |
+------+------------+------------+------------+----------+-------------------+------------------+-----------------+----------+-----------------+----------------------------+----------------------------+---------------------+----------------------------+
|    1 | 1000-01-01 | 9999-12-31 | 0000-00-00 | 00:00:59 | -838:59:59.000000 | 838:59:59.000000 | 00:00:00.000000 | 00:00:00 | 00:00:00.000000 | 1000-01-01 00:00:00.000000 | 9999-12-31 23:59:59.000000 | 0000-00-00 00:00:00 | 0000-00-00 00:00:00.000000 |
+------+------------+------------+------------+----------+-------------------+------------------+-----------------+----------+-----------------+----------------------------+----------------------------+---------------------+----------------------------+


drop table if exists test.t;
create table test.t(a int);
insert into test.t values (1);
alter table test.t add timestamp_a TIMESTAMP NOT NULL;
alter table test.t add timestamp_b TIMESTAMP(6) NOT NULL;
select * from test.t;
+------+---------------------+----------------------------+
| a    | timestamp_a         | timestamp_b                |
+------+---------------------+----------------------------+
|    1 | 0000-00-00 00:00:00 | 0000-00-00 00:00:00.000000 |
+------+---------------------+----------------------------+

drop table if exists test.t;
create table test.t(a int);
insert into test.t values (1);
alter table test.t add year_0 YEAR NULL DEFAULT '1901';
alter table test.t add year_1 YEAR NULL DEFAULT '2155';
alter table test.t add year_2 YEAR NULL DEFAULT '0000';
alter table test.t add year_3 YEAR NULL DEFAULT '01';
alter table test.t add year_4 YEAR NULL DEFAULT '70';
alter table test.t add year_5 YEAR NULL DEFAULT '00';
alter table test.t add year_a YEAR NOT NULL;

select * from test.t;
+------+--------+--------+--------+--------+--------+--------+--------+
| a    | year_0 | year_1 | year_2 | year_3 | year_4 | year_5 | year_a |
+------+--------+--------+--------+--------+--------+--------+--------+
|    1 |   1901 |   2155 |   0000 |   2001 |   1970 |   2000 |   0000 |
+------+--------+--------+--------+--------+--------+--------+--------+

@morgo
Copy link
Contributor

morgo commented Dec 9, 2020

/run-tics-test tics=pr/1273

@morgo
Copy link
Contributor

morgo commented Dec 9, 2020

/run-all-tests tics=pr/1273

@morgo
Copy link
Contributor

morgo commented Dec 9, 2020

It looks like the bot doesn't accept the hint for tics=pr/1273. I will ask a colleague to merge, and we will let you know when its done.

@7yyo
Copy link
Contributor Author

7yyo commented Dec 9, 2020

CASE 1
MySQL 8.0.20

> mysql> select * from test.t;
> +------+------------+------------+----------+-------------------+------------------+-----------------+----------+-----------------+----------------------------+----------------------------+
> | a    | date_0     | date_1     | time_0   | time_1            | time_2           | time_3          | time_a   | time_b          | datetime_0                 | datetime_1                 |
> +------+------------+------------+----------+-------------------+------------------+-----------------+----------+-----------------+----------------------------+----------------------------+
> |    1 | 1000-01-01 | 9999-12-31 | 00:00:59 | -838:59:59.000000 | 838:59:59.000000 | 00:00:00.000000 | 00:00:00 | 00:00:00.000000 | 1000-01-01 00:00:00.000000 | 9999-12-31 23:59:59.000000 |
> +------+------------+------------+----------+-------------------+------------------+-----------------+----------+-----------------+----------------------------+----------------------------+
> 1 row in set (0.00 sec)

my branch

> mysql> select * from test.t;
> +------+------------+------------+----------+-------------------+------------------+-----------------+----------+-----------------+----------------------------+----------------------------+---------------------+----------------------------+
> | a    | date_0     | date_1     | time_0   | time_1            | time_2           | time_3          | time_a   | time_b          | datetime_0                 | datetime_1                 | datetime_a          | datetime_b                 |
> +------+------------+------------+----------+-------------------+------------------+-----------------+----------+-----------------+----------------------------+----------------------------+---------------------+----------------------------+
> |    1 | 1000-01-01 | 9999-12-31 | 00:00:59 | -838:59:59.000000 | 838:59:59.000000 | 00:00:00.000000 | 00:00:00 | 00:00:00.000000 | 1000-01-01 00:00:00.000000 | 9999-12-31 23:59:59.000000 | 0000-00-00 00:00:00 | 0000-00-00 00:00:00.000000 |
> +------+------------+------------+----------+-------------------+------------------+-----------------+----------+-----------------+----------------------------+----------------------------+---------------------+----------------------------+
> 1 row in set (0.00 sec)

CASE 2
MySQL 8.0.20

> mysql> select * from test.t;
> +------+---------------------+----------------------------+
> | a    | timestamp_a         | timestamp_b                |
> +------+---------------------+----------------------------+
> |    1 | 0000-00-00 00:00:00 | 0000-00-00 00:00:00.000000 |
> +------+---------------------+----------------------------+
> 1 row in set (0.00 sec)

my branch

> mysql> select * from test.t;
> +------+---------------------+----------------------------+
> | a    | timestamp_a         | timestamp_b                |
> +------+---------------------+----------------------------+
> |    1 | 0000-00-00 00:00:00 | 0000-00-00 00:00:00.000000 |
> +------+---------------------+----------------------------+
> 1 row in set (0.00 sec)

CASE 3
MySQL 8.0.20

> mysql> select * from test.t;
> +------+--------+--------+--------+--------+--------+--------+--------+
> | a    | year_0 | year_1 | year_2 | year_3 | year_4 | year_5 | year_a |
> +------+--------+--------+--------+--------+--------+--------+--------+
> |    1 |   1901 |   2155 |   0000 |   2001 |   1970 |   2000 |   0000 |
> +------+--------+--------+--------+--------+--------+--------+--------+
> 1 row in set (0.00 sec)

my branch

> mysql> select * from test.t;
> +------+--------+--------+--------+--------+--------+--------+--------+
> | a    | year_0 | year_1 | year_2 | year_3 | year_4 | year_5 | year_a |
> +------+--------+--------+--------+--------+--------+--------+--------+
> |    1 |   1901 |   2155 |   0000 |   2001 |   1970 |   2000 |   0000 |
> +------+--------+--------+--------+--------+--------+--------+--------+
> 1 row in set (0.00 sec)

I think I need to add restrictions on the datetime type, thank you for your help @morgo 🙏
I will add some test case.

@bb7133
Copy link
Member

bb7133 commented Dec 9, 2020

Hi @sev7ndayyoo , thanks for trying to solve this issue!

There are some cases we need to consider:

  1. If the session is not in strict mode, the alter table can be succeeded even when NO_ZERO_DATE is set(there are just warnings). However, this is something trivial, IMO it's fine to leave it to a separate PR.

  2. Adding the column for an empty table should be acceptable, that is:

    create table t(d int);
    alter table t add column e date not null;
    

    It succeeded in MySQL but reports an error in the current PR.

  3. If you solve case 2 above by checking if the table is empty at the beginning of the DDL. It is possible that the new records are inserted during the execution of DDL jobs(there will be 4 states for ALTER TABLE ... ADD COLUMN) because, in TiDB, DDL does not block the execution of DML. This is the tricky part of this issue.

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2021
@ti-chi-bot
Copy link
Member

@7yyo: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@guo-shaoge guo-shaoge removed the request for review from XuHuaiyu June 10, 2021 03:16
@ti-chi-bot
Copy link

ti-chi-bot bot commented Jul 6, 2023

@7yyo: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-br-integration-test c0a81f9 link true /test pull-br-integration-test

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@ti-chi-bot
Copy link
Member

@7yyo: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-br-integration-test c0a81f9 link true /test pull-br-integration-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/execution SIG execution sig/sql-infra SIG: SQL Infra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect behavior of NO_ZERO_DATE when altering table
5 participants