backend/local: use range properties to optimize region range estimate #422

glorv · 2020-10-21T08:33:52Z

What problem does this PR solve?

Add a RangePropertiesCollector in pebble db so we can use the range properties to make region range estimation more precisely.

What is changed and how it works?

Since pebble support TablePropertyCollector . This pr implement RangePropertiesCollector like what implements in tikv at here.

~~After cockroachdb/pebble#963, we can fetch the user properties for each stable, so we can decode range properties from the user properties and use it as sample data to estimate range split.~~

There is a small difference in this pr compare to the implement in tikv-importer. Since we only write level 0 sst files and avoid compaction, our range properties are not as precise, so the split ranges are not as precise as what in tikv-importer. But In our benchmark with different datasets, this approach is good enough.

Benchmark Result:
This pr use the following three data set to benchmark lightning performance:

tpcc. 14k warehouse tpcc csv files generated by go-tpc.
zipf. 10 billion rows SQL files generated by dbgen with following template:

CREATE TABLE `test`.`dbgen` (
        "id"    INTEGER,
        /*{{ rownum }}*/
        "g_id"  INTEGER,
        /*{{ rand.zipf(26, 2) }}*/
        PRIMARY KEY ("g_id", "id")
);

sysbench. table with about 500 million rows generated by sysbench prepare --test=oltp_insert

The benchmark is running on a 40-cpu machine with source files on a nvme-ssd disk. The bench cluster is v4.0.6 tidb with 1tidb/1pd/6tikv.

The master branch is run on the current newest commit aa83de1692ad384c3d243132d7d0eb28a14bd986

Result:

Data Set	Code Branch	Time Cost
tpcc	master	2h25m
tpcc	range-property	2h25m
zipf	master	15h40m
zipf	range-property	3h23m
sysbench	master	46m
sysbench	range-property	34m

Check List

Tests

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

Side effects

Related changes

Release Note

Use range properties to make range split more precise with local backend

* add optional level for opst-restore operations * trim leading and suffix '" * use UnmarshalTOML to unmarshal post restore op level * resolve comments and fix unit test

* do not retry epochNotMatch error when ingest sst * add retry ingest for 'Raft raft: proposal dropped' error in ingest * change some retryable error log level from Error to Warn * fix nextKey * add a comment for nextKey * fix comment and add a unit test * wrap time.Sleep in select Co-authored-by: kennytm <kennytm@gmail.com>

…to range-property

glorv · 2020-10-21T08:34:33Z

Waiting for cockroachdb/pebble#963 to be merged, and add benchmark result

…to range-property

overvenus · 2020-10-29T04:14:36Z

lightning/backend/local.go

+	"github.com/google/btree"
+
+	"github.com/pingcap/tidb/util/hack"
+


Suggested change

"github.com/google/btree"

"github.com/pingcap/tidb/util/hack"

"github.com/google/btree"

"github.com/pingcap/tidb/util/hack"

lightning/backend/local.go

overvenus · 2020-10-29T04:15:55Z

lightning/backend/local.go

-	start = append([]byte{}, iter.Key()...)
-	iter.Last()
-	end = nextKey(iter.Key())
+type rangeProperty struct {


Please port tests from tikv.

kennytm · 2020-10-26T09:02:43Z

lightning/backend/local.go

+	"github.com/google/btree"
+
+	"github.com/pingcap/tidb/util/hack"
+
 	"github.com/cockroachdb/pebble"


delete some empty lines

lightning/backend/local.go

kennytm · 2020-10-29T04:12:38Z

lightning/backend/local.go

+	s.indexHandles.ReplaceOrInsert(item)
+}
+
+// iter the tree unit f return false


Suggested change

// iter the tree unit f return false

// iter the tree until f returns false

kennytm · 2020-10-29T04:13:20Z

lightning/backend/local.go

 	}

-	step := (eValue - sValue) / sampleCount
+	userProps[PROP_RANGE_INDEX] = string(hack.String(c.props.Encode()))


Suggested change

userProps[PROP_RANGE_INDEX] = string(hack.String(c.props.Encode()))

userProps[PROP_RANGE_INDEX] = string(c.props.Encode())

lightning/backend/local.go

