Skip to content

*: insert of invalid timestamp succeeded#26584

Merged
ti-chi-bot merged 12 commits intopingcap:masterfrom
mjonss:issue-25999
Aug 5, 2021
Merged

*: insert of invalid timestamp succeeded#26584
ti-chi-bot merged 12 commits intopingcap:masterfrom
mjonss:issue-25999

Conversation

@mjonss
Copy link
Contributor

@mjonss mjonss commented Jul 26, 2021 •
edited
Loading

What problem does this PR solve?

Issue Number: close #25999

Problem Summary:
Insert succeeded with values out-of-range for Timestamp

What is changed and how it works?

Never accept timestamp values out-of-range

What's Changed:
Check for values out-of-range

How it Works:
Now truncates and returns a warning or returns an error depending on sql_mode.

Check List

Tests

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

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note 

Timestamp values out-of-range can no longer be inserted.

mjonss added 3 commits July 20, 2021 14:34
@mjonss
types: do not allow invalid TIMESTAMP values.
ba1281a
@mjonss
Fixed similar issue as for tidb/pull/24051 due to added check.
975c040 
Timestamp value was no longer valid, but instead of flagging as
error internally in the lookup join, we skip it, since there
cannot be any match.

Also removed the debug build option in Makefile.common
@mjonss
Added insert test for issue-25999 and fixed the warning text
4fbfd13
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jul 26, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • bb7133
  • zimulala

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 26, 2021
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2021
@ti-chi-bot ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 26, 2021
@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Jul 26, 2021
@mjonss
Copy link
Contributor Author

mjonss commented Jul 27, 2021

/cc @tangenta @zimulala Could you please review this PR, since I think it touches code you worked with before?

zimulala
zimulala reviewed Jul 28, 2021
View reviewed changes
zimulala
zimulala reviewed Jul 29, 2021
View reviewed changes
zimulala
zimulala approved these changes Jul 30, 2021
View reviewed changes
Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 30, 2021
@mjonss
Copy link
Contributor Author

mjonss commented Jul 30, 2021

@tangenta Could you please take a look/review?

@mjonss
Copy link
Contributor Author

mjonss commented Jul 30, 2021

/cc @tangenta

@ti-chi-bot ti-chi-bot requested a review from tangenta July 30, 2021 12:46
@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 30, 2021
@bb7133
Copy link
Member

bb7133 commented Aug 3, 2021

/bench

@mjonss
Copy link
Contributor Author

mjonss commented Aug 5, 2021

/merge

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 5, 2021

@mjonss: /merge is only allowed for the committers, you can assign this pull request to the committer in list by filling /assign @committer in the comment to help merge this pull request.

Details

In response to this:

/merge

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 ti-community-infra/tichi repository.

@bb7133
Copy link
Member

bb7133 commented Aug 5, 2021

/merge

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 5, 2021

This pull request has been accepted and is ready to merge.

DetailsCommit hash: 00859df

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 5, 2021
@purelind
Copy link
Contributor

purelind commented Aug 5, 2021

/run-build

@ti-chi-bot ti-chi-bot merged commit 3cda7d0 into pingcap:master Aug 5, 2021
@mjonss mjonss deleted the issue-25999 branch August 5, 2021 10:38
@Yisaer Yisaer mentioned this pull request Aug 17, 2021
Merged
1 task
mjonss added a commit to mjonss/tidb that referenced this pull request Nov 1, 2021
@mjonss
*: query failed after add index / timestamp out-of-range (pingcap#28424)
87f250f 
Two issues, convertToMysqlTimestamp did set out-of-range timestamp to
Zero, when it would still be a storable/comparable KindMysqlTime.
(we only artificially limit the range of Timestamp to be compatible
with MySQL).

When this change was done,
it broke pingcap#26584 (insert of invalid timestamp succeeded),
which needed a different change in handleZeroDatetime
@mjonss mjonss mentioned this pull request Nov 1, 2021
Merged
12 tasks
This was referenced Dec 23, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bb7133 bb7133 bb7133 approved these changes

@zimulala zimulala zimulala approved these changes

@tangenta tangenta tangenta approved these changes

Assignees

No one assigned

Labels

component/expression release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/execution SIG execution sig/sql-infra SIG: SQL Infra size/M Denotes a PR that changes 30-99 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

execute insert statement successfully with invaild timestamp

6 participants

@mjonss @ti-chi-bot @bb7133 @purelind @zimulala @tangenta