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

Truncate error shouldn't be allowed when handling default values #49141

Open
YangKeao opened this issue Dec 4, 2023 · 2 comments
Open

Truncate error shouldn't be allowed when handling default values #49141

YangKeao opened this issue Dec 4, 2023 · 2 comments
Labels
severity/moderate sig/sql-infra SIG: SQL Infra type/bug The issue is confirmed as a bug.

Comments

@YangKeao
Copy link
Member

YangKeao commented Dec 4, 2023

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

set sql_mode='';
create table t (d int default '123121241241242515521525215525' );

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

ERROR 1067 (42000): Invalid default value for 'd'

3. What did you see instead (Required)

Query OK, 0 rows affected, 1 warning (0.32 sec)

4. What is your TiDB version? (Required)

@YangKeao YangKeao added type/bug The issue is confirmed as a bug. severity/moderate labels Dec 4, 2023
@YangKeao
Copy link
Member Author

YangKeao commented Dec 4, 2023

Actually, I think OverflowAsWarning can be removed, and replaced with TruncatedAsWarning (which is not set in the typectx, and will be moved into errctx in the future). Here is a proof that OverflowAsWarning and TruncatedAsWarning (and corresponding flags in typectx) is the same:

OverflowAsWarning <=> TruncatedAsWarning

Analyzing the codes, I found that every time the OverflowAsWarning is set to true, the WithTruncateAsWarning is also called, so we can make sure OverflowAsWarning => TruncatedAsWarning

On the other side, there are some places when TruncatedAsWarning is set to true but OverflowAsWarning is left as false:

  1. INSERT, CREATE TABLE, ALTER TABLE, DELETE and UPDATE statement. Only WithTruncateAsWarning is set according to SQL_MODE and IGNORE, but OverflowAsWarning is left as false.
  2. Around buildPartitionDefinitionsInfo, only WithTruncateAsWarning is set to false temporarily.
  3. setNonRestrictiveFlags in LOAD DATA worker.

Let's see whether these differences are expected 😆

  1. Let's discuss this issue for each statements.
    1. INSERT statement should also ignore overflow error, as shown in INSERT should also ignore ErrOverflow if the SQL_MODE is not strict #49137.
    2. CREATE TABLE and ALTER TABLE. It will set TruncatedAsWarning according to SQL_MODE after ddl: Allow create timestamp default 0 without sql_mode NO_ZERO_DATE (#30305) #30507. However, the default value check shouldn't follow TruncatedAsWarning (I recorded this issue in Truncate error shouldn't be allowed when handling default values #49141). And the problem in ddl: Allow create timestamp default 0 without sql_mode NO_ZERO_DATE (#30305) #30507 has been fixed by another flag (IgnoreZeroDateErr), so it's not related with TruncatedAsWarning now.
    3. DELETE and UPDATE. It will set TruncatedAsWarning according to SQL_MODE, but OverflowAsWarning is not set. I think it's also a bug. I recorded it in ErrOverflow should be allowed in DELETE and UPDATE statement if the sql_mode is not strict #49143 🤦.
  2. Around calling buildPartitionDefinitionsInfo. The WithTruncateAsWarning is set to false for ALTER TABLE case (in GetModifiableColumnJob). This behavior is modified in ddl: restricting MODIFY COLUMN on partitioning columns. #38670. Now, modifying column on partition table is forbidden (*: revert 38302, forbid modify column on partition table #39991), so it's fine to not consider the OverflowAsWarning here.
  3. As for setNonRestrictiveFlags in LOAD DATA worker, it uses the CastValue. We'll show that HandleOverflow in CastValue is meaningless and all errors (if not handled explicitly) are already handled according to TruncateAsWarning in the next chapter. Therefore, it's also fine to not consider OverflowAsWarning in this function.

Given these deduction, I can tell the OverflowAsWarning can be safely replaced with TruncatedAsWarning (and a lot of bugs will be fixed by this modification).

Replace HandleOverflow with HandleTruncate is safe

Before every call to HandleOverflow, we have checked the types of the error is ErrOverflow, so it meets the filter in HandleTruncate. The only negative case is in CastValue, which has a complicated error handling logic. It uses both HandleTruncate and HandleOverflow:

err = sc.HandleTruncate(err)
err = sc.HandleOverflow(err, err)

It's meaningless, because if OverflowAsWarning is true, TruncateAsWarning is also true, so that the error turns to nil in the first HandleTruncate. Remove the HandleOverflow shouldn't affect any behavior.

Why did OverflowAsWarning exist?

OverflowAsWarning seems to be redundant as shown above, then a natual question comes into my heart: "Why did OverflowAsWarning ever exist?". Let me try to show the history of this field, so that we can know when this problem appeared:

  1. expression: fix #3762, signed integer overflow handle in minus unary scalar function #3780 introduces a field IgnoreOverflow. It's used to change the behavior of typeInfer. With this field set, an overflow integer constant may be infered as a decimal.
  2. expression, parser: fix issue #3691, cast compatibility #3894 IgnoreOverflow is renamed to OverflowAsWarning. The usage introduced in expression: fix #3762, signed integer overflow handle in minus unary scalar function #3780 is removed, as the typeInfer starts to depend on InSelectStmt instead of IgnoreOverflow. However, it sets the OverflowAsWarning just the same value with the original IgnoreOverflow, though they have different meanings.

The comments in this PR didn't give any clue why OverflowAsWarning is different with TruncateAsWarning.

@lcwangchao
Copy link
Collaborator

A dup issue before #47870

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
severity/moderate sig/sql-infra SIG: SQL Infra type/bug The issue is confirmed as a bug.
Projects
None yet
Development

No branches or pull requests

3 participants