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

non-DXF ADD INDEX of large table (trigger import due to disk quota) will write KV with TS = 0 #57980

Closed
lance6716 opened this issue Dec 4, 2024 · 3 comments · Fixed by #57998
Labels
affects-8.1 This bug affects the 8.1.x(LTS) versions. affects-8.5 This bug affects the 8.5.x(LTS) versions. component/ddl This issue is related to DDL of TiDB. impact/crash crash/fatal report/customer Customers have encountered this bug. severity/critical type/bug The issue is confirmed as a bug.

Comments

@lance6716
Copy link
Contributor

lance6716 commented Dec 4, 2024

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

use this diff to trigger disk quota import on small tables

diff --git a/pkg/ddl/ingest/backend.go b/pkg/ddl/ingest/backend.go
index f11849f70b..cd08ec14c5 100644
--- a/pkg/ddl/ingest/backend.go
+++ b/pkg/ddl/ingest/backend.go
@@ -187,6 +187,8 @@ func (bc *litBackendCtx) Flush(indexID int64, mode FlushMode) (flushed, imported
        }

        shouldFlush, shouldImport := bc.checkFlush(mode)
+       shouldFlush = true
+       shouldImport = true
        if !shouldFlush {
                return false, false, nil
        }

and print TS used in import, or check TiKV

run TestAddIndexRemoteDuplicateCheck with the change, or other ADD INDEX that can trigger disk-quota import

diff --git a/tests/realtikvtest/addindextest4/ingest_test.go b/tests/realtikvtes
t/addindextest4/ingest_test.go
index 2e393ce4f8..7a3f91f031 100644
--- a/tests/realtikvtest/addindextest4/ingest_test.go
+++ b/tests/realtikvtest/addindextest4/ingest_test.go
@@ -407,7 +407,8 @@ func TestAddIndexRemoteDuplicateCheck(t *testing.T) {
        tk.MustExec("insert into t values(100000, 1, 1);")

        ingest.ForceSyncFlagForTest = true
-       tk.MustGetErrCode("alter table t add unique index idx(b);", errno.ErrDupEntry)
+       tk.MustExec("alter table t add index idx_test(b);")
+       //tk.MustGetErrCode("alter table t add unique index idx(b);", errno.ErrDupEntry)
        ingest.ForceSyncFlagForTest = false

        tk.MustExec("set global tidb_ddl_reorg_worker_cnt=4;")

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

no error

3. What did you see instead (Required)

TiKV panic due to it has data with TS = 0

4. What is your TiDB version? (Required)

@lance6716 lance6716 added type/bug The issue is confirmed as a bug. severity/critical labels Dec 4, 2024
@lance6716 lance6716 added severity/critical type/bug The issue is confirmed as a bug. affects-8.1 This bug affects the 8.1.x(LTS) versions. affects-8.5 This bug affects the 8.5.x(LTS) versions. and removed type/bug The issue is confirmed as a bug. severity/critical labels Dec 4, 2024
@lance6716
Copy link
Contributor Author

lance6716 commented Dec 4, 2024

introduced by #52993 #53188 . When reset TS for engine, it write the new TS to a wrong field. The expected TS field is still zero.

@tangenta
Copy link
Contributor

tangenta commented Dec 4, 2024

Trigger conditions:

  1. Run create-index DDL.
  2. The target index size exceeds the 'temp-dir' capacity or @@tidb_ddl_disk_quota(by default, it is 100GiB).
  3. @@tidb_enable_dist_task is disabled. In newly created v8.1 or v8.5 clusters, this variable has been enabled by default. But for upgraded cluster, this variable is likely disabled

@seiya-annie
Copy link

/report customer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-8.1 This bug affects the 8.1.x(LTS) versions. affects-8.5 This bug affects the 8.5.x(LTS) versions. component/ddl This issue is related to DDL of TiDB. impact/crash crash/fatal report/customer Customers have encountered this bug. severity/critical type/bug The issue is confirmed as a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants