ddl: Fix compatibility with null expression index by JaySon-Huang · Pull Request #10162 · pingcap/tiflash

JaySon-Huang · 2025-05-14T04:23:38Z

What problem does this PR solve?

Issue Number: close #9891

Problem Summary:

CREATE TABLE `t` (
  `id` int,
  KEY `idx_name` ((null))
);
alter table t set tiflash replica 1;
insert t values(0);

If the user create an expression index as above, TiDB will generate a "column" for the expression with "Tp": 6. This type of column will generate ColumnNothing which is not expected to be created at the IStorage, thus causing exception thrown during the DDL syncing when TiFlash handling the RaftLog from the table.
Because TiFlash-proxy has persisted the RaftLog but get exception when decoding the RaftLog and apply it to the Storage layer, TiFlash panic.

tiflash/dbms/src/TiDB/Schema/TiDBTypes.h

Line 33 in 5c59a51

M(Null, 6, Nil, Nothing) \

tiflash/dbms/src/Storages/StorageFactory.cpp

Lines 39 to 47 in 5c59a51

    
           static void checkAllTypesAreAllowedInTable(const NamesAndTypesList & names_and_types) 
        
           { 
        
               for (const auto & elem : names_and_types) 
        
                   if (elem.type->cannotBeStoredInTables()) 
        
                       throw Exception( 
        
                           ErrorCodes::DATA_TYPE_CANNOT_BE_USED_IN_TABLES, 
        
                           "Data type {} cannot be used in tables", 
        
                           elem.type->getName()); 
        
           }

What is changed and how it works?

ddl: Fix TiFlash panic when meets expression index with format `((NULL))`
Using `ColumnNothing` in IStorage may bring unexpected behavior when we try to write or read the column. So TiFlash create `ColumnInt8` instead of `ColumnNothing` when `TiDB::ColumnInfo.Tp == Null`.

However, we don't expected the column is used when reading or writing to TiFlash.

* `((null))` is not a normal expression index, this could be created by the user in accident (e.g. executing wrong SQL statement to create the expression index), TiDB should not write or read from it
* Even for normal expression index, TiDB does not use expression when accessing to TiFlash

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

Release note

Fix the issue that TiFlash panic when meets expression index with format `((NULL))`

Signed-off-by: JaySon-Huang <tshent@qq.com>

CalvinNeo

lgtm

ti-chi-bot · 2025-05-15T05:28:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CalvinNeo, JinheLin

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [CalvinNeo,JinheLin]

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 · 2025-05-15T05:28:13Z

[LGTM Timeline notifier]

Timeline:

2025-05-15 05:26:57.942507334 +0000 UTC m=+3984.198404723: ☑️ agreed by JinheLin.
2025-05-15 05:28:12.626216084 +0000 UTC m=+4058.882113480: ☑️ agreed by CalvinNeo.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2025-05-16T02:27:28Z

In response to a cherrypick label: new pull request created to branch release-8.1: #10173.
But this PR has conflicts, please resolve them!

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2025-05-16T02:27:31Z

In response to a cherrypick label: new pull request created to branch release-7.5: #10174.
But this PR has conflicts, please resolve them!

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2025-05-20T02:46:16Z

In response to a cherrypick label: new pull request created to branch release-8.5: #10187.
But this PR has conflicts, please resolve them!

close #9891 ddl: Fix TiFlash panic when meets expression index with format `((NULL))` Using `ColumnNothing` in IStorage may bring unexpected behavior when we try to write or read the column. So TiFlash create `ColumnInt8` instead of `ColumnNothing` when `TiDB::ColumnInfo.Tp == Null`. However, we don't expected the column is used when reading or writing to TiFlash. * `((null))` is not a normal expression index, this could be created by the user in accident (e.g. executing wrong SQL statement to create the expression index), TiDB should not write or read from it * Even for normal expression index, TiDB does not use expression when accessing to TiFlash Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io> Signed-off-by: JaySon-Huang <tshent@qq.com> Co-authored-by: JaySon <tshent@qq.com> Co-authored-by: JaySon-Huang <tshent@qq.com>

Fix null expression index

0cb72dc

Signed-off-by: JaySon-Huang <tshent@qq.com>

JaySon-Huang added 3 commits May 14, 2025 22:19

Add fullstack tests

8300c72

Signed-off-by: JaySon-Huang <tshent@qq.com>

Fix alter table

e88377b

Signed-off-by: JaySon-Huang <tshent@qq.com>

Add integrate test for create expression index

61bc500

Signed-off-by: JaySon-Huang <tshent@qq.com>

JaySon-Huang force-pushed the fix_null_expression_index branch from 2e8f5d5 to 61bc500 Compare May 14, 2025 14:38

JaySon-Huang changed the title ~~[WIP] ddl: Fix null expression index~~ ddl: Fix compatibility with null expression index May 14, 2025

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 14, 2025

JaySon-Huang requested review from CalvinNeo, JinheLin and Lloyd-Pottiger May 14, 2025 14:42

JinheLin approved these changes May 15, 2025

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels May 15, 2025

CalvinNeo approved these changes May 15, 2025

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels May 15, 2025

ti-chi-bot bot merged commit 8e3d547 into pingcap:master May 15, 2025
5 checks passed

JaySon-Huang deleted the fix_null_expression_index branch May 15, 2025 06:25

ti-chi-bot bot added needs-cherry-pick-release-8.1 Should cherry pick this PR to release-8.1 branch. needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. labels May 16, 2025

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request May 16, 2025

This is an automated cherry-pick of pingcap#10162

382510e

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request May 16, 2025

ddl: Fix compatibility with null expression index (#10162) #10173

Merged

12 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request May 16, 2025

This is an automated cherry-pick of pingcap#10162

3749ff4

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request May 16, 2025

ddl: Fix compatibility with null expression index (#10162) #10174

Merged

12 tasks

ti-chi-bot bot added the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label May 20, 2025

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request May 20, 2025

This is an automated cherry-pick of pingcap#10162

a3f790d

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request May 20, 2025

ddl: Fix compatibility with null expression index (#10162) #10187

Merged

12 tasks

ti-chi-bot bot removed the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label Jun 12, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: Fix compatibility with null expression index#10162

ddl: Fix compatibility with null expression index#10162
ti-chi-bot[bot] merged 4 commits intopingcap:masterfrom
JaySon-Huang:fix_null_expression_index

JaySon-Huang commented May 14, 2025 •

edited

Loading

Uh oh!

CalvinNeo left a comment

Uh oh!

ti-chi-bot bot commented May 15, 2025

Uh oh!

ti-chi-bot bot commented May 15, 2025

Uh oh!

Uh oh!

ti-chi-bot commented May 16, 2025

Uh oh!

ti-chi-bot commented May 16, 2025

Uh oh!

ti-chi-bot commented May 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	static void checkAllTypesAreAllowedInTable(const NamesAndTypesList & names_and_types)
	{
	for (const auto & elem : names_and_types)
	if (elem.type->cannotBeStoredInTables())
	throw Exception(
	ErrorCodes::DATA_TYPE_CANNOT_BE_USED_IN_TABLES,
	"Data type {} cannot be used in tables",
	elem.type->getName());
	}

Conversation

JaySon-Huang commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

CalvinNeo left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented May 15, 2025

Uh oh!

ti-chi-bot bot commented May 15, 2025

[LGTM Timeline notifier]

Uh oh!

Uh oh!

ti-chi-bot commented May 16, 2025

Uh oh!

ti-chi-bot commented May 16, 2025

Uh oh!

ti-chi-bot commented May 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JaySon-Huang commented May 14, 2025 •

edited

Loading