Skip to content

Conversation

ti-chi-bot
Copy link
Member

This is an automated cherry-pick of #61476

What problem does this PR solve?

Issue Number: close #61350

Problem Summary:

The length returned by TiDB is not compatible with MySQL. Sometimes it's too small so that the client may fail to allocate big enough buffer and cause panic/security issue. This PR modifies the logic of calculating the length, and make it compatible with MySQL for most of the cases.

What changed and how does it work?

  1. Change the logic of calculating length for casting many types to string.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

Run the SQLs provided in #61350 and check the results are the same.

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

Please refer to Release Notes Language Style Guide to write a quality release note.

Fix the issue that the length returned by `cast` function is not compatible with MySQL.

@ti-chi-bot ti-chi-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/planner SIG: Planner size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. type/cherry-pick-for-release-8.5 This PR is cherry-picked to release-8.5 from a source PR. labels Jul 9, 2025
@ti-chi-bot
Copy link
Member Author

@YangKeao This PR has conflicts, I have hold it.
Please resolve them or ask others to resolve them, then comment /unhold to remove the hold label.

Copy link

codecov bot commented Jul 9, 2025

Codecov Report

Attention: Patch coverage is 89.47368% with 18 lines in your changes missing coverage. Please review.

Please upload report for BASE (release-8.5@f1df099). Learn more about missing BASE report.

Additional details and impacted files
@@               Coverage Diff                @@
##             release-8.5     #62330   +/-   ##
================================================
  Coverage               ?   57.1980%           
================================================
  Files                  ?       1777           
  Lines                  ?     631495           
  Branches               ?          0           
================================================
  Hits                   ?     361203           
  Misses                 ?     246179           
  Partials               ?      24113           
Flag Coverage Δ
integration 37.2054% <69.0058%> (?)
unit 72.8490% <88.3040%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 52.9278% <0.0000%> (?)
parser ∅ <0.0000%> (?)
br 52.7291% <0.0000%> (?)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ti-chi-bot ti-chi-bot bot added cherry-pick-approved Cherry pick PR approved by release team. and removed do-not-merge/cherry-pick-not-approved labels Jul 9, 2025
@YangKeao
Copy link
Member

/unhold

@ti-chi-bot ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2025
@YangKeao YangKeao changed the title expression: fix the length of casting from INT/REAL/DECIMAL/.... to string | tidb-test=pr/2549 (#61476) expression: fix the length of casting from INT/REAL/DECIMAL/.... to string | tidb-test=pr/2554 Jul 10, 2025
Signed-off-by: Yang Keao <[email protected]>
@ti-chi-bot ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jul 10, 2025
Copy link
Contributor

@windtalker windtalker 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 bot added approved lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jul 10, 2025
Copy link

ti-chi-bot bot commented Jul 10, 2025

[LGTM Timeline notifier]

Timeline:

  • 2025-07-10 03:21:26.651549495 +0000 UTC m=+2142739.374728475: ☑️ agreed by AilinKid.
  • 2025-07-10 03:30:41.224874549 +0000 UTC m=+2143293.948053530: ☑️ agreed by windtalker.

@YangKeao
Copy link
Member

/retest

1 similar comment
@YangKeao
Copy link
Member

/retest

@YangKeao
Copy link
Member

/hold

@ti-chi-bot ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2025
@YangKeao
Copy link
Member

There is a bug reported by TiFlash test:

TiDB [email protected]:test> create table test.t(a datetime, b date, c timestamp(3))
TiDB [email protected]:test> alter table test.t set tiflash replica 1
TiDB [email protected]:test> insert into test.t values('2021/04/13 00:34:00.123456', '2021/04/13', '2021/04/13 00:34:00.123456')
-- tidb-server throw error, 3c2dc46853f5a2edb89da78c1a678da9725ed7b8
TiDB [email protected]:test> select count(*), cast(a as char), cast(b as char), cast(c as char) from test.t group by cast(a as char), cast(b as char), cast(c as char);
(1055, "Expression #2 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'test.t.a' which is not functionally dependent on columns in GROUP BY clause; this
is incompatible with sql_mode=only_full_group_by")
TiDB [email protected]:test> select tidb_version() \G;
***************************[ 1. row ]***************************
tidb_version() | Release Version: v9.0.0-beta.2.pre-84-g3c2dc46853
Edition: Community
Git Commit Hash: 3c2dc46853f5a2edb89da78c1a678da9725ed7b8
Git Branch: master
UTC Build Time: 2025-07-10 07:12:11
GoVersion: go1.23.10
Race Enabled: false
Check Table Before Drop: false
Store: tikv
Kernel Type: Classic
-- tidb-server is ok, fe8d82fedd8f9a638d1e0d88472a320dd4f2b392 is OK
TiDB [email protected]:test> select count(*), cast(a as char), cast(b as char), cast(c as char) from test.t group by cast(a as char), cast(b as char), cast(c as char);
+----------+---------------------+-----------------+-------------------------+
| count(*) | cast(a as char)     | cast(b as char) | cast(c as char)         |
+----------+---------------------+-----------------+-------------------------+
| 1        | 2021-04-13 00:34:00 | 2021-04-13      | 2021-04-13 00:34:00.123 |
+----------+---------------------+-----------------+-------------------------+
TiDB [email protected]:test> select tidb_version() \G;
***************************[ 1. row ]***************************
tidb_version() | Release Version: v9.0.0-beta.2.pre-83-gfe8d82fedd
Edition: Community
Git Commit Hash: fe8d82fedd8f9a638d1e0d88472a320dd4f2b392
Git Branch: master
UTC Build Time: 2025-07-10 07:16:14
GoVersion: go1.23.10
Race Enabled: false
Check Table Before Drop: false
Store: tikv
Kernel Type: Classic
1 row in set
Time: 0.001s

@YangKeao
Copy link
Member

There is a bug reported by TiFlash test:

TiDB [email protected]:test> create table test.t(a datetime, b date, c timestamp(3))
TiDB [email protected]:test> alter table test.t set tiflash replica 1
TiDB [email protected]:test> insert into test.t values('2021/04/13 00:34:00.123456', '2021/04/13', '2021/04/13 00:34:00.123456')
-- tidb-server throw error, 3c2dc46853f5a2edb89da78c1a678da9725ed7b8
TiDB [email protected]:test> select count(*), cast(a as char), cast(b as char), cast(c as char) from test.t group by cast(a as char), cast(b as char), cast(c as char);
(1055, "Expression #2 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'test.t.a' which is not functionally dependent on columns in GROUP BY clause; this
is incompatible with sql_mode=only_full_group_by")
TiDB [email protected]:test> select tidb_version() \G;
***************************[ 1. row ]***************************
tidb_version() | Release Version: v9.0.0-beta.2.pre-84-g3c2dc46853
Edition: Community
Git Commit Hash: 3c2dc46853f5a2edb89da78c1a678da9725ed7b8
Git Branch: master
UTC Build Time: 2025-07-10 07:12:11
GoVersion: go1.23.10
Race Enabled: false
Check Table Before Drop: false
Store: tikv
Kernel Type: Classic
-- tidb-server is ok, fe8d82fedd8f9a638d1e0d88472a320dd4f2b392 is OK
TiDB [email protected]:test> select count(*), cast(a as char), cast(b as char), cast(c as char) from test.t group by cast(a as char), cast(b as char), cast(c as char);
+----------+---------------------+-----------------+-------------------------+
| count(*) | cast(a as char)     | cast(b as char) | cast(c as char)         |
+----------+---------------------+-----------------+-------------------------+
| 1        | 2021-04-13 00:34:00 | 2021-04-13      | 2021-04-13 00:34:00.123 |
+----------+---------------------+-----------------+-------------------------+
TiDB [email protected]:test> select tidb_version() \G;
***************************[ 1. row ]***************************
tidb_version() | Release Version: v9.0.0-beta.2.pre-83-gfe8d82fedd
Edition: Community
Git Commit Hash: fe8d82fedd8f9a638d1e0d88472a320dd4f2b392
Git Branch: master
UTC Build Time: 2025-07-10 07:16:14
GoVersion: go1.23.10
Race Enabled: false
Check Table Before Drop: false
Store: tikv
Kernel Type: Classic
1 row in set
Time: 0.001s

It's not related to TiFlash.

@YangKeao
Copy link
Member

Ref #62354

@YangKeao
Copy link
Member

I'll also pick #62354 to this PR.

Signed-off-by: Yang Keao <[email protected]>
@ti-chi-bot ti-chi-bot bot removed the approved label Jul 14, 2025
@YangKeao
Copy link
Member

/retest

@YangKeao YangKeao requested a review from tangenta July 14, 2025 05:29
@YangKeao
Copy link
Member

#62354 has been merged.

/unhold

@ti-chi-bot ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 14, 2025
Copy link

ti-chi-bot bot commented Jul 14, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AilinKid, tangenta, windtalker

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added the approved label Jul 14, 2025
@YangKeao
Copy link
Member

/retest

@ti-chi-bot ti-chi-bot bot merged commit 6b45e27 into pingcap:release-8.5 Jul 14, 2025
14 of 18 checks passed
@JaySon-Huang JaySon-Huang deleted the cherry-pick-61476-to-release-8.5 branch July 14, 2025 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved cherry-pick-approved Cherry pick PR approved by release team. lgtm release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/planner SIG: Planner size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. type/cherry-pick-for-release-8.5 This PR is cherry-picked to release-8.5 from a source PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants