planner: forbid point get with select lock choose tiflash as datasource path #39388

Yisaer · 2022-11-25T04:57:30Z

What problem does this PR solve?

Issue Number: close #39344

Problem Summary:

What is changed and how it works?

forbid point get with select lock choose tiflash as datasource path

Check List

Tests

Unit test

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Signed-off-by: yisaer <disxiaofei@163.com>

ti-chi-bot · 2022-11-25T04:57:31Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

fixdb
qw4990

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: yisaer <disxiaofei@163.com>

fixdb · 2022-11-25T09:12:12Z

planner/property/physical_property.go

@@ -202,6 +202,9 @@ type PhysicalProperty struct {
 	// RejectSort means rejecting the sort property from its children, but it only works for MPP tasks.
 	// Non-MPP tasks do not care about it.
 	RejectSort bool
+
+	// InLock indicates the Plan is under LogicalLock
+	InLock bool


Why do we need it in physical property? Is there any other better place?

updated, use isUpdateRead instead

winoros

We can do this just in the plan builder?

Signed-off-by: yisaer <disxiaofei@163.com> address the comment Signed-off-by: yisaer <disxiaofei@163.com>

Yisaer · 2022-11-28T03:43:01Z

We can do this just in the plan builder?

I tended to check it during findbestTask thus we can directly used the info in the isUpdateRead

Signed-off-by: yisaer <disxiaofei@163.com>

fixdb

+1

gengliqi · 2022-11-29T02:38:35Z

planner/core/integration_test.go

@@ -7554,6 +7554,26 @@ func TestEnableTiFlashReadForWriteStmt(t *testing.T) {
 	checkMpp(rs)
 }

+func TestPointGetWithSelectLock(t *testing.T) {


Not just a test for a primary key, a test for a unique key is also necessary.

Yisaer · 2022-11-29T07:17:34Z

/run-unit-test

Yisaer · 2022-11-29T10:14:32Z

/merge

ti-chi-bot · 2022-11-29T10:14:36Z

This pull request has been accepted and is ready to merge.

Commit hash: 38e1593

gengliqi

LGTM

ti-chi-bot · 2022-11-29T10:18:06Z

@gengliqi: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

LGTM

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

Yisaer · 2022-11-29T14:21:26Z

/merge

ti-chi-bot · 2022-11-29T14:21:29Z

This pull request has been accepted and is ready to merge.

Commit hash: 9f5d8cd

sre-bot · 2022-11-29T17:36:26Z

TiDB MergeCI notify

✅ Well Done! New fixed [1] after this pr merged.

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/integration-ddl-test	✅ all 6 tests passed	5 min 19 sec	Fixed
idc-jenkins-ci/integration-cdc-test	🟢 all 40 tests passed	28 min	Existing passed
idc-jenkins-ci-tidb/integration-common-test	🟢 all 17 tests passed	14 min	Existing passed
idc-jenkins-ci-tidb/common-test	🟢 all 11 tests passed	9 min 19 sec	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	6 min 59 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	6 min 10 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	4 min 40 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	4 min 8 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	2 min 30 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

Yisaer added 4 commits November 25, 2022 12:18

fix

d9affe7

Signed-off-by: yisaer <disxiaofei@163.com>

fix

b632349

Signed-off-by: yisaer <disxiaofei@163.com>

add test

d83c23c

Signed-off-by: yisaer <disxiaofei@163.com>

add test

238f7a1

Signed-off-by: yisaer <disxiaofei@163.com>

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 25, 2022

Yisaer added 3 commits November 25, 2022 13:32

add test

117a1e6

Signed-off-by: yisaer <disxiaofei@163.com>

add test

f1e48da

Signed-off-by: yisaer <disxiaofei@163.com>

fix

c7b6250

Signed-off-by: yisaer <disxiaofei@163.com>

fixdb reviewed Nov 25, 2022

View reviewed changes

winoros reviewed Nov 25, 2022

View reviewed changes

address the comment

b8e5712

Signed-off-by: yisaer <disxiaofei@163.com> address the comment Signed-off-by: yisaer <disxiaofei@163.com>

Yisaer force-pushed the fix_39344_2 branch from ba5dc6c to b8e5712 Compare November 28, 2022 03:37

qw4990 approved these changes Nov 28, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 28, 2022

fix test

89f9eaa

Signed-off-by: yisaer <disxiaofei@163.com>

Yisaer force-pushed the fix_39344_2 branch from 60e537c to 89f9eaa Compare November 28, 2022 05:03

fix test

4af70e7

Signed-off-by: yisaer <disxiaofei@163.com>

Yisaer force-pushed the fix_39344_2 branch from a4fe1b4 to 4af70e7 Compare November 28, 2022 05:33

Yisaer added 2 commits November 28, 2022 14:34

fix

d5aaf6f

Signed-off-by: yisaer <disxiaofei@163.com>

Merge remote-tracking branch 'upstream/master' into fix_39344_2

2453692

fixdb approved these changes Nov 29, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 29, 2022

gengliqi reviewed Nov 29, 2022

View reviewed changes

add unique testcase

fcc9b0c

Yisaer force-pushed the fix_39344_2 branch from c6bb3c4 to fcc9b0c Compare November 29, 2022 05:20

ti-chi-bot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 29, 2022

ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 29, 2022

add comment

285950b

address the comment

941cbe3

Yisaer force-pushed the fix_39344_2 branch from 5f5e325 to 941cbe3 Compare November 29, 2022 08:00

address the comment

13ef191

Yisaer force-pushed the fix_39344_2 branch from 0f6a356 to 13ef191 Compare November 29, 2022 08:07

address the comment

05e5d82

Yisaer force-pushed the fix_39344_2 branch from b404265 to 05e5d82 Compare November 29, 2022 08:58

address the comment

38e1593

Yisaer force-pushed the fix_39344_2 branch from fd1f528 to 38e1593 Compare November 29, 2022 10:14

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 29, 2022

gengliqi approved these changes Nov 29, 2022

View reviewed changes

address the comment

2cc606b

Yisaer force-pushed the fix_39344_2 branch from 52f37e1 to 2cc606b Compare November 29, 2022 14:21

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Nov 29, 2022

Merge branch 'master' into fix_39344_2

9f5d8cd

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 29, 2022

ti-chi-bot added 4 commits November 29, 2022 23:04

Merge branch 'master' into fix_39344_2

c9b11eb

Merge branch 'master' into fix_39344_2

0d889b8

Merge branch 'master' into fix_39344_2

f7e445c

Merge branch 'master' into fix_39344_2

3841f1b

ti-chi-bot merged commit cca78e3 into pingcap:master Nov 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: forbid point get with select lock choose tiflash as datasource path #39388

planner: forbid point get with select lock choose tiflash as datasource path #39388

Yisaer commented Nov 25, 2022

ti-chi-bot commented Nov 25, 2022 •

edited

Loading

fixdb Nov 25, 2022 •

edited

Loading

Yisaer Nov 28, 2022 •

edited

Loading

winoros left a comment

Yisaer commented Nov 28, 2022 •

edited

Loading

fixdb left a comment

gengliqi Nov 29, 2022

Yisaer Nov 29, 2022

Yisaer commented Nov 29, 2022

Yisaer commented Nov 29, 2022

ti-chi-bot commented Nov 29, 2022

gengliqi left a comment

ti-chi-bot commented Nov 29, 2022

Yisaer commented Nov 29, 2022

ti-chi-bot commented Nov 29, 2022

sre-bot commented Nov 29, 2022

planner: forbid point get with select lock choose tiflash as datasource path #39388

planner: forbid point get with select lock choose tiflash as datasource path #39388

Conversation

Yisaer commented Nov 25, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Nov 25, 2022 • edited Loading

fixdb Nov 25, 2022 • edited Loading

Choose a reason for hiding this comment

Yisaer Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

winoros left a comment

Choose a reason for hiding this comment

Yisaer commented Nov 28, 2022 • edited Loading

fixdb left a comment

Choose a reason for hiding this comment

gengliqi Nov 29, 2022

Choose a reason for hiding this comment

Yisaer Nov 29, 2022

Choose a reason for hiding this comment

Yisaer commented Nov 29, 2022

Yisaer commented Nov 29, 2022

ti-chi-bot commented Nov 29, 2022

gengliqi left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Nov 29, 2022

Yisaer commented Nov 29, 2022

ti-chi-bot commented Nov 29, 2022

sre-bot commented Nov 29, 2022

TiDB MergeCI notify

ti-chi-bot commented Nov 25, 2022 •

edited

Loading

fixdb Nov 25, 2022 •

edited

Loading

Yisaer Nov 28, 2022 •

edited

Loading

Yisaer commented Nov 28, 2022 •

edited

Loading