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

query returns incorrect result when context done in the middle #50089

Closed
D3Hunter opened this issue Jan 4, 2024 · 2 comments · Fixed by #53489
Closed

query returns incorrect result when context done in the middle #50089

D3Hunter opened this issue Jan 4, 2024 · 2 comments · Fixed by #53489
Labels
affects-6.5 This bug affects the 6.5.x(LTS) versions. affects-7.1 This bug affects the 7.1.x(LTS) versions. affects-7.5 This bug affects the 7.5.x(LTS) versions. affects-7.6 affects-8.0 affects-8.1 This bug affects the 8.1.x(LTS) versions. may-affects-5.4 This bug maybe affects 5.4.x versions. may-affects-6.1 severity/moderate sig/execution SIG execution type/bug The issue is confirmed as a bug.

Comments

@D3Hunter
Copy link
Contributor

D3Hunter commented Jan 4, 2024

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

suppose we have this patch added to help reproduce

diff --git a/pkg/executor/table_reader.go b/pkg/executor/table_reader.go
index 4d87861c95..fddbfe284a 100644
--- a/pkg/executor/table_reader.go
+++ b/pkg/executor/table_reader.go
@@ -250,6 +250,9 @@ func (e *TableReaderExecutor) Next(ctx context.Context, req *chunk.Chunk) error
                }
                return tableName
        }), e.ranges)
+       if e.columns[0].Name.L == "a" {
+               time.Sleep(5 * time.Second)
+       }
        if err := e.resultHandler.nextChunk(ctx, req); err != nil {
                return err
        }

then run this test

func TestIncorrectQuery(t *testing.T) {
	store := testkit.CreateMockStore(t)
	tk := testkit.NewTestKit(t, store)
	tk.MustExec("use test")
	tk.MustExec("create table t(a int)")
	tk.MustExec("insert into t values(1)")

	ctx, cancelFunc := context.WithTimeout(context.Background(), 2*time.Second)
	defer cancelFunc()
	ctx = util.WithInternalSourceType(ctx, "scheduler")
	rs, err := tk.Session().ExecuteInternal(ctx, "select * from test.t")
	rows, err2 := session.ResultSetToStringSlice(ctx, tk.Session(), rs)
	t.Log("result&err: ", len(rows), err, err2)
}

might need to run it for a few times

    scheduler_test.go:720: result&err:  0 <nil> <nil>

here we don't return context error, if context cancelled in the middle the query returns incorrect result.
Query from normal client don't have issue, as we don't cancel context except when kill where it's handled.

case <-ctx.Done():
// We select the ctx.Done() in the thread of `Next` instead of in the worker to avoid the cost of `WithCancel`.
if atomic.CompareAndSwapUint32(&it.closed, 0, 1) {
close(it.finishCh)
}
exit = true
return

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

got err: context deadline exceed

3. What did you see instead (Required)

no error, result set is empty

4. What is your TiDB version? (Required)

@D3Hunter D3Hunter added the type/bug The issue is confirmed as a bug. label Jan 4, 2024
@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Jan 5, 2024

No reproducible step for now, I'll change the label to severity/major.

@D3Hunter
Copy link
Contributor Author

No reproducible step for now, I'll change the label to severity/major.

this ut can reproduce it stablely, https://github.com/pingcap/tidb/pull/53489/files#diff-2e7a91a59b5e9ba8d14357364ef5aa423fc0ab5c7624bb4104e4c12c0bde3470R42

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-6.5 This bug affects the 6.5.x(LTS) versions. affects-7.1 This bug affects the 7.1.x(LTS) versions. affects-7.5 This bug affects the 7.5.x(LTS) versions. affects-7.6 affects-8.0 affects-8.1 This bug affects the 8.1.x(LTS) versions. may-affects-5.4 This bug maybe affects 5.4.x versions. may-affects-6.1 severity/moderate sig/execution SIG execution type/bug The issue is confirmed as a bug.
Projects
None yet
5 participants