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 result is incorrect when execute prepared statement in RC isolation with plan cache enabled #34447

Closed
lcwangchao opened this issue May 7, 2022 · 7 comments · Fixed by #34523
Assignees
Labels
affects-4.0 This bug affects 4.0.x versions. affects-5.0 This bug affects 5.0.x versions. affects-5.1 This bug affects 5.1.x versions. affects-5.2 This bug affects 5.2.x versions. affects-5.3 This bug affects 5.3.x versions. affects-5.4 This bug affects 5.4.x versions. affects-6.0 epic/plan-cache severity/major sig/planner SIG: Planner sig/transaction SIG:Transaction type/bug The issue is confirmed as a bug.

Comments

@lcwangchao
Copy link
Collaborator

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

func TestPlanCacheWithRCWhenInfoSchemaChange(t *testing.T) {
	orgEnable := plannercore.PreparedPlanCacheEnabled()
	defer func() {
		plannercore.SetPreparedPlanCache(orgEnable)
	}()
	plannercore.SetPreparedPlanCache(true)

	store, clean := createStorage(t)
	defer clean()
	tk1 := testkit.NewTestKit(t, store)
	tk1.MustExec("use test")
	tk1.MustExec("create table t1(id int primary key, c int, index ic (c))")
	tk1.MustExec("prepare s from 'select /*+use_index(t1, ic)*/ * from t1 where 1'")
	tk1.MustExec("set tx_isolation='READ-COMMITTED'")
	tk1.MustExec("begin pessimistic")
	tk1.MustQuery("execute s").Check(testkit.Rows())

	tk2 := testkit.NewTestKit(t, store)
	tk2.MustExec("use test")
	tk2.MustExec("alter table t1 drop index ic")
	tk2.MustExec("insert into t1 values(1, 0)")

        // This assert will fail and the actual result is empty
	tk1.MustQuery("execute s").Check(testkit.Rows("1 0"))
}

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

The test case will pass

3. What did you see instead (Required)

Test test case failed

4. What is your TiDB version? (Required)

master

@lcwangchao lcwangchao added type/bug The issue is confirmed as a bug. sig/transaction SIG:Transaction labels May 7, 2022
@cfzjywxk
Copy link
Contributor

cfzjywxk commented May 9, 2022

@zyguan @vivid392845427
Is this the meta and data consistency historical issue or related issue?

@qw4990
Copy link
Contributor

qw4990 commented May 9, 2022

If you need any help from the optimizer(plan-cache) side, please contact me or @Reminiscent .

@qw4990
Copy link
Contributor

qw4990 commented May 9, 2022

I guess it's caused by we using the wrong snopshotTS in session.preparedStmtExec.

@zyguan
Copy link
Contributor

zyguan commented May 9, 2022

@zyguan @vivid392845427 Is this the meta and data consistency historical issue or related issue?

Related to #21498 . here both schema versions are staled, and then the cached plan gets used unexpectedly.

@qw4990
Copy link
Contributor

qw4990 commented May 9, 2022

I tested it locally and found that the schema version used by tk1 is not the latest.
It seems that under Read-Commit infoSchema.SchemaMetaVersion() doesn't return the latest schema-version in some cases and it breaks some assumptions required by our plan-cache... PTAL @zyguan @cfzjywxk
image

@ti-chi-bot ti-chi-bot added may-affects-4.0 This bug maybe affects 4.0.x versions. may-affects-5.0 This bug maybe affects 5.0.x versions. may-affects-5.1 This bug maybe affects 5.1.x versions. may-affects-5.2 This bug maybe affects 5.2.x versions. may-affects-5.3 This bug maybe affects 5.3.x versions. may-affects-5.4 This bug maybe affects 5.4.x versions. may-affects-6.0 labels May 9, 2022
@zyguan
Copy link
Contributor

zyguan commented May 9, 2022

@you06 @cfzjywxk We try to resolve the issue by setting ForUpdateRead in #22381. However, ForUpdateRead is true if SessionVars.IsPessimisticReadConsistency: s.TxnCtx.IsPessimistic && s.IsIsolation(ast.ReadCommitted) is true when preparing the statement. In this case:

  1. we do prepare before setting isolation level
  2. since prepare is autocommitted, s.TxnCtx.IsPessimistic is false
  3. I'm also not sure if text protocol can reach here

@you06
Copy link
Contributor

you06 commented May 10, 2022

We try to resolve the issue by setting ForUpdateRead in #22381.

#22381 only solve the issue in RR mode. And this issue contains a more complex case that it prepares the plan before changing the isolation level.

ForUpdateRead is the property of the statement which should keep the same in RC and RR isolation levels. Otherwise, switching between RC and RR repeatably can lead to some more complex issues.

We may check whether the schema is stale for plans which use ForUpdateRead or if we are in RC isolation level.

When rebuilding the plan, use the latest schema for statements that use ForUpdateRead or if we are in RC isolation level.

@you06 you06 added affects-4.0 This bug affects 4.0.x versions. affects-5.0 This bug affects 5.0.x versions. and removed may-affects-4.0 This bug maybe affects 4.0.x versions. may-affects-5.1 This bug maybe affects 5.1.x versions. may-affects-5.2 This bug maybe affects 5.2.x versions. may-affects-5.3 This bug maybe affects 5.3.x versions. may-affects-5.4 This bug maybe affects 5.4.x versions. may-affects-5.0 This bug maybe affects 5.0.x versions. may-affects-6.0 labels May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-4.0 This bug affects 4.0.x versions. affects-5.0 This bug affects 5.0.x versions. affects-5.1 This bug affects 5.1.x versions. affects-5.2 This bug affects 5.2.x versions. affects-5.3 This bug affects 5.3.x versions. affects-5.4 This bug affects 5.4.x versions. affects-6.0 epic/plan-cache severity/major sig/planner SIG: Planner sig/transaction SIG:Transaction type/bug The issue is confirmed as a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants