planner: check index valid while forUpdateRead #21596

you06 · 2020-12-09T07:00:45Z

Signed-off-by: you06 you1474600@gmail.com

What problem does this PR solve?

Issue Number: workaround for #21498

Problem Summary:

ForUpdateRead will read data using schema version of startTS, this may lead to incorrectness, the cases are listed in #21498

What is changed and how it works?

What's Changed:

For RC and forUpdateRead in RR, check if index's state is still StatePublic.

How it Works:

This check confirms index between snapshotTS and forUpdateTS is not changed, otherwise, the index should not be used.

Check List

Tests

Unit test

Side effects

Performance regression
- Consumes more CPU, there is a little regression testing with tpc-c

Release note

Fix a bug that schema change will lead to incorrectness when forUpdateRead

MyonKeminta · 2020-12-10T03:25:22Z

🤔 Is it ok to fix it directly to release-4.0 without changing master?

you06 · 2020-12-10T03:36:35Z

thinking Is it ok to fix it directly to release-4.0 without changing master?

IMO, there should be a better solution in master.

you06 · 2020-12-12T14:51:06Z

tpcc 1000 warehouses, 128 threads
before this PR: tpmC: 16906.1, efficiency: 131.5%
this PR: tpmC: 16607.8, efficiency: 129.1%

It's about 1.8% of regression.

Signed-off-by: you06 <you1474600@gmail.com>

planner/core/planbuilder.go

planner/core/point_get_plan.go

cfzjywxk · 2020-12-16T05:38:35Z

session/pessimistic_test.go

@@ -1903,3 +1903,106 @@ func (s *testPessimisticSuite) TestResolveStalePessimisticPrimaryLock(c *C) {

 	c.Assert(tk2.ExecToErr("admin check table t1"), IsNil)
 }
+
+func (s *testPessimisticSuite) TestIssue21498(c *C) {


Need to add some unique key uk(varchar_column) case using the IndexLookUp executor fetching data from unique index and look up data in main table.

Added snapshot read and for update read cases for IndexLookUp.

How about the insert into table select from anothertable, we have another issue #21506, what is the expecting behaviour for this case?

Signed-off-by: you06 <you1474600@gmail.com>

you06 · 2020-12-16T09:22:25Z

I wonder there will be a better solution for this issue in master and future release versions, do we need to cherry pick this hotfix to master?

cfzjywxk · 2020-12-16T09:29:32Z

I wonder there will be a better solution for this issue in master and future release versions, do we need to cherry pick this hotfix to master?

Yes, on master maybe we need a better mechanism to solve the concurrent ddl and dml problem so we won't meet this problem.

planner/core/planbuilder.go

Signed-off-by: you06 <you1474600@gmail.com>

planner/core/logical_plan_builder.go

planner/core/point_get_plan.go

cfzjywxk · 2020-12-17T07:36:00Z

session/pessimistic_test.go

@@ -1903,3 +1903,106 @@ func (s *testPessimisticSuite) TestResolveStalePessimisticPrimaryLock(c *C) {

 	c.Assert(tk2.ExecToErr("admin check table t1"), IsNil)
 }
+
+func (s *testPessimisticSuite) TestIssue21498(c *C) {


How about the insert into table select from anothertable, we have another issue #21506, what is the expecting behaviour for this case?

lysu · 2020-12-17T09:56:15Z

session/pessimistic_test.go

+			// amend transaction does not support partition table
+			tk.MustExec("insert into t(id, v, v2) select 6, v + 20, v2 + 200 from t where id = 4") // insert ... select with index unchanged
+		}
+		err = tk.ExecToErr("insert into t(id, v, v2) select 7, v + 30, v2 + 300 from t use index (iv) where id = 4") // insert ... select with index changed


maybe need add more test: prepare plan cache (maybe have question) and plan binding (maybe have no question)

lysu · 2020-12-21T03:05:26Z

LGTM

Signed-off-by: you06 <you1474600@gmail.com>

cfzjywxk · 2020-12-21T05:47:08Z

LGTM

cfzjywxk · 2020-12-22T03:35:25Z

/merge

ti-srebot · 2020-12-22T03:35:28Z

Sorry @cfzjywxk, this branch cannot be merged without an approval of release maintainers.

cfzjywxk · 2020-12-22T04:00:15Z

/merge

ti-srebot · 2020-12-22T04:02:29Z

/run-all-tests

ti-srebot · 2020-12-22T04:14:57Z

@you06 merge failed.

you06 · 2020-12-22T05:44:17Z

/run-unit-test

cfzjywxk · 2020-12-22T05:59:28Z

/merge

ti-srebot · 2020-12-22T05:59:57Z

/run-all-tests

Signed-off-by: you06 <you1474600@gmail.com>

you06 requested review from coocood and wjhuang2016 December 11, 2020 12:12

lysu self-requested a review December 14, 2020 03:25

cfzjywxk requested review from zyguan and MyonKeminta December 14, 2020 08:25

you06 added 8 commits December 15, 2020 13:48

check index while forUpdateRead

e53d24e

Signed-off-by: you06 <you1474600@gmail.com>

fix admin check

b92185f

Signed-off-by: you06 <you1474600@gmail.com>

do not influence inner stmt

334f340

Signed-off-by: you06 <you1474600@gmail.com>

improve performance

efe7940

Signed-off-by: you06 <you1474600@gmail.com>

skip check for same start ts

b3ae30c

Signed-off-by: you06 <you1474600@gmail.com>

fix fast plan

1d66010

Signed-off-by: you06 <you1474600@gmail.com>

add tests for RR

6fd79cf

Signed-off-by: you06 <you1474600@gmail.com>

add test for update/delete in common path

9267958

Signed-off-by: you06 <you1474600@gmail.com>

you06 force-pushed the check-index-valid branch from a651f42 to 9267958 Compare December 15, 2020 05:50

cfzjywxk reviewed Dec 16, 2020

View reviewed changes

cfzjywxk added type/bugfix This PR fixes a bug. sig/transaction SIG:Transaction labels Dec 16, 2020

cfzjywxk added this to the v4.0.10 milestone Dec 16, 2020

you06 added 2 commits December 16, 2020 16:24

address comment

37dc87f

Signed-off-by: you06 <you1474600@gmail.com>

add index look up tests & remove log

3681642

Signed-off-by: you06 <you1474600@gmail.com>

cfzjywxk reviewed Dec 17, 2020

View reviewed changes

planner/core/planbuilder.go Outdated Show resolved Hide resolved

planner/core/planbuilder.go Show resolved Hide resolved

cfzjywxk added the sig/planner SIG: Planner label Dec 17, 2020

address comment & lazy check schema

f11db75

Signed-off-by: you06 <you1474600@gmail.com>

cfzjywxk reviewed Dec 17, 2020

View reviewed changes

lysu reviewed Dec 17, 2020

View reviewed changes

ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 21, 2020

you06 added 3 commits December 21, 2020 11:36

Merge branch 'release-4.0' into check-index-valid

094b98b

address comment

25e473e

Signed-off-by: you06 <you1474600@gmail.com>

add test case for

8a4530a

Signed-off-by: you06 <you1474600@gmail.com>

ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 21, 2020

ti-srebot approved these changes Dec 21, 2020

View reviewed changes

ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 22, 2020

Merge branch 'release-4.0' into check-index-valid

7b71aa8

ti-srebot merged commit a6f2643 into pingcap:release-4.0 Dec 22, 2020

you06 deleted the check-index-valid branch December 22, 2020 06:13

cfzjywxk mentioned this pull request Dec 30, 2020

RC transactions may see inconsistent data when there are concurrent DDLs #21498

Closed

you06 mentioned this pull request Jan 4, 2021

planner: check index valid while forUpdateRead #22152

Merged

you06 added a commit to you06/tidb that referenced this pull request Jan 7, 2021

planner: check index valid while forUpdateRead (pingcap#21596)

63d2fe6

Signed-off-by: you06 <you1474600@gmail.com>

ti-srebot mentioned this pull request Jan 12, 2021

planner: check index valid while forUpdateRead (#22152) #22365

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: check index valid while forUpdateRead #21596

planner: check index valid while forUpdateRead #21596

you06 commented Dec 9, 2020 •

edited

Loading

MyonKeminta commented Dec 10, 2020

you06 commented Dec 10, 2020

you06 commented Dec 12, 2020

cfzjywxk Dec 16, 2020

you06 Dec 16, 2020

cfzjywxk Dec 17, 2020

you06 commented Dec 16, 2020

cfzjywxk commented Dec 16, 2020

cfzjywxk Dec 17, 2020

lysu Dec 17, 2020

lysu commented Dec 21, 2020

cfzjywxk commented Dec 21, 2020

cfzjywxk commented Dec 22, 2020

ti-srebot commented Dec 22, 2020

cfzjywxk commented Dec 22, 2020

ti-srebot commented Dec 22, 2020

ti-srebot commented Dec 22, 2020

you06 commented Dec 22, 2020

cfzjywxk commented Dec 22, 2020

ti-srebot commented Dec 22, 2020

planner: check index valid while forUpdateRead #21596

planner: check index valid while forUpdateRead #21596

Conversation

you06 commented Dec 9, 2020 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

MyonKeminta commented Dec 10, 2020

you06 commented Dec 10, 2020

you06 commented Dec 12, 2020

cfzjywxk Dec 16, 2020

Choose a reason for hiding this comment

you06 Dec 16, 2020

Choose a reason for hiding this comment

cfzjywxk Dec 17, 2020

Choose a reason for hiding this comment

you06 commented Dec 16, 2020

cfzjywxk commented Dec 16, 2020

cfzjywxk Dec 17, 2020

Choose a reason for hiding this comment

lysu Dec 17, 2020

Choose a reason for hiding this comment

lysu commented Dec 21, 2020

cfzjywxk commented Dec 21, 2020

cfzjywxk commented Dec 22, 2020

ti-srebot commented Dec 22, 2020

cfzjywxk commented Dec 22, 2020

ti-srebot commented Dec 22, 2020

ti-srebot commented Dec 22, 2020

you06 commented Dec 22, 2020

cfzjywxk commented Dec 22, 2020

ti-srebot commented Dec 22, 2020

you06 commented Dec 9, 2020 •

edited

Loading