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

*: Make use of the upperBound of ticlient's kv_scan interface to ensure no overbound scan will happen #8081

Merged
merged 22 commits into from
Nov 9, 2018

Conversation

MyonKeminta
Copy link
Contributor

@MyonKeminta MyonKeminta commented Oct 28, 2018

What problem does this PR solve?

When TiDB invokes kv_scan interface of TiKV, it provides an start_key and a limit (which means at most how many keys are retrieved in one RPC call). However, it may scan to a range that is already deleted.
The data in there ranges is undefined, which may cause some trouble. This PR tries to add an end_key limit to it.

What is changed and how it works?

This PR added end_key limit to the kv scan interface, and added the upper_bound parameter to the scanner of tikv client.

Meanwhile, the Seek method of Retriever interface returns an Iterator. The end_key limit is perfectly suitable to be passed when an Iterator is creating, however it weird that the end_key parameter occurs in an function named Seek. Since it returns an Iterator, I think it's better to be renamed to Iter ratherthan Seek. In fact, the changes are just similar to #7581 .

This PR depends on pingcap/kvproto#306 , tikv/tikv#3720 and #8178 . Also, this PR will be separated into multiple after merging the PR of kvproto.

Check List

  • Unit test (TODO)

Code changes

  • Has interface methods change
  • Seek and SeekReverse of Retriever are renamed to Iter and IterReverse

Side effects

None

Related changes

  • Need to be included in the release note (maybe)

kv/memdb_buffer.go Outdated Show resolved Hide resolved
Signed-off-by: MyonKeminta <[email protected]>
Signed-off-by: MyonKeminta <[email protected]>
@MyonKeminta
Copy link
Contributor Author

/run-integration-tests

@MyonKeminta
Copy link
Contributor Author

/run-integration-common-test tikv=pr/3720

@@ -154,7 +154,7 @@ func (dr *delRange) doTask(ctx sessionctx.Context, r util.DelRangeTask) error {
finish := true
dr.keys = dr.keys[:0]
err := kv.RunInNewTxn(dr.store, false, func(txn kv.Transaction) error {
iter, err := txn.Seek(oldStartKey)
iter, err := txn.Iter(oldStartKey, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

r.EndKey?

@@ -199,7 +202,7 @@ func (s *Scanner) getData(bo *Backoffer) error {
// No more data in current Region. Next getData() starts
// from current Region's endKey.
s.nextStartKey = loc.EndKey
if len(loc.EndKey) == 0 {
if len(loc.EndKey) == 0 || (len(s.endKey) > 0 && kv.Key(s.nextStartKey).Cmp(kv.Key(s.endKey)) >= 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you want to check endKey in Scanner.Next() too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the underlying TiKV or mock TiKV here should promise that it doesn't return any keys that >= endKey here, so I didn't do any more check. How do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then TiDB can't work with older TiKV normally. Do you think that's ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. I'd like to say it is acceptable -- it won't break anything because current usages do not rely on the iterator to stop at upperBound. But it could be more safe to check bound in Next.

@@ -240,7 +240,7 @@ func (c *index) Delete(sc *stmtctx.StatementContext, m kv.Mutator, indexedValues

// Drop removes the KV index from store.
func (c *index) Drop(rm kv.RetrieverMutator) error {
it, err := rm.Seek(c.prefix)
it, err := rm.Iter(c.prefix, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do wee need to set endKey to TableIndexPrefix(index.ID+1)?

@@ -238,7 +238,7 @@ func (t *TxStructure) HClear(key []byte) error {

func (t *TxStructure) iterateHash(key []byte, fn func(k []byte, v []byte) error) error {
dataPrefix := t.hashDataKeyPrefix(key)
it, err := t.reader.Seek(dataPrefix)
it, err := t.reader.Iter(dataPrefix, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to set endKey to dataPrefix.PrefixNext()?

@@ -782,7 +782,7 @@ func (t *tableCommon) buildIndexForRow(ctx sessionctx.Context, rm kv.RetrieverMu
// IterRecords implements table.Table IterRecords interface.
func (t *tableCommon) IterRecords(ctx sessionctx.Context, startKey kv.Key, cols []*table.Column,
fn table.RecordIterFunc) error {
it, err := ctx.Txn().Seek(startKey)
it, err := ctx.Txn().Iter(startKey, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to set endKey to table's index prefix?

@@ -912,7 +912,7 @@ func (t *tableCommon) RebaseAutoID(ctx sessionctx.Context, newBase int64, isSetS
// Seek implements table.Table Seek interface.
func (t *tableCommon) Seek(ctx sessionctx.Context, h int64) (int64, bool, error) {
seekKey := tablecodec.EncodeRowKeyWithHandle(t.physicalTableID, h)
iter, err := ctx.Txn().Seek(seekKey)
iter, err := ctx.Txn().Iter(seekKey, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to set endKey to table's index prefix?

@@ -26,7 +26,7 @@ import (

// ScanMetaWithPrefix scans metadata with the prefix.
func ScanMetaWithPrefix(retriever kv.Retriever, prefix kv.Key, filter func(kv.Key, []byte) bool) error {
iter, err := retriever.Seek(prefix)
iter, err := retriever.Iter(prefix, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Set endKey to prefix.PrefixNext()?

@@ -56,7 +56,7 @@ func ScanMetaWithPrefix(retriever kv.Retriever, prefix kv.Key, filter func(kv.Ke
// DelKeyWithPrefix deletes keys with prefix.
func DelKeyWithPrefix(rm kv.RetrieverMutator, prefix kv.Key) error {
var keys []kv.Key
iter, err := rm.Seek(prefix)
iter, err := rm.Iter(prefix, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Set endKey to prefix.PrefixNext()?

ddl/index.go Outdated Show resolved Hide resolved
ddl/index.go Outdated Show resolved Hide resolved
}
// Iter creates an Iterator.
func (m *memDbBuffer) Iter(k Key, upperBound Key) (Iterator, error) {
i := &memDbIter{iter: m.db.NewIterator(&util.Range{Start: []byte(k), Limit: []byte(upperBound)}), reverse: false}
Copy link
Contributor

Choose a reason for hiding this comment

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

How about k or upperBound is nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's ok. If k is nil then []byte(k) is still nil.

@MyonKeminta
Copy link
Contributor Author

MyonKeminta commented Nov 6, 2018

@disksing It seems those usages of Seek can be updated in more PRs in the future, or this PR may change too much logic, which causes more difficulty in reviewing.

@MyonKeminta MyonKeminta changed the title store/tikv: Add end_key limit to kv_scan interface *: Make use of the upperBound of ticlient's kv_scan interface to ensure no overbound scan will happen Nov 8, 2018
@MyonKeminta
Copy link
Contributor Author

/run-all-tests

@MyonKeminta
Copy link
Contributor Author

@disksing @tiancaiamao PTAL

@MyonKeminta
Copy link
Contributor Author

@winkyao PTAL

@MyonKeminta
Copy link
Contributor Author

Is there any one who can help me remove the DNM tag? 😭

@shenli
Copy link
Member

shenli commented Nov 8, 2018

/run-all-tests

@disksing
Copy link
Contributor

disksing commented Nov 9, 2018

LGTM.

@disksing disksing added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 9, 2018
@MyonKeminta
Copy link
Contributor Author

/run-unit-test

@tiancaiamao
Copy link
Contributor

LGTM @winkyao

@tiancaiamao tiancaiamao added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label Nov 9, 2018
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. status/all tests passed and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 9, 2018
@MyonKeminta
Copy link
Contributor Author

if there's no more problems , is there any one who can help me merge it?

@zz-jason zz-jason merged commit 09c6bff into pingcap:master Nov 9, 2018
@MyonKeminta MyonKeminta deleted the misono/kv-scan-end-key branch November 9, 2018 11:34
MyonKeminta added a commit to MyonKeminta/tidb that referenced this pull request Nov 9, 2018
zz-jason pushed a commit that referenced this pull request Nov 12, 2018
MyonKeminta added a commit to MyonKeminta/tidb that referenced this pull request Nov 14, 2018
…re no overbound scan will happen (pingcap#8081)

Signed-off-by: MyonKeminta <[email protected]>
winkyao pushed a commit that referenced this pull request Nov 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/tikv priority/release-blocker This issue blocks a release. Please solve it ASAP. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants