table: fix zero date in different sqlmode #20206

9547 · 2020-09-24T15:39:19Z

What problem does this PR solve?

Issue Number: close #19892, #19634

Problem Summary:

What is changed and how it works?

What's Changed:

How it Works:

Related changes

PR to update pingcap/docs/pingcap/docs-cn:

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)

Side effects

Release note

The same behavior of NO_ZERO_DATE and NO_ZERO_IN_DATE to MySQL's in INSERT/UPDATE statement

sre-bot · 2020-09-24T15:39:35Z

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

9547 · 2020-09-24T17:30:57Z

@qw4990 PTAL, and curious, why this test case failed, seems not related to my pr? If I've missed something, please let me know.

[2020-09-24T17:15:23.120Z] PASS: session_test.go:1573: testSessionSuite3.TestISColumns	0.005s

[2020-09-24T17:15:23.376Z] panic: runtime error: invalid memory address or nil pointer dereference

[2020-09-24T17:15:23.376Z] [signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x2acfdf2]

[2020-09-24T17:15:23.376Z] 

[2020-09-24T17:15:23.376Z] goroutine 5416 [running]:

[2020-09-24T17:15:23.376Z] github.com/pingcap/tidb/util/execdetails.(*BasicRuntimeStats).GetTime(...)

[2020-09-24T17:15:23.376Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/util/execdetails/execdetails.go:555

[2020-09-24T17:15:23.376Z] github.com/pingcap/tidb/executor.(*insertRuntimeStat).String(0xc01085d500, 0x3acaa40, 0xc0107b9920)

[2020-09-24T17:15:23.376Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/executor/insert_common.go:1072 +0x1d2

[2020-09-24T17:15:23.376Z] github.com/pingcap/tidb/util/execdetails.(*RootRuntimeStats).String(0xc00f769920, 0x1, 0xc00f769920)

[2020-09-24T17:15:23.376Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/util/execdetails/execdetails.go:530 +0x235

[2020-09-24T17:15:23.376Z] github.com/pingcap/tidb/planner/core.getRuntimeInfo(0x3b0f9e0, 0xc00ebc8780, 0x3af92a0, 0xc01066a180, 0xc010861260, 0xc010861230, 0x203004, 0x203004, 0x203004, 0xc010861170, ...)

[2020-09-24T17:15:23.376Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/planner/core/common_plans.go:1069 +0x38b

[2020-09-24T17:15:23.377Z] github.com/pingcap/tidb/planner/core.(*planEncoder).encodePlan(0xc00e2bcde0, 0x3af92a0, 0xc01066a180, 0x1, 0x0)

[2020-09-24T17:15:23.377Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/planner/core/encode.go:64 +0xaa

[2020-09-24T17:15:23.377Z] github.com/pingcap/tidb/planner/core.(*planEncoder).encodePlanTree(0xc00e2bcde0, 0x3af92a0, 0xc01066a180, 0x0, 0x2)

[2020-09-24T17:15:23.377Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/planner/core/encode.go:58 +0x84

[2020-09-24T17:15:23.377Z] github.com/pingcap/tidb/planner/core.EncodePlan(0x3af92a0, 0xc01066a180, 0x0, 0x0)

[2020-09-24T17:15:23.377Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/planner/core/encode.go:52 +0x123

[2020-09-24T17:15:23.377Z] github.com/pingcap/tidb/executor.getPlanTree(0x3af92a0, 0xc01066a180, 0x3af92a0, 0xc01066a180)

[2020-09-24T17:15:23.377Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/executor/adapter.go:955 +0x87

[2020-09-24T17:15:23.377Z] github.com/pingcap/tidb/executor.(*ExecStmt).LogSlowQuery(0xc00f589c30, 0x5d3047314c8000b, 0x3ac0000)

[2020-09-24T17:15:23.377Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/executor/adapter.go:899 +0x456

[2020-09-24T17:15:23.377Z] github.com/pingcap/tidb/executor.(*ExecStmt).FinishExecuteStmt(0xc00f589c30, 0x5d3047314c8000b, 0xc00ebc0000)

[2020-09-24T17:15:23.377Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/executor/adapter.go:810 +0xeb

[2020-09-24T17:15:23.377Z] github.com/pingcap/tidb/session.runStmt(0x3ac7f00, 0xc000062070, 0xc00ebc8780, 0x3ae6680, 0xc00f589c30, 0x0, 0x0, 0x3a6c260, 0xc01085d420)

[2020-09-24T17:15:23.377Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/session/session.go:1235 +0x446

[2020-09-24T17:15:23.377Z] github.com/pingcap/tidb/session.(*session).ExecuteStmt(0xc00ebc8780, 0x3ac7f00, 0xc000062070, 0x3ad1000, 0xc00f599a00, 0x0, 0x0, 0x0, 0x0)

[2020-09-24T17:15:23.377Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/session/session.go:1171 +0x7b9

[2020-09-24T17:15:23.377Z] github.com/pingcap/tidb/util/testkit.(*TestKit).Exec(0xc00f94bf88, 0x357dfcb, 0x24, 0x0, 0x0, 0x0, 0xc00f74e758, 0x12523b2, 0xc00e1df5d0, 0xc00f74e780)

[2020-09-24T17:15:23.377Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/util/testkit/testkit.go:160 +0x215

[2020-09-24T17:15:23.377Z] github.com/pingcap/tidb/session_test.(*testSessionSuite2).TestErrorRollback.func1(0xc00f6e94c0, 0xc00eb9cff0, 0xc0007fea80, 0x14)

[2020-09-24T17:15:23.377Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/session/session_test.go:300 +0x180

[2020-09-24T17:15:23.377Z] created by github.com/pingcap/tidb/session_test.(*testSessionSuite2).TestErrorRollback

[2020-09-24T17:15:23.377Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/session/session_test.go:295 +0x1d9

[2020-09-24T17:15:23.377Z] exit status 2

[2020-09-24T17:15:23.377Z] FAIL	github.com/pingcap/tidb/session	4.716s

rebelice · 2020-09-27T11:14:14Z

/run-check_dev_2

rebelice · 2020-09-27T11:34:12Z

expression/integration_test.go

+ tk.MustExec("TRUNCATE TABLE dd")
+ tk.MustExec("INSERT INTO dd(a) values('2000-10-01')")
+ tk.MustExec("UPDATE dd SET a = '0000-00-00'")
+ // tk.MustQuery("SHOW WARNINGS").Check(testkit.Rows("Warning 1292 Incorrect date value: '0000-00-00'"))


Could you tell me why comment it here？

Seems there was two identical warning messages , but I have no idea why that happened

maybe it's to check insert and update both generating a warning

But the insert stmt is valid, no warnings should generated

i mean line 7521 and 7527 should both be checked

thanks for the advice, and is there any way to clear warnings before an stmt executing.

Seems the warning messages are automatically deleted after a valid STMT executed, IMO, line 7521's warning will not affect this line.

seems our communication is misleading 🤣 i mean this line shouldn't be commented as it could check above update generated a warning. and maybe you say that it generated two identical warnings? that's an unexpected behaviour, maybe some bug in code

😂 sorry for the misunderstanding. In my local tests, the warning count maybe 1 or 2. currently, rebase from the master branch and remove the comment, the local test seems passed. Push it up to see the CI passed or not.

It's passed 🎉

table/column.go

lance6716

good work

lance6716 · 2020-10-12T02:03:41Z

PTAL @rebelice @lzmhhh123

table/column.go

expression/integration_test.go

9547 · 2020-10-17T19:32:46Z

@rebelice Sorry for the late reply, please have a look again

rebelice

LGTM

ti-srebot · 2020-10-23T06:34:40Z

@rebelice, Thanks for your review. The bot only counts LGTMs from Reviewers and higher roles, but you're still welcome to leave your comments. See the corresponding SIG page for more information. Related SIG: execution(slack).

rebelice · 2020-10-23T06:36:08Z

/run-all-tests

rebelice · 2020-10-23T06:42:00Z

/rebuild

lzmhhh123 · 2020-11-13T08:47:07Z

@ichn-hu Please help to review this PR.

ichn-hu

Rest LGTM

ichn-hu · 2020-11-13T10:32:23Z

table/column.go

+ }
+
+ innerErr := types.ErrWrongValue.GenWithStackByArgs(zeroT, val.GetString())
+ ignoreErr := sc.DupKeyAsWarning


Why use DupKeyAsWarning for ignore error? I think here it is not about dup key, but zero date?

Because the value of sc.DupKeyAsWarning is the original value of ignoreError: https://github.com/pingcap/tidb/blob/43a5bb47977fc7952a8aaedffe0154e6918bf4ad/executor/executor.go#L1601-L1615

lzmhhh123

LGTM

ghost · 2020-11-23T16:40:32Z

OK. In the worst case, we can add a rand DateTime for each inserted row.

That was my initial plan, but the zero date is used on a table which uses this to signify a deleted_at date:

INSERT INTO `addresses` (`address1`,`address2`,`post`,`created_at`,`updated_at`,`deleted_at`) VALUES (?,?,?,?,?,?) [arguments: (Billing Address pluck_user3, , , 2020-11-19 20:30:17.882469, 2020-11-19 20:30:17.882469, 0000-00-00)];

I would have to create a special valid date to replace this, which is not ideal. In anycase, the gorm test is now passing with an expicit sql mode.

ghost · 2020-11-23T16:41:22Z

/run-common-test tidb-test=pr/1109
/run-integration-common-test tidb-test=pr/1109

ghost · 2020-11-23T17:24:06Z

/run-common-test tidb-test=pr/1109
/run-integration-common-test tidb-test=pr/1109

ghost · 2020-11-23T17:37:00Z

/run-common-test tidb-test=pr/1109

ghost · 2020-11-23T18:58:56Z

/run-common-test tidb-test=pr/1109

ghost · 2020-11-23T19:03:16Z

There were more broken tests than I first noticed:

Gorm test (fixed in pr/1109)
JDBC test (fixed in pr/1109)
mysql_test (fixed)

9547 · 2020-11-23T23:33:20Z

@nullnotnil Is there anything I need to fix first, or I can help?

ghost · 2020-11-23T23:44:53Z

@nullnotnil Is there anything I need to fix first, or I can help?

It's okay. I took another look at it, and I think I will fix the mysql_test cases that break and close pingcap/mysql-tester#9 . It doesn't look as bad as I thought - give me a few hours :-)

ghost · 2020-11-23T23:47:26Z

I don't think that we should cherry-pick this into the release branch (v4.0). Given that the regression test suite is breaking in a few places, there are likely apps that are depending on this without knowing it. It is better to have them discover the issue when upgrading to the next major release.

ghost · 2020-11-24T00:50:14Z

/run-common-test tidb-test=pr/1109
/run-integration-common-test tidb-test=pr/1109

lzmhhh123 · 2020-11-24T03:33:34Z

I don't think that we should cherry-pick this into the release branch (v4.0). Given that the regression test suite is breaking in a few places, there are likely apps that are depending on this without knowing it. It is better to have them discover the issue when upgrading to the next major release.

OK, I agree.

lzmhhh123 · 2020-11-24T03:35:49Z

/merge

ti-srebot · 2020-11-24T03:35:56Z

Your auto merge job has been accepted, waiting for:

21182
21124

ti-srebot · 2020-11-24T03:57:24Z

/run-all-tests

ti-srebot · 2020-11-24T04:08:22Z

@9547 merge failed.

ichn-hu · 2020-11-24T04:23:20Z

/run-unit-tests

wjhuang2016 · 2020-12-08T07:11:24Z

/run-cherry-picker

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2020-12-08T07:11:52Z

cherry pick to release-4.0 in PR #21553

9547 requested a review from a team as a code owner September 24, 2020 15:39

9547 requested review from lzmhhh123 and removed request for a team September 24, 2020 15:39

ti-srebot added the contribution This PR is from a community contributor. label Sep 24, 2020

github-actions bot added the component/expression label Sep 24, 2020

9547 force-pushed the fix/no-zero-date-sqlmode branch from 63e73dd to fb22ff9 Compare September 24, 2020 17:14

rebelice reviewed Sep 27, 2020

View reviewed changes

9547 force-pushed the fix/no-zero-date-sqlmode branch from 22de6a4 to 02cefbf Compare October 9, 2020 22:42

lance6716 reviewed Oct 11, 2020

View reviewed changes

rebelice reviewed Oct 12, 2020

View reviewed changes

table/column.go Outdated Show resolved Hide resolved

table/column.go Outdated Show resolved Hide resolved

expression/integration_test.go Outdated Show resolved Hide resolved

9547 force-pushed the fix/no-zero-date-sqlmode branch 2 times, most recently from a300651 to 54158e2 Compare October 17, 2020 19:15

github-actions bot added the sig/execution SIG execution label Oct 17, 2020

rebelice reviewed Oct 23, 2020

View reviewed changes

ichn-hu mentioned this pull request Nov 3, 2020

Welcome to contribute #20804

Closed

9547 force-pushed the fix/no-zero-date-sqlmode branch from c731b04 to eaec4f8 Compare November 4, 2020 13:29

9547 requested a review from lance6716 November 4, 2020 13:48

lzmhhh123 requested a review from ichn-hu November 13, 2020 08:46

ichn-hu reviewed Nov 13, 2020

View reviewed changes

lzmhhh123 reviewed Nov 17, 2020

View reviewed changes

ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 17, 2020

ghost mentioned this pull request Nov 23, 2020

Add workaround for sql_mode NO_ZERO_DATE pingcap/mysql-tester#9

Closed

lzmhhh123 approved these changes Nov 24, 2020

View reviewed changes

Merge branch 'master' into fix/no-zero-date-sqlmode

18e2cb9

lzmhhh123 merged commit afb2ab9 into pingcap:master Nov 24, 2020

morgo mentioned this pull request Dec 4, 2020

Incorrect behavior of NO_ZERO_DATE when altering table #11952

Closed

wjhuang2016 added the needs-cherry-pick-4.0 label Dec 8, 2020

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Dec 8, 2020

cherry pick pingcap#20206 to release-4.0

3721f58

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Dec 8, 2020

table: fix zero date in different sqlmode (#20206) #21553

Merged

This was referenced Dec 8, 2020

Parsing datetime when insert is not compatible with MySQL #9762

Closed

bugs in DATETIME related session mode: NO_ZERO_IN_DATE #12571

Closed

morgo mentioned this pull request Dec 17, 2020

mysql-compatibility: update ZERO_DATE docs pingcap/docs#4436

Merged

10 tasks

qw4990 pushed a commit that referenced this pull request Jan 29, 2021

table: fix zero date in different sqlmode (#20206) (#21553)

9a904e1

table: fix zero date in different sqlmode #20206

table: fix zero date in different sqlmode #20206

Conversation

9547 commented Sep 24, 2020 • edited by ghost Loading

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

sre-bot commented Sep 24, 2020

9547 commented Sep 24, 2020

rebelice commented Sep 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 left a comment

Choose a reason for hiding this comment

lance6716 commented Oct 12, 2020

9547 commented Oct 17, 2020

rebelice left a comment

Choose a reason for hiding this comment

ti-srebot commented Oct 23, 2020

rebelice commented Oct 23, 2020

rebelice commented Oct 23, 2020

lzmhhh123 commented Nov 13, 2020

ichn-hu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzmhhh123 left a comment

Choose a reason for hiding this comment

ghost commented Nov 23, 2020

ghost commented Nov 23, 2020

ghost commented Nov 23, 2020

ghost commented Nov 23, 2020

ghost commented Nov 23, 2020

ghost commented Nov 23, 2020 • edited by ghost Loading

9547 commented Nov 23, 2020

ghost commented Nov 23, 2020

ghost commented Nov 23, 2020

ghost commented Nov 24, 2020

lzmhhh123 commented Nov 24, 2020

lzmhhh123 commented Nov 24, 2020

ti-srebot commented Nov 24, 2020

ti-srebot commented Nov 24, 2020

ti-srebot commented Nov 24, 2020

ichn-hu commented Nov 24, 2020

wjhuang2016 commented Dec 8, 2020

ti-srebot commented Dec 8, 2020

9547 commented Sep 24, 2020 •

edited by ghost

Loading

ghost commented Nov 23, 2020 •

edited by ghost

Loading