executor: refactor union scan and dirty table #11702

crazycs520 · 2019-08-09T13:17:34Z

What problem does this PR solve?

background

In #10673, TiDB union-scan use memory index reader in the transaction.

TiDB has optimized in the update statement: TiDB won't write index key/value unless the index key/value changed. This optimization affects the union-scan implementation as follows:

In TiDB transaction mem-buffer, the row data may be inconsistent with the untouched index when you execute the update. Then simply using mem-index-reader and mem-index-look-up-reader trigger a bug. To avoid this problem, I have to use the original buildAndSortAddedRows function to avoid the corner case and use getMissIndexRowsByHandle to get the missed row(s) when using mem-index-reader.

The purpose of this PR

Remove the buildAndSortAddedRows function.
Remove the getMissIndexRowsByHandle function.
Add mem-index-lookup-reader.
Remove the redundant logic in DirtyTable.

How to implement?

If the update statement with the untouched index will also write the untouched index key/value to mem-buffer, Then everything of union-scan will be easy, Right?

So, In this PR, the update statement with the untouched index will also write the untouched index key/value to mem-buffer, but ignore those key/value when committing the transaction.

How to ignore the untouched index key/value when committing the transaction?

When writing the untouched index key/value to mem-buffer, I mark the index value as uncommitted, then check the type of the key is the index key and the value is marked to be uncommitted when committing the transaction.

How to mark the untouched index value?

The original value of no-unique-index is 0, and the value of the unique index is the handle ID
after marked the value to uncommitted,
The value of no-unique-index is 1, and the value of the unique index is the handle ID + '1'.

Drawback

As you can see, in this way, TiDB needs to write untouched index key/value to mem-buffer, This will introduce the encode and memory overhead.

Benchmark select in transaction

1 TiDB, 1 PD, 1 TiKV, deploy in 1 machine.

prepare

create table t (a int,b int,c int, index idxa(a,b));
insert into 10000 rows. [0,10000).

Best Case for mem index lookup reader:

There is no update with untouched index, so we can safely use the mem-index-lookup reader in the union scan.

insert 1000 rows in the transaction.

begin
insert 1000 rows. [10000,11000);
select * from t use index (idxa)  where a = 1; // Bench test index-look-up-reader of Uion-scan.

Before This PR:
1.8 ms. // This still uses the union-scan.BuildAndSort function:

This PR:
0.8ms

insert 10000 rows in the transaction.

begin
insert 1000 rows. [10000,20000);
select * from t use index (idxa)  where a = 1; // Bench test index-look-up-reader of Uion-scan.

Before This PR:
12 ms.

This PR:
0.8ms

Benchmark for mem-index-reader

Since the getMissIndexRowsByHandle function was removed, so the mem-index-reader also has improvement.

begin
insert 1000 rows. [10000,20000);
select sum(a) from t use index (idx) where a>10000;"

Before This PR:
0.6ms.

This PR:
0.52ms.

Drawback: Bench for update with untouched index.

update t set c=c+1; -- Update 10000 rows. and table t has 1 index was untouched.

Before This PR:
250ms.

This PR:
265ms.

What is changed and how it works?

Check List

Tests

Unit test

Code changes

Has exported function/method change

Side effects

Related changes

…r with full range

…ntouch-index

codecov · 2019-08-09T13:23:19Z

Codecov Report

Merging #11702 into master will decrease coverage by 0.0444%.
The diff coverage is 88.372%.

@@               Coverage Diff                @@
##             master     #11702        +/-   ##
================================================
- Coverage   81.5397%   81.4953%   -0.0445%     
================================================
  Files           450        449         -1     
  Lines         96456      95992       -464     
================================================
- Hits          78650      78229       -421     
+ Misses        12222      12189        -33     
+ Partials       5584       5574        -10

crazycs520 · 2019-08-09T13:23:27Z

/run-all-tests

crazycs520 · 2019-08-09T13:35:29Z

/run-unit-test

crazycs520 · 2019-08-09T13:38:03Z

/run-all-tests

crazycs520 · 2019-08-09T15:04:37Z

/bench

…ntouch-index

crazycs520 · 2019-08-10T17:07:46Z

/run-all-tests

crazycs520 · 2019-08-10T17:07:57Z

/bench

winkyao · 2019-08-11T00:39:13Z

@crazycs520 Need a benchmark for this PR.

…ntouch-index

into union-untouch-index

crazycs520 · 2019-09-09T12:46:46Z

/run-all-tests

store/tikv/2pc.go

table/tables/index.go

crazycs520 · 2019-09-09T15:20:28Z

/run-all-tests

codecov · 2019-09-09T15:27:28Z

Codecov Report

Merging #11702 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #11702   +/-   ##
===========================================
  Coverage   81.3349%   81.3349%           
===========================================
  Files           453        453           
  Lines         97369      97369           
===========================================
  Hits          79195      79195           
  Misses        12504      12504           
  Partials       5670       5670

jackysp · 2019-09-10T07:15:32Z

executor/mem_reader.go

+		belowHandleIndex: us.belowHandleIndex,
+	}
+
+	return &memIndexLookUpReader{


Why not embed memIndexReader to memIndexLookUpReader?

PhysicalIndexLookUpReader also use indexPlan and tablePlan separately.
And I think it is more clear.

table/tables/index.go

coocood · 2019-09-11T06:44:56Z

LGTM

ngaut · 2019-09-11T06:46:10Z

/bench

sre-bot · 2019-09-11T13:33:52Z

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: 213b783dc685a47f26dfc46a05730ed228afe6cc
+++ tidb: 0d10ddc2fcb5be76d2999138addfc70c8f93297c
tikv: 9b0e35d53adb819cfaad8f9299f4e132874f883d
pd: e678a1f5c022fed729fb79397fe02b6c9f54ff4a
================================================================================
test-1: < oltp_select_desc >
    * QPS : 11919.82 ± 0.3973% (std=38.61) delta: -2.49%
    * AvgMs : 64.43 ± 0.3983% (std=0.21) delta: 2.56%
    * PercentileMs99 : 124.19 ± 2.9116% (std=2.29) delta: 2.57%
            
test-2: < oltp_insert >
    * QPS : 21380.86 ± 0.2042% (std=27.34) delta: 0.85%
    * AvgMs : 11.96 ± 0.2089% (std=0.02) delta: -0.87%
    * PercentileMs99 : 42.61 ± 0.0000% (std=0.00) delta: 0.00%
            
test-3: < oltp_update_non_index >
    * QPS : 29453.83 ± 0.0125% (std=2.62) delta: -0.60%
    * AvgMs : 8.69 ± 0.0000% (std=0.00) delta: 0.63%
    * PercentileMs99 : 30.26 ± 0.0000% (std=0.00) delta: 0.00%
            
test-4: < oltp_read_write >
    * QPS : 37162.94 ± 0.6862% (std=158.49) delta: 0.34%
    * AvgMs : 138.32 ± 0.6854% (std=0.58) delta: -0.33%
    * PercentileMs99 : 257.95 ± 0.0000% (std=0.00) delta: 0.00%
            
test-5: < oltp_point_select >
    * QPS : 75474.33 ± 1.3469% (std=753.96) delta: -0.67%
    * AvgMs : 3.39 ± 1.2382% (std=0.03) delta: 0.77%
    * PercentileMs99 : 7.43 ± 0.0000% (std=0.00) delta: 0.00%
            
test-6: < oltp_update_index >
    * QPS : 17020.34 ± 0.6948% (std=71.58) delta: 0.56%
    * AvgMs : 15.04 ± 0.6783% (std=0.06) delta: -0.56%
    * PercentileMs99 : 48.34 ± 0.0000% (std=0.00) delta: 0.36%

https://perf.pingcap.com

jackysp

LGTM

sre-bot · 2019-09-12T01:44:55Z

Your auto merge job has been accepted, waiting for 12159

sre-bot · 2019-09-12T01:56:18Z

/run-all-tests

crazycs520 added 2 commits August 9, 2019 21:02

add update untouched index in txn context. todo: remove dirty table.

e2eae1a

remove get miss rows and add build and sort rows from mem table reade…

7b2f106

…r with full range

crazycs520 added status/WIP sig/execution SIG execution sig/transaction SIG:Transaction labels Aug 9, 2019

Merge branch 'master' of https://github.com/pingcap/tidb into union-u…

84f6896

…ntouch-index

remove dirty table and only keep deleted rows

2e5eb30

crazycs520 added 3 commits August 11, 2019 00:19

remove txn dirty table

54ddcfe

Merge branch 'master' of https://github.com/pingcap/tidb into union-u…

0e867e0

…ntouch-index

add comment

dce01eb

crazycs520 changed the title ~~executor: simplify union scan logic, remove get miss rows and try to remove dirty table.~~ executor: simplify union scan logic, remove get miss rows and remove dirty table. Aug 10, 2019

crazycs520 added 3 commits August 11, 2019 00:43

rename variable

c3e7dba

refine code

fa72c15

refine code

a88ef10

refine code

3bf59a7

crazycs520 added 2 commits August 13, 2019 11:03

Merge branch 'master' into union-untouch-index

6add367

Merge branch 'master' of https://github.com/pingcap/tidb into union-u…

a92aca2

…ntouch-index

sre-bot mentioned this pull request Aug 17, 2019

sessionctx/binloginfo: fix uncomment pre_split_regions ddl-querys in binlog #11762

Merged

crazycs520 closed this Aug 17, 2019

crazycs520 reopened this Aug 22, 2019

sre-bot added the closed/no-pr-quota label Aug 22, 2019

sre-bot closed this Aug 22, 2019

crazycs520 added 2 commits September 9, 2019 20:31

fmt code

4c13bd6

Merge branch 'union-untouch-index' of https://github.com/crazycs520/tidb

1f43d0f

into union-untouch-index

coocood reviewed Sep 9, 2019

View reviewed changes

store/tikv/2pc.go Outdated Show resolved Hide resolved

table/tables/index.go Outdated Show resolved Hide resolved

table/tables/index.go Outdated Show resolved Hide resolved

crazycs520 added 2 commits September 9, 2019 21:33

avoid alloc memory in mem-index-lookup-reader

bb6e242

address comment

b9bdaeb

jackysp reviewed Sep 10, 2019

View reviewed changes

table/tables/index.go Show resolved Hide resolved

update comment

0d10ddc

coocood added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 11, 2019

jackysp approved these changes Sep 12, 2019

View reviewed changes

jackysp added the status/can-merge Indicates a PR has been approved by a committer. label Sep 12, 2019

jackysp added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Sep 12, 2019

Merge branch 'master' into union-untouch-index

913dc73

sre-bot merged commit fb683be into pingcap:master Sep 12, 2019

coocood mentioned this pull request Sep 18, 2019

session: do not lock untouched unique key in pessimistic mode #12256

Merged

crazycs520 mentioned this pull request Oct 10, 2019

table: avoid write untouched index in auto commit and avoid get untouched index key from tikv #12609

Merged

crazycs520 mentioned this pull request Jan 6, 2020

executor: refactor union_scan executor by using txn_mem_buffer_reader (#10673) #14359

Merged

crazycs520 added a commit to crazycs520/tidb that referenced this pull request Jan 7, 2020

executor: refactor union scan and dirty table (pingcap#11702)

73f86a7

crazycs520 mentioned this pull request Jan 7, 2020

executor: refactor union scan and dirty table (#11702) (#14057) #14383

Merged

jackysp pushed a commit that referenced this pull request Jan 8, 2020

executor: refactor union scan and dirty table (#11702) (#14057) (#14383)

294f393

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: refactor union scan and dirty table #11702

executor: refactor union scan and dirty table #11702

crazycs520 commented Aug 9, 2019 •

edited

Loading

codecov bot commented Aug 9, 2019 •

edited

Loading

crazycs520 commented Aug 9, 2019

crazycs520 commented Aug 9, 2019

crazycs520 commented Aug 9, 2019

crazycs520 commented Aug 9, 2019

crazycs520 commented Aug 10, 2019

crazycs520 commented Aug 10, 2019

winkyao commented Aug 11, 2019

crazycs520 commented Sep 9, 2019

crazycs520 commented Sep 9, 2019

codecov bot commented Sep 9, 2019 •

edited

Loading

jackysp Sep 10, 2019

crazycs520 Sep 10, 2019

coocood commented Sep 11, 2019

ngaut commented Sep 11, 2019

sre-bot commented Sep 11, 2019

jackysp left a comment

sre-bot commented Sep 12, 2019

sre-bot commented Sep 12, 2019

executor: refactor union scan and dirty table #11702

executor: refactor union scan and dirty table #11702

Conversation

crazycs520 commented Aug 9, 2019 • edited Loading

What problem does this PR solve?

background

The purpose of this PR

How to implement?

Drawback

Benchmark select in transaction

Best Case for mem index lookup reader:

Benchmark for mem-index-reader

Drawback: Bench for update with untouched index.

What is changed and how it works?

Check List

codecov bot commented Aug 9, 2019 • edited Loading

Codecov Report

crazycs520 commented Aug 9, 2019

crazycs520 commented Aug 9, 2019

crazycs520 commented Aug 9, 2019

crazycs520 commented Aug 9, 2019

crazycs520 commented Aug 10, 2019

crazycs520 commented Aug 10, 2019

winkyao commented Aug 11, 2019

crazycs520 commented Sep 9, 2019

crazycs520 commented Sep 9, 2019

codecov bot commented Sep 9, 2019 • edited Loading

Codecov Report

jackysp Sep 10, 2019

Choose a reason for hiding this comment

crazycs520 Sep 10, 2019

Choose a reason for hiding this comment

coocood commented Sep 11, 2019

ngaut commented Sep 11, 2019

sre-bot commented Sep 11, 2019

jackysp left a comment

Choose a reason for hiding this comment

sre-bot commented Sep 12, 2019

sre-bot commented Sep 12, 2019

crazycs520 commented Aug 9, 2019 •

edited

Loading

codecov bot commented Aug 9, 2019 •

edited

Loading

codecov bot commented Sep 9, 2019 •

edited

Loading