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

*: support paging protocol on unistore #35244

Merged
merged 12 commits into from
Jun 14, 2022

Conversation

tiancaiamao
Copy link
Contributor

@tiancaiamao tiancaiamao commented Jun 8, 2022

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

[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.

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
@sre-bot
Copy link
Contributor

sre-bot commented Jun 9, 2022

@tiancaiamao
Copy link
Contributor Author

/run-unit-test

@tiancaiamao
Copy link
Contributor Author

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

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

@tiancaiamao
Copy link
Contributor Author

/run-unit-test

@hawkingrei
Copy link
Member

/run-unit-test

1 similar comment
@tiancaiamao
Copy link
Contributor Author

/run-unit-test

@tiancaiamao
Copy link
Contributor Author

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

@tiancaiamao: PR needs rebase.

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.

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
}
if req.Tp != kv.ReqTypeDAG {
// coprocessor request but type is not DAG
req.Paging = false
Copy link
Contributor

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

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

/merge

@ti-chi-bot
Copy link
Member

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

Commit 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

implement coprocessor paging protocol on unistore
6 participants