overvenus · 2020-10-30T05:48:23Z

lightning/backend/localhelper.go

@@ -96,6 +91,11 @@ func (local *local) SplitAndScatterRegionByRanges(ctx context.Context, ranges []
 			})
 			minKey = retryKeys[0]
 			maxKey = nextKey(retryKeys[len(retryKeys)-1])
+			select {
+			case <-time.After(time.Second):


Why sleep 1 second？

There are some failures in split regions, so we should wait sometime to avoid retry too fast

lightning/backend/local_test.go

overvenus · 2020-10-30T05:52:33Z

lightning/backend/local_test.go

+	c.Assert(e.Size, Equals, uint64(defaultPropSizeIndexDistance+5))
+	i := props.get([]byte("i"))
+	c.Assert(i.Size, Equals, uint64(defaultPropSizeIndexDistance/8*17+9))
+	k := props.get([]byte("k"))
+	c.Assert(k.Size, Equals, uint64(defaultPropSizeIndexDistance/8*25+11))
+	m := props.get([]byte("m"))
+	c.Assert(m.Keys, Equals, uint64(defaultPropKeysIndexDistance+11))
+	n := props.get([]byte("n"))
+	c.Assert(n.Keys, Equals, uint64(defaultPropKeysIndexDistance*2+11))
+	o := props.get([]byte("o"))
+	c.Assert(o.Keys, Equals, uint64(defaultPropKeysIndexDistance*2+12))


Could you add some comments about these magic numbers?

These values are calculated from the input cases, just sum up each case up, not sure to put an expression here is necessary

And there are comments in the cases array with sizes/keys expressions for each range property here

overvenus · 2020-10-30T05:53:48Z

lightning/backend/local_test.go

@@ -31,3 +33,127 @@ func (s *localSuite) TestNextKey(c *C) {
 	next = nextKey([]byte{1, 255})
 	c.Assert(bytes.Compare(next, []byte{1, 255, 0, 1, 2}), Equals, -1)
 }
+
+func (s *localSuite) TestRangeProperties(c *C) {


Please add a link to the original test in TiKV.

overvenus · 2020-10-30T05:59:57Z

lightning/backend/local.go


-	sampleValues := make([]uint64, 0, sampleCount/10)
+type sizeProperties struct {


Could you remove sizeProperties? It has been deprecated.

tikv-import also depend this range properties to split range? https://github.com/tikv/importer/blob/release-4.0/src/import/prepare.rs#L78-L87

This rangeProperties is built from the regionProperties. the deprecated rangeProperties should be the RangePropertiesCollector used in early version tikv?

I mean sizeProperties, not rangeProperties.

sorry for my typo. both import and lightning use the size properties built from rangeProperties to split ranges into proper size. I think tikv has deprecated the SizePropertiesCollector in early version?

Actually, the sizeProperties here is just a merge of all the rangeProperties. The usage should be much different from tikv

overvenus

Please add a unit test that creating a pebble instance to test if property works. Rest LGTM

…to range-property

glorv · 2020-11-04T12:00:14Z

Please add a unit test that creating a pebble instance to test if property works. Rest LGTM

done. PTAL, again @overvenus

overvenus

LGTM

glorv · 2020-11-05T08:56:17Z

@kennytm PTAL again

kennytm

rest LGTM

lightning/backend/local.go

…to range-property

kennytm

LGTM please fix merge conflict

add notes save work save work fix unit test remove tidbMgr in RestoreController remove some comments remove some comments change logger in SQLWithRetry revert replace log.Logger to *zap.Logger dep: update uuid dependency to latest google/uuid (pingcap#452) * dep: update satori/go.uuid to latest * fix tests * change to google/uuid * fix build * try fix test * get familiar with google/uuid * address comment tidb-lightning-ctl: change default of -d to 'noop://' (pingcap#453) also add noop:// to supported storage types (to represent an empty store) replace tab to space try another port to fix CI remove some comment *: more glue restore: fix the bug that gc life time ttl does not take effect (pingcap#448) * fix gc ttl loop * resolve comment and add tests fix CI report info to host TiDB config: filter out all system schemas by default (pingcap#459) backend: fix auto random default value for primary key (pingcap#457) * fix auto generate auto random primary key column * fix default for auto random primary key * fix test * use prev row id for auto random and add a test * replace chunck with session opt * fix * fix mydumper: fix parquet data parser (pingcap#435) * fix parquet * reorder imports * fix test * use empty collation * fix a error and add more test cases * add pointer type tests * resolve comments Co-authored-by: kennytm <kennytm@gmail.com> address comment backend/local: use range properties to optimize region range estimate (pingcap#422) * use range propreties to estimate region range * post-restore: add optional level for post-restore operations (pingcap#421) * add optional level for opst-restore operations * trim leading and suffix '" * use UnmarshalTOML to unmarshal post restore op level * resolve comments and fix unit test * backend/local: do not retry epochNotMatch error when ingest sst (pingcap#419) * do not retry epochNotMatch error when ingest sst * add retry ingest for 'Raft raft: proposal dropped' error in ingest * change some retryable error log level from Error to Warn * fix nextKey * add a comment for nextKey * fix comment and add a unit test * wrap time.Sleep in select Co-authored-by: kennytm <kennytm@gmail.com> * update * use range properties to optimze region range estimate * update pebble * change the default value for batch-size * add unit tests and reslove comments * add a comment to range properties test * add a comment * add a test for range property with pebble * rename const variable Co-authored-by: kennytm <kennytm@gmail.com> fix pd service id is empty (pingcap#460) fix s3 parquet reader (pingcap#461) Co-authored-by: Neil Shen <overvenus@gmail.com> fix service gc ttl again (pingcap#465) address comment mydumper: verify file routing config (pingcap#470) * fix file routing * remove useless line * remove redundant if check rename a method in interface save work try fix CI could work change ctx usage try fix CI try fix CI refine function interface refine some fucntion interface debug CI address comment config: allow four byte-size config to be specified using human-readable units ("100 GiB") (pingcap#471) * Makefile: add `make finish-prepare` action * config: accept human-readable size for most byte-related config e.g. allow `region-split-size = '96M'` in additional to `= 100663296` (known issue: these values' precisions will be truncated to 53 bits instead of supporting all 63 bits) * restore: reduce chance of spurious errors from TestGcTTLManagerSingle Co-authored-by: glorv <glorvs@163.com> remove debug log test: change double type syntax (pingcap#474) address comment checkpoint: add glue checkpoint resolve cycle import expose Retry refine change interface to cope with TiDB fix SQL string fix SQL adjust interface to embedded in TiDB could import now reduce TLS restore: add `glue.Glue` interface and other function (pingcap#456) * save my work * add notes * save work * save work * fix unit test * remove tidbMgr in RestoreController * remove some comments * remove some comments * change logger in SQLWithRetry * revert replace log.Logger to *zap.Logger * replace tab to space * try another port to fix CI * remove some comment * *: more glue * report info to host TiDB * fix CI * address comment * address comment * rename a method in interface * save work * try fix CI * could work * change ctx usage * try fix CI * try fix CI * refine function interface * refine some fucntion interface * debug CI * address comment * remove debug log * address comment modify code add comment refine some code

* save my work add notes save work save work fix unit test remove tidbMgr in RestoreController remove some comments remove some comments change logger in SQLWithRetry revert replace log.Logger to *zap.Logger dep: update uuid dependency to latest google/uuid (#452) * dep: update satori/go.uuid to latest * fix tests * change to google/uuid * fix build * try fix test * get familiar with google/uuid * address comment tidb-lightning-ctl: change default of -d to 'noop://' (#453) also add noop:// to supported storage types (to represent an empty store) replace tab to space try another port to fix CI remove some comment *: more glue restore: fix the bug that gc life time ttl does not take effect (#448) * fix gc ttl loop * resolve comment and add tests fix CI report info to host TiDB config: filter out all system schemas by default (#459) backend: fix auto random default value for primary key (#457) * fix auto generate auto random primary key column * fix default for auto random primary key * fix test * use prev row id for auto random and add a test * replace chunck with session opt * fix * fix mydumper: fix parquet data parser (#435) * fix parquet * reorder imports * fix test * use empty collation * fix a error and add more test cases * add pointer type tests * resolve comments Co-authored-by: kennytm <kennytm@gmail.com> address comment backend/local: use range properties to optimize region range estimate (#422) * use range propreties to estimate region range * post-restore: add optional level for post-restore operations (#421) * add optional level for opst-restore operations * trim leading and suffix '" * use UnmarshalTOML to unmarshal post restore op level * resolve comments and fix unit test * backend/local: do not retry epochNotMatch error when ingest sst (#419) * do not retry epochNotMatch error when ingest sst * add retry ingest for 'Raft raft: proposal dropped' error in ingest * change some retryable error log level from Error to Warn * fix nextKey * add a comment for nextKey * fix comment and add a unit test * wrap time.Sleep in select Co-authored-by: kennytm <kennytm@gmail.com> * update * use range properties to optimze region range estimate * update pebble * change the default value for batch-size * add unit tests and reslove comments * add a comment to range properties test * add a comment * add a test for range property with pebble * rename const variable Co-authored-by: kennytm <kennytm@gmail.com> fix pd service id is empty (#460) fix s3 parquet reader (#461) Co-authored-by: Neil Shen <overvenus@gmail.com> fix service gc ttl again (#465) address comment mydumper: verify file routing config (#470) * fix file routing * remove useless line * remove redundant if check rename a method in interface save work try fix CI could work change ctx usage try fix CI try fix CI refine function interface refine some fucntion interface debug CI address comment config: allow four byte-size config to be specified using human-readable units ("100 GiB") (#471) * Makefile: add `make finish-prepare` action * config: accept human-readable size for most byte-related config e.g. allow `region-split-size = '96M'` in additional to `= 100663296` (known issue: these values' precisions will be truncated to 53 bits instead of supporting all 63 bits) * restore: reduce chance of spurious errors from TestGcTTLManagerSingle Co-authored-by: glorv <glorvs@163.com> remove debug log test: change double type syntax (#474) address comment checkpoint: add glue checkpoint resolve cycle import expose Retry refine change interface to cope with TiDB fix SQL string fix SQL adjust interface to embedded in TiDB could import now reduce TLS restore: add `glue.Glue` interface and other function (#456) * save my work * add notes * save work * save work * fix unit test * remove tidbMgr in RestoreController * remove some comments * remove some comments * change logger in SQLWithRetry * revert replace log.Logger to *zap.Logger * replace tab to space * try another port to fix CI * remove some comment * *: more glue * report info to host TiDB * fix CI * address comment * address comment * rename a method in interface * save work * try fix CI * could work * change ctx usage * try fix CI * try fix CI * refine function interface * refine some fucntion interface * debug CI * address comment * remove debug log * address comment modify code add comment refine some code * address comment * add some comments * fix CI and change CREATE TABLE

glorv and others added 6 commits October 16, 2020 16:47

use range propreties to estimate region range

95e71bd

post-restore: add optional level for post-restore operations (#421)

8e3471f

* add optional level for opst-restore operations * trim leading and suffix '" * use UnmarshalTOML to unmarshal post restore op level * resolve comments and fix unit test

update

617d476

use range properties to optimze region range estimate

6e6c55d

Merge branch 'master' of https://github.com/pingcap/tidb-lightning in…

4dc5e9d

…to range-property

glorv added the status/WIP Work in progress label Oct 21, 2020

kennytm added the status/DNM Do not merge, test is failing or blocked by another PR label Oct 21, 2020

glorv added 4 commits October 23, 2020 17:23

Merge branch 'master' of https://github.com/pingcap/tidb-lightning in…

c0db910

…to range-property

update pebble

309a51e

Merge branch 'master' of https://github.com/pingcap/tidb-lightning in…

ea0b613

…to range-property

change the default value for batch-size

9d3fcf5

glorv added status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/DNM Do not merge, test is failing or blocked by another PR status/WIP Work in progress labels Oct 26, 2020

glorv requested review from 3pointer and kennytm October 26, 2020 03:00

Merge branch 'master' into range-property

09d958f

overvenus reviewed Oct 29, 2020

View reviewed changes

kennytm reviewed Oct 29, 2020

View reviewed changes

bb7133 mentioned this pull request Oct 29, 2020

*: insufficient bytes decode value reported when adding index for some split common-handled tables. pingcap/tidb#20727

Closed

add unit tests and reslove comments

fcd453d

glorv force-pushed the range-property branch from 4d6a6f0 to fcd453d Compare October 30, 2020 04:17

overvenus reviewed Oct 30, 2020

View reviewed changes

glorv added 2 commits October 30, 2020 15:10

add a comment to range properties test

2f62a2a

add a comment

ea81558

overvenus added this to the 4.0.9 milestone Nov 4, 2020

overvenus reviewed Nov 4, 2020

View reviewed changes

add a test for range property with pebble

f5179f0

Merge branch 'master' of https://github.com/pingcap/tidb-lightning in…

2a00eba

…to range-property

overvenus previously approved these changes Nov 5, 2020

View reviewed changes

glorv added status/LGT1 One reviewer already commented LGTM (LGTM1) and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Nov 5, 2020

kennytm reviewed Nov 5, 2020

View reviewed changes

lightning/backend/local.go Outdated Show resolved Hide resolved

glorv mentioned this pull request Nov 9, 2020

Analyze table timeout in lightning #447

Closed

rename const variable

13d8a7a

glorv dismissed overvenus’s stale review via 13d8a7a November 9, 2020 12:31

Merge branch 'master' of https://github.com/pingcap/tidb-lightning in…

7dec62b

…to range-property

kennytm previously approved these changes Nov 9, 2020

View reviewed changes

glorv dismissed kennytm’s stale review via 7dec62b November 9, 2020 12:47

kennytm approved these changes Nov 9, 2020

View reviewed changes

Merge branch 'master' into range-property

13b13df

kennytm added status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) and removed status/LGT1 One reviewer already commented LGTM (LGTM1) labels Nov 9, 2020

Merge branch 'master' into range-property

642fe5b

glorv merged commit 51d0e7b into master Nov 10, 2020

glorv deleted the range-property branch November 10, 2020 02:11

tangenta mentioned this pull request Jan 20, 2021

alter table add index reports 'panic in the recoverable goroutine' pingcap/tidb#22453

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backend/local: use range properties to optimize region range estimate #422

backend/local: use range properties to optimize region range estimate #422

glorv commented Oct 21, 2020 •

edited

Loading

glorv commented Oct 21, 2020

overvenus Oct 29, 2020

overvenus Oct 29, 2020

glorv Oct 30, 2020

kennytm Oct 26, 2020

kennytm Oct 29, 2020

kennytm Oct 29, 2020

overvenus Oct 30, 2020

glorv Oct 30, 2020

overvenus Oct 30, 2020

glorv Oct 30, 2020 •

edited

Loading

overvenus Oct 30, 2020

glorv Oct 30, 2020

overvenus Oct 30, 2020

glorv Oct 30, 2020

overvenus Oct 30, 2020

glorv Oct 30, 2020

glorv Oct 30, 2020

overvenus left a comment

glorv commented Nov 4, 2020

overvenus left a comment

glorv commented Nov 5, 2020

kennytm left a comment

kennytm left a comment

		"github.com/google/btree"

		"github.com/pingcap/tidb/util/hack"

	// iter the tree unit f return false
	// iter the tree until f returns false

	userProps[PROP_RANGE_INDEX] = string(hack.String(c.props.Encode()))
	userProps[PROP_RANGE_INDEX] = string(c.props.Encode())


		sampleValues := make([]uint64, 0, sampleCount/10)
		type sizeProperties struct {

backend/local: use range properties to optimize region range estimate #422

backend/local: use range properties to optimize region range estimate #422

Conversation

glorv commented Oct 21, 2020 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

glorv commented Oct 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glorv Oct 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

overvenus left a comment

Choose a reason for hiding this comment

glorv commented Nov 4, 2020

overvenus left a comment

Choose a reason for hiding this comment

glorv commented Nov 5, 2020

kennytm left a comment

Choose a reason for hiding this comment

kennytm left a comment

Choose a reason for hiding this comment

glorv commented Oct 21, 2020 •

edited

Loading

glorv Oct 30, 2020 •

edited

Loading