planner: check index valid while forUpdateRead #22152

you06 · 2021-01-04T07:45:51Z

What problem does this PR solve?

cherry pick #21596 to master as a temporary solution.

Issue Number: close #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.

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
Need to cherry-pick to the release branch

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

jackysp

LGTM

ti-srebot · 2021-01-04T13:07:39Z

@jackysp, Thanks for your review. The bot only counts LGTMs from Reviewers and higher roles, but you're still welcome to leave your comments. See the corresponding SIG page for more information. Related SIG: planner(slack).

lysu · 2021-01-05T03:02:21Z

planner/core/logical_plan_builder.go

+		if err != nil {
+			return nil, err
+		}
+	} else {


why add this if else~?

it seems buildTableRefsWithCache will return tableDual when sel.From == nil

tidb/planner/core/logical_plan_builder.go

Line 309 in b45fa8b

p = b.buildTableDual()

lysu · 2021-01-05T03:23:29Z

planner/core/logical_plan_builder.go

@@ -3599,7 +3608,7 @@ func (b *PlanBuilder) buildDataSource(ctx context.Context, tn *ast.TableName, as
 	if tblName.L == "" {
 		tblName = tn.Name
 	}
-	possiblePaths, err := getPossibleAccessPaths(b.ctx, b.TableHints(), tn.IndexHints, tbl, dbName, tblName)
+	possiblePaths, err := getPossibleAccessPaths(b.ctx, b.TableHints(), tn.IndexHints, tbl, dbName, tblName, b.isForUpdateRead, b.is.SchemaMetaVersion())


buildTableRefsWithCache's cache mechanism should be taken care...(which buildDs during correlatedAggregateResolver#resolveSelect)

for sql:

create table t1(id int); create table t2(id int); select (select id from t2 limit 1 for update), id from t1;

the value in this line should be:

tbl b.isForUpdateRead

t1 false

t2 true

but current get:

tbl b.isForUpdateRead

t1 false

t2 false

Good catch, added some cases for resolveSelect.

lysu · 2021-01-07T08:28:11Z

LGTM

you06 · 2021-01-07T08:47:12Z

I have a question about forUpdateRead.

insert into t1(id, v, v2) select (select id from t where v = 33), v, v2 from t where v = 34;
select (select id from t where v = 33), v, v2 from t where v = 34 for update;

For the above SQLs, select *** where v = 34 should use for update read, but what about the sub query of select *** where v = 33?

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

planner/core/planbuilder.go

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

lysu · 2021-01-08T03:06:05Z

I have a question about forUpdateRead.
insert into t1(id, v, v2) select (select id from t where v = 33), v, v2 from t where v = 34;
select (select id from t where v = 33), v, v2 from t where v = 34 for update;
For the above SQLs, select *** where v = 34 should use for update read, but what about the sub query of select *** where v = 33?

for mysql, ...where v = 34 should be current-read, and ...where v = 33 should be snapshot-read, so

create table t (id int primary key, v int);
insert into t values(1, 1), (2, 2);

-- t1: begin;
-- t1: select v from t where id in (1, 2) ----> v=1
-- t2: update t set v = v * 10 where id in (1, 2)
-- t1: select v, (select v from t where id = 1) vv from t where id = 2 for update  --> v = 20, vv = 1

cfzjywxk · 2021-01-08T03:08:26Z

I have a question about forUpdateRead.
insert into t1(id, v, v2) select (select id from t where v = 33), v, v2 from t where v = 34;
select (select id from t where v = 33), v, v2 from t where v = 34 for update;
For the above SQLs, select *** where v = 34 should use for update read, but what about the sub query of select *** where v = 33?

@lysu For TiDB I remember the subquery always use snapshot read, it that right?

lysu · 2021-01-08T03:18:51Z

it seems tidb work well for those test in decorrelated-subquery, but maybe it will break when correlate-subquery(for example: be rewriten into a join) 😂

lock-read in subquery is corner case...maybe we can check it later - -

MySQL [test]> select v, (select v from t where id = 1 for update) vv from t where id = 2 for update;
+------+------+
| v    | vv   |
+------+------+
|   20 |   10 |
+------+------+
1 row in set (0.003 sec)

MySQL [test]> 
MySQL [test]> 
MySQL [test]> 
MySQL [test]> select v, (select v from t where id = 1 for update) vv from t where id = 2;
+------+------+
| v    | vv   |
+------+------+
|    2 |   10 |
+------+------+
1 row in set (0.002 sec)

MySQL [test]> select v, (select v from t where id = 1) vv from t where id = 2 for update;
+------+------+
| v    | vv   |
+------+------+
|   20 |    1 |
+------+------+
1 row in set (0.003 sec)

MySQL [test]> select v, (select v from t where id = 1) vv from t where id = 2;
+------+------+
| v    | vv   |
+------+------+
|    2 |    1 |
+------+------+
1 row in set (0.003 sec)

MySQL [test]> select tidb_version();
+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| tidb_version()                                                                                                                                                                                                                              |
+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Release Version: None
Edition: Community
Git Commit Hash: None
Git Branch: None
UTC Build Time: None
GoVersion: go1.15
Race Enabled: false
TiKV Min Version: v3.0.0-60965b006877ca7234adaced7890d7b029ed1306
Check Table Before Drop: false |
+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.001 sec)

cfzjywxk · 2021-01-08T03:22:16Z

it seems tidb work well for those test in decorrelated-subquery, but maybe it will break when correlate-subquery(for example: be rewriten into a join) 😂

lock-read in subquery is corner case...- -

MySQL [test]> select v, (select v from t where id = 1 for update) vv from t where id = 2 for update;
+------+------+
| v    | vv   |
+------+------+
|   20 |   10 |
+------+------+
1 row in set (0.003 sec)

MySQL [test]> 
MySQL [test]> 
MySQL [test]> 
MySQL [test]> select v, (select v from t where id = 1 for update) vv from t where id = 2;
+------+------+
| v    | vv   |
+------+------+
|    2 |   10 |
+------+------+
1 row in set (0.002 sec)

MySQL [test]> select v, (select v from t where id = 1) vv from t where id = 2 for update;
+------+------+
| v    | vv   |
+------+------+
|   20 |    1 |
+------+------+
1 row in set (0.003 sec)

MySQL [test]> select v, (select v from t where id = 1) vv from t where id = 2;
+------+------+
| v    | vv   |
+------+------+
|    2 |    1 |
+------+------+
1 row in set (0.003 sec)

MySQL [test]> select tidb_version();
+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| tidb_version()                                                                                                                                                                                                                              |
+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Release Version: None
Edition: Community
Git Commit Hash: None
Git Branch: None
UTC Build Time: None
GoVersion: go1.15
Race Enabled: false
TiKV Min Version: v3.0.0-60965b006877ca7234adaced7890d7b029ed1306
Check Table Before Drop: false |
+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.001 sec)

That is a problem.. Actually if two statements are equal from the planner prospective but returns different results..
I'm confused.. How should we set our expected behaviours?

cfzjywxk · 2021-01-08T03:24:15Z

Seems here it's coupled with planner much more than we think for the for update statement processing.

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

you06 · 2021-01-08T05:48:40Z

945f4cf forbid the propagate of current read for correlate-aggregate query, in another word, correlate-subquery should use snapshot read unless it has forUpdate decorated, we got the following result then.

insert into t1(id, v, v2) select (select id from t where v = 33), v, v2 from t where v = 34;
                                           └ snapshot read                   └ current read
select (select id from t where v = 33), v, v2 from t where v = 34 for update;
                      └ snapshot read             └ current read

@lysu, @cfzjywxk PTAL.

cfzjywxk · 2021-01-08T08:01:41Z

945f4cf forbid the propagate of current read for correlate-aggregate query, in another word, correlate-subquery should use snapshot read unless it has forUpdate decorated, we got the following result then.
insert into t1(id, v, v2) select (select id from t where v = 33), v, v2 from t where v = 34;
                                           └ snapshot read                   └ current read
select (select id from t where v = 33), v, v2 from t where v = 34 for update;
                      └ snapshot read             └ current read
@lysu, @cfzjywxk PTAL.

@you06
This place should be rewriting the subquery expression but not correlatedAggregateResolver?

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

you06 · 2021-01-08T10:20:12Z

945f4cf forbid the propagate of current read for correlate-aggregate query, in another word, correlate-subquery should use snapshot read unless it has forUpdate decorated, we got the following result then.
insert into t1(id, v, v2) select (select id from t where v = 33), v, v2 from t where v = 34;
                                           └ snapshot read                   └ current read
select (select id from t where v = 33), v, v2 from t where v = 34 for update;
                      └ snapshot read             └ current read
@lysu, @cfzjywxk PTAL.
@you06
This place should be rewriting the subquery expression but not correlatedAggregateResolver?

Yes, and I added the case, the capture mechanism should be patches to release-4.0 also.

cfzjywxk · 2021-01-09T03:05:55Z

@lysu PTAL

lysu · 2021-01-11T12:32:17Z

planner/core/expression_rewriter.go

+	// capture the forUpdateRead mark
+	isForUpdateRead := b.isForUpdateRead
+	b.isForUpdateRead = false
 	expr, resultPlan, err := b.rewriteWithPreprocess(ctx, exprNode, p, aggMapper, nil, asScalar, nil)
+	b.isForUpdateRead = isForUpdateRead


it seems without this change, TestIssue21498 still can work

and it seems subQuery in Projection will call rewriteWithPreprocess directly

tidb/planner/core/logical_plan_builder.go

Line 1222 in f6cac2f

newExpr, np, err := b.rewriteWithPreprocess(ctx, field.Expr, p, mapper, windowMapper, true, nil)

Yeah, removed the capture now, at least not in this PR.

lysu · 2021-01-11T12:37:18Z

planner/core/logical_plan_builder.go

@@ -2253,6 +2253,10 @@ func (r *correlatedAggregateResolver) resolveSelect(sel *ast.SelectStmt) (err er
 	if err != nil {
 		return err
 	}
+	// do not use cache when for update read
+	if isForUpdateReadSelectLock(sel.LockInfo) {
+		useCache = false


after debug, it seems useCache = false only not-use-cache, but it still refill cache..

so in Access Path Selection will happen in correlatedAggregateResolver and directly use cache in later buildProjection

Seems useCache = false only cache sub-sub-query when there is a select stmt inside from clause.

I added some cases for sub queries, the cache problem is also related with rewrite, it may be solved together with it.

lysu · 2021-01-11T12:47:34Z

@you06 @cfzjywxk how about fix subQuery question in other PRs and merge this one first, it seems too many question in subquery + lock like #22345, but it's not critial :)

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

lysu · 2021-01-12T06:30:35Z

LGTM

cfzjywxk · 2021-01-12T06:56:40Z

LGTM

cfzjywxk · 2021-01-12T06:56:55Z

/merge

ti-srebot · 2021-01-12T06:57:25Z

/run-all-tests

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2021-01-12T07:10:47Z

cherry pick to release-5.0-rc in PR #22365

you06 requested a review from a team as a code owner January 4, 2021 07:45

you06 requested review from SunRunAway and removed request for a team January 4, 2021 07:45

cfzjywxk requested review from MyonKeminta, coocood, jackysp and lysu January 4, 2021 07:52

cfzjywxk added type/bugfix This PR fixes a bug. sig/planner SIG: Planner sig/transaction SIG:Transaction needs-cherry-pick-5.0-rc and removed sig/transaction SIG:Transaction labels Jan 4, 2021

you06 requested a review from cfzjywxk January 4, 2021 09:04

ichn-hu mentioned this pull request Jan 4, 2021

Welcome to contribute #20804

Closed

jackysp reviewed Jan 4, 2021

View reviewed changes

lysu reviewed Jan 5, 2021

View reviewed changes

cfzjywxk added the sig/transaction SIG:Transaction label Jan 6, 2021

github-actions bot added the sig/sql-infra SIG: SQL Infra label Jan 7, 2021

ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 7, 2021

you06 added 4 commits January 7, 2021 17:14

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

63d2fe6

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

fix panic

069340f

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

do not use cache when for update read

a1a2c47

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

fix invalid cache usage

13a4ba0

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

you06 force-pushed the cp-21596 branch from b86052e to 13a4ba0 Compare January 7, 2021 09:15

cfzjywxk reviewed Jan 7, 2021

View reviewed changes

planner/core/planbuilder.go Show resolved Hide resolved

add comments

ace8009

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

capture isForUpdateRead

945f4cf

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

move capture to rewrite

f6cac2f

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

lysu reviewed Jan 11, 2021

View reviewed changes

you06 added 2 commits January 12, 2021 14:03

add test for sub queries

ac74dd7

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

Merge branch 'master' into cp-21596

19e9dfa

ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Jan 12, 2021

ti-srebot approved these changes Jan 12, 2021

View reviewed changes

ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Jan 12, 2021

ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 12, 2021

ti-srebot merged commit 4088c20 into pingcap:master Jan 12, 2021

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jan 12, 2021

cherry pick pingcap#22152 to release-5.0-rc

aedecb5

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Jan 12, 2021

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

Closed

you06 deleted the cp-21596 branch January 12, 2021 07:12

time-and-fate mentioned this pull request Oct 27, 2021

planner: fix outerJoinEliminator couldn't be applied in some cases #29061

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: check index valid while forUpdateRead #22152

planner: check index valid while forUpdateRead #22152

you06 commented Jan 4, 2021 •

edited

Loading

jackysp left a comment

ti-srebot commented Jan 4, 2021

lysu Jan 5, 2021

lysu Jan 5, 2021 •

edited

Loading

you06 Jan 6, 2021

lysu commented Jan 7, 2021

you06 commented Jan 7, 2021 •

edited

Loading

lysu commented Jan 8, 2021

cfzjywxk commented Jan 8, 2021

lysu commented Jan 8, 2021 •

edited

Loading

cfzjywxk commented Jan 8, 2021

cfzjywxk commented Jan 8, 2021

you06 commented Jan 8, 2021 •

edited

Loading

cfzjywxk commented Jan 8, 2021

you06 commented Jan 8, 2021

cfzjywxk commented Jan 9, 2021

lysu Jan 11, 2021

you06 Jan 12, 2021

lysu Jan 11, 2021

you06 Jan 12, 2021

you06 Jan 12, 2021

lysu commented Jan 11, 2021 •

edited

Loading

lysu commented Jan 12, 2021

cfzjywxk commented Jan 12, 2021

cfzjywxk commented Jan 12, 2021

ti-srebot commented Jan 12, 2021

ti-srebot commented Jan 12, 2021

planner: check index valid while forUpdateRead #22152

planner: check index valid while forUpdateRead #22152

Conversation

you06 commented Jan 4, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

jackysp left a comment

Choose a reason for hiding this comment

ti-srebot commented Jan 4, 2021

lysu Jan 5, 2021

Choose a reason for hiding this comment

lysu Jan 5, 2021 • edited Loading

Choose a reason for hiding this comment

you06 Jan 6, 2021

Choose a reason for hiding this comment

lysu commented Jan 7, 2021

you06 commented Jan 7, 2021 • edited Loading

lysu commented Jan 8, 2021

cfzjywxk commented Jan 8, 2021

lysu commented Jan 8, 2021 • edited Loading

cfzjywxk commented Jan 8, 2021

cfzjywxk commented Jan 8, 2021

you06 commented Jan 8, 2021 • edited Loading

cfzjywxk commented Jan 8, 2021

you06 commented Jan 8, 2021

cfzjywxk commented Jan 9, 2021

lysu Jan 11, 2021

Choose a reason for hiding this comment

you06 Jan 12, 2021

Choose a reason for hiding this comment

lysu Jan 11, 2021

Choose a reason for hiding this comment

you06 Jan 12, 2021

Choose a reason for hiding this comment

you06 Jan 12, 2021

Choose a reason for hiding this comment

lysu commented Jan 11, 2021 • edited Loading

lysu commented Jan 12, 2021

cfzjywxk commented Jan 12, 2021

cfzjywxk commented Jan 12, 2021

ti-srebot commented Jan 12, 2021

ti-srebot commented Jan 12, 2021

you06 commented Jan 4, 2021 •

edited

Loading

lysu Jan 5, 2021 •

edited

Loading

you06 commented Jan 7, 2021 •

edited

Loading

lysu commented Jan 8, 2021 •

edited

Loading

you06 commented Jan 8, 2021 •

edited

Loading

lysu commented Jan 11, 2021 •

edited

Loading