Skip to content

Conversation

JaySon-Huang
Copy link
Contributor

@JaySon-Huang JaySon-Huang commented May 14, 2025

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.

M(Null, 6, Nil, Nothing) \

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

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

Release note

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

Signed-off-by: JaySon-Huang <[email protected]>
@ti-chi-bot ti-chi-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 14, 2025
Signed-off-by: JaySon-Huang <[email protected]>
Signed-off-by: JaySon-Huang <[email protected]>
@JaySon-Huang JaySon-Huang force-pushed the fix_null_expression_index branch from 2e8f5d5 to 61bc500 Compare May 14, 2025 14:38
@JaySon-Huang JaySon-Huang changed the title [WIP] ddl: Fix null expression index ddl: Fix compatibility with null expression index May 14, 2025
@ti-chi-bot 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
@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels May 15, 2025
Copy link
Member

@CalvinNeo CalvinNeo left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

ti-chi-bot bot commented May 15, 2025

[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

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 lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels May 15, 2025
Copy link
Contributor

ti-chi-bot bot commented May 15, 2025

[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.

@ti-chi-bot ti-chi-bot bot merged commit 8e3d547 into pingcap:master May 15, 2025
5 checks passed
@JaySon-Huang JaySon-Huang deleted the fix_null_expression_index branch May 15, 2025 06:25
@ti-chi-bot 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
@ti-chi-bot
Copy link
Member

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

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

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

@ti-chi-bot 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
@ti-chi-bot
Copy link
Member

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

ti-chi-bot bot pushed a commit that referenced this pull request May 20, 2025
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 <[email protected]>
Signed-off-by: JaySon-Huang <[email protected]>

Co-authored-by: JaySon <[email protected]>
Co-authored-by: JaySon-Huang <[email protected]>
ti-chi-bot bot pushed a commit that referenced this pull request May 20, 2025
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 <[email protected]>
Signed-off-by: JaySon-Huang <[email protected]>

Co-authored-by: JaySon <[email protected]>
Co-authored-by: JaySon-Huang <[email protected]>
@ti-chi-bot 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
ti-chi-bot bot pushed a commit that referenced this pull request Jun 13, 2025
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 <[email protected]>
Signed-off-by: JaySon-Huang <[email protected]>

Co-authored-by: JaySon <[email protected]>
Co-authored-by: JaySon-Huang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. needs-cherry-pick-release-8.1 Should cherry pick this PR to release-8.1 branch. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TiFlash panics with Data type Nullable(Nothing) cannot be used in tables while syncing expression index schema ((null))
4 participants