Skip to content

*: support paging protocol on unistore#35244

Merged
ti-chi-bot merged 12 commits intopingcap:masterfrom
tiancaiamao:paging-unistore
Jun 14, 2022
Merged

*: support paging protocol on unistore#35244
ti-chi-bot merged 12 commits intopingcap:masterfrom
tiancaiamao:paging-unistore

Conversation

@tiancaiamao
Copy link
Contributor

@tiancaiamao tiancaiamao commented Jun 8, 2022 •
edited
Loading

What problem does this PR solve?

Issue Number: close #35243 #35242

Problem Summary:

I'm try to support coprocessor paging protocol on unistore, and found those issues:

  • There are several kinds of coprocessor request, including DAG and others. admin check table/index analyze are not using DAG so they don't support paging
  • Find bug In dynamic partition mode distsql request key ranges is not sorted #35242 that for partition table on dynamic pruning mode, the key ranges is not sorted. So rebuilding cop tasks and calculate remain key ranges get wrong result.
  • cop cache for paging is not well supported.
  • When enable/disable paging, the explain result (related to IndexLookup) is different, so some test cases need to be updated.
  • QueryFeedback is broken when coprocessor paging is enabled.
  • mocktikv doesn't support paging, and some test cases are still using mocktikv, so if we enable paging by default, those test cases needs update
  • Read from INFORMATION_SCHEMA.CLUSTER_XXX table use coprocessor request to query tidb instances, and tidb coprocessor doesn't support paging

What is changed and how it works?

You can ignore all the xxx_test.go files and just review cop_handler.go / mpp_exec.go
The actual changes are there.

In brief, I update the paging in tablescan and indexscan's next() function to keep the paging key range update to date.
And in the out most loop, check if the chunk pages are enough to break the next() looping.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)

I make session variable tidb_enable_paging default value to On and run all the unit tests to check everything go work well.

  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

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

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jun 8, 2022 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • XuHuaiyu
  • wshwsh12

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.

Details

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

@ti-chi-bot ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 8, 2022
@ti-chi-bot ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 9, 2022
@tiancaiamao tiancaiamao marked this pull request as ready for review June 9, 2022 07:21
@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 9, 2022
@tiancaiamao tiancaiamao requested review from XuHuaiyu and wshwsh12 June 9, 2022 07:21
@sre-bot
Copy link
Contributor

sre-bot commented Jun 9, 2022 •
edited
Loading

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/827ef7b9b3182a8cfb4214c9becfdec070eb93e5

@tiancaiamao
Copy link
Contributor Author

tiancaiamao commented Jun 9, 2022

/run-unit-test

@tiancaiamao tiancaiamao mentioned this pull request Jun 9, 2022
Merged
12 tasks
@tiancaiamao
Copy link
Contributor Author

tiancaiamao commented Jun 10, 2022

PTAL @XuHuaiyu @wshwsh12

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2022
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2022
@tiancaiamao
Copy link
Contributor Author

tiancaiamao commented Jun 10, 2022

/run-unit-test
/run-realtikv-test

@tiancaiamao
Copy link
Contributor Author

tiancaiamao commented Jun 10, 2022

/run-unit-test

@hawkingrei
Copy link
Member

hawkingrei commented Jun 10, 2022

/run-unit-test

1 similar comment
@tiancaiamao
Copy link
Contributor Author

tiancaiamao commented Jun 10, 2022

/run-unit-test

@tiancaiamao
Copy link
Contributor Author

tiancaiamao commented Jun 13, 2022

PTAL @XuHuaiyu @wshwsh12

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2022
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jun 13, 2022

@tiancaiamao: PR needs rebase.

Details

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 kubernetes/test-infra repository.

wshwsh12
wshwsh12 approved these changes Jun 14, 2022
View reviewed changes
Copy link
Contributor

@wshwsh12 wshwsh12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 14, 2022
XuHuaiyu
XuHuaiyu reviewed Jun 14, 2022
View reviewed changes
store/copr/coprocessor.go Outdated
Copy link
Contributor

@XuHuaiyu XuHuaiyu Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that ReqTypeAnalyze will not use paging. We do not need to set enable_paging = false for the analyze tests.

Copy link
Contributor Author

@tiancaiamao tiancaiamao Jun 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

paging is a property of the kv request,
kv request can be one of the following kinds:

// ReqTypes.
const (
	ReqTypeSelect   = 101
	ReqTypeIndex    = 102
	ReqTypeDAG      = 103
	ReqTypeAnalyze  = 104
	ReqTypeChecksum = 105

	ReqSubTypeBasic      = 0
	ReqSubTypeDesc       = 10000
	ReqSubTypeGroupBy    = 10001
	ReqSubTypeTopN       = 10002
	ReqSubTypeSignature  = 10003
	ReqSubTypeAnalyzeIdx = 10004
	ReqSubTypeAnalyzeCol = 10005
)

As you can see, there are ReqTypeDAG and ReqTypeAnalyze, and also ReqTypeChecksum for admin check table ... for now, we just support ReqTypeDAG, so all other cases req.Paging = false is set.

A possible change is that we can move the paging property to coprocessor.Request.
That change would make it clear that paging is a property of ReqTypeDAG (or coprocessor.Request to be more specific), not a property kv.Request.
The drawback of the change is, if one day, we need paging protocol also for Analyze/Checksum, we need to add the property to kv.Request...

@ti-chi-bot 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 Jun 14, 2022
@tiancaiamao
Copy link
Contributor Author

tiancaiamao commented Jun 14, 2022

/merge

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jun 14, 2022

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

DetailsCommit hash: eae342b

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 14, 2022
@tiancaiamao tiancaiamao removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2022
@ti-chi-bot ti-chi-bot merged commit ad1cb78 into pingcap:master Jun 14, 2022
@tiancaiamao tiancaiamao deleted the paging-unistore branch June 14, 2022 13:47
@sre-bot
Copy link
Contributor

sre-bot commented Jun 14, 2022

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-common-test 🔴 failed 2, success 9, total 11 19 min Existing failure
idc-jenkins-ci-tidb/common-test ✅ all 12 tests passed 9 min 37 sec Fixed
idc-jenkins-ci/integration-cdc-test 🟢 all 35 tests passed 25 min Existing passed
idc-jenkins-ci-tidb/integration-ddl-test 🟢 all 6 tests passed 6 min 16 sec Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2 🟢 all 28 tests passed 5 min 24 sec Existing passed
idc-jenkins-ci-tidb/tics-test 🟢 all 1 tests passed 5 min 14 sec Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1 🟢 all 26 tests passed 5 min 1 sec Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test 🟢 all 1 tests passed 3 min 16 sec Existing passed
idc-jenkins-ci-tidb/mybatis-test 🟢 all 1 tests passed 3 min 4 sec Existing passed
idc-jenkins-ci-tidb/plugin-test 🟢 build success, plugin test success 4min Existing passed

@tiancaiamao tiancaiamao mentioned this pull request Jun 22, 2022
Closed
24 tasks
@lcwangchao lcwangchao mentioned this pull request Apr 18, 2023
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@XuHuaiyu XuHuaiyu XuHuaiyu approved these changes

@wshwsh12 wshwsh12 wshwsh12 approved these changes

Assignees

No one assigned

Labels

release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

implement coprocessor paging protocol on unistore

6 participants

@tiancaiamao @ti-chi-bot @sre-bot @hawkingrei @XuHuaiyu @wshwsh12