Rewrite data file parser; Insert _tidb_rowid when needed; Update checkpoint structure #82

kennytm · 2018-11-01T05:15:12Z

What problem does this PR solve?

Completely fix TOOL-462, by recording the _tidb_rowid on non-PkIsHandle tables to ensure idempotence when importing the same chunk twice.

What is changed and how it works?

Assign a Row ID to every row of a table before import starts, so importing two chunks from the same table is no longer order-dependent.
To properly assign a Row ID, we need to know exactly how many rows each chunk has. So we replaced splitFuzzyRegion back to an exact version.
Since we need to read the whole file before importing, we want to make this step as fast as possible. Therefore, I replaced the MDDataReader by a ragel-based parser, which is about 8x faster on my machine.
We also need to record the RowIDs into the checkpoints. The checkpoint tables are modified to accommodate this change. Additionally, the checksums are stored as properties of a chunk instead of the whole table.
To ensure the only global property, the allocator, won't interfere with the data output in future updates, I've created a custom allocator which will panic on any unsupported operation.

Check List

Tests

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

Code changes

Has exported function/method change
Has exported variable/fields change
Has interface methods change
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility (only if you update Lightning after it saved a checkpoint)

Related changes

Need to cherry-pick to the release branch (2.1)
Need to update the tidb-ansible repository
Need to update the documentation
Need to be included in the release note

sre-bot · 2018-11-01T05:15:15Z

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

kennytm · 2018-11-01T05:16:40Z

/run-all-tests

The new lexer is 8x faster than MDDataReader. Speed is now a concern because we are going to read the entire file to get the accurate rows count per chunk.

kennytm · 2018-11-06T04:08:17Z

/run-all-tests

kennytm · 2018-11-06T07:12:22Z

PTAL @GregoryIan ~~(... throw dice ...) @csuzhangxc~~ @july2993

IANTHEREAL · 2018-11-07T11:58:17Z

lightning/restore/checkpoints.go

 }

 func (merger *ChunkCheckpointMerger) MergeInto(cpd *TableCheckpointDiff) {
-	cpd.hasChecksum = true
+	cpd.hasChunks = true
 	cpd.allocBase = merger.AllocBase


I always want to ask must it be it increasing

It should be. Adding a MaxInt64 call to ensure.

lightning/mydump/region.go

lightning/kv/sql2kv.go

lightning/kv/allocator.go

kennytm · 2018-11-07T15:45:45Z

/run-all-tests

lightning/mydump/region.go

lightning/restore/restore.go

lightning/mydump/parser.rl

IANTHEREAL · 2018-11-08T08:47:26Z

lightning/mydump/parser.rl

+back_quoted = '`' ^'`'* '`';
+unquoted = ^([,;()'"`] | space)+;
+
+row = '(' (^[)'"`] | single_quoted | double_quoted | back_quoted)* ')';


I'm not sure about it, maybe we need more test or descirptions

IANTHEREAL · 2018-11-09T04:02:51Z

@july2993 and I would review it again after we handle _row_id rebase

On first read, we will reset the allocator base it is the maximum of 1. the AUTO_INCREMENT option of the CREATE TABLE statement, or 2. the total number of rows This ensures future writes after importing will not clobber existing rows due to overlapping _tidb_rowid.

kennytm · 2018-11-09T17:42:18Z

I believe TiDB isn't going to accept any PR which doesn't handle the _tidb_rowid rebase in the distributed case (which is irrelevant to Lightning), so I explicitly rebase on the number of rows here instead.

kennytm · 2018-11-09T17:44:46Z

/ok-to-test

kennytm · 2018-11-09T17:46:36Z

/run-all-tests

tests/tidb_rowid/run.sh

IANTHEREAL · 2018-11-13T07:46:00Z

@july2993 PTAL

IANTHEREAL · 2018-11-14T06:57:33Z

@csuzhangxc @amyangfei PTAL

…tput

kennytm · 2018-11-14T12:19:07Z

/run-all-tests

lightning/mydump/parser.go

lightning/mydump/parser_test.go

lightning/mydump/region.go

lightning/mydump/region_test.go

csuzhangxc · 2018-11-19T16:36:32Z

lightning/restore/restore.go

+		start = time.Now()
+		kvs, _, err := kvEncoder.SQL2KV(sqls.String())
+		metrics.MarkTiming(encodeMark, start)
+		common.AppLogger.Debugf("len(kvs) %d, len(sql) %d", len(kvs), sqls.Len())


is sqls.Len() (number of bytes) for len(sql) significant?

Not sure if it's significant. This is just migration of existing code.

tidb-lightning/lightning/restore/restore.go

Lines 1115 to 1117 in 2ca7481

kvs, affectedRows, err := kvEncoder.SQL2KV(stmt)

metrics.MarkTiming(encodeMark, start)

common.AppLogger.Debugf("len(kvs) %d, len(sql) %d", len(kvs), len(stmt))

lightning/restore/checkpoints.go

kennytm · 2018-11-20T09:00:12Z

/run-integration-test

amyangfei

LGTM

csuzhangxc

LGTM

kennytm · 2018-11-20T14:31:24Z

/run-unit-test

kennytm mentioned this pull request Nov 1, 2018

*: added support for storing checkpoint at local filesystem #77

Closed

This comment has been minimized.

Sign in to view

kennytm force-pushed the kennytm/inject-tidb-rowid branch from 3d9dde9 to fd3d23d Compare November 6, 2018 03:17

kennytm added 4 commits November 6, 2018 11:20

mydump: added a ragel-based data file lexer and chunk parser

e48e549

The new lexer is 8x faster than MDDataReader. Speed is now a concern because we are going to read the entire file to get the accurate rows count per chunk.

mydump, restore: use the new parser for chunkRestore

761e8b9

restore, kv: replace the default ID allocator by a panicking allocator

efd1776

restore: include _tidb_rowid if this column is required

1b1f1c5

kennytm force-pushed the kennytm/inject-tidb-rowid branch from fd3d23d to ea5e94c Compare November 6, 2018 03:21

kennytm added 2 commits November 6, 2018 12:03

lightning: applied some missing go fmt change

e4ba06a

*: minor test fixes

47d5df6

kennytm force-pushed the kennytm/inject-tidb-rowid branch from ea5e94c to 47d5df6 Compare November 6, 2018 04:07

checkpoints: minor fix, ensure recorded alloc_base is increasing

38f87a5

IANTHEREAL reviewed Nov 7, 2018

View reviewed changes

lightning/mydump/region.go Outdated Show resolved Hide resolved

IANTHEREAL reviewed Nov 7, 2018

View reviewed changes

lightning/kv/sql2kv.go Outdated Show resolved Hide resolved

*: addressed comments

48cb1f7

IANTHEREAL reviewed Nov 7, 2018

View reviewed changes

lightning/kv/allocator.go Show resolved Hide resolved

tests: added a test case to ensure non-PK AUTO_INC works

5e7e455

IANTHEREAL reviewed Nov 8, 2018

View reviewed changes

lightning/mydump/region.go Show resolved Hide resolved

IANTHEREAL reviewed Nov 8, 2018

View reviewed changes

lightning/restore/restore.go Show resolved Hide resolved

IANTHEREAL reviewed Nov 8, 2018

View reviewed changes

lightning/mydump/parser.rl Show resolved Hide resolved

IANTHEREAL reviewed Nov 8, 2018

View reviewed changes

kennytm added 2 commits November 10, 2018 00:53

*: addressed comments

56e724a

IANTHEREAL reviewed Nov 13, 2018

View reviewed changes

tests/tidb_rowid/run.sh Outdated Show resolved Hide resolved

kennytm added the status/DNM Do not merge, test is failing or blocked by another PR label Nov 14, 2018

kennytm added 2 commits November 14, 2018 20:13

tests: move the exotic filename tests into its own folder

4b4670b

tests: place Lightning log into a file to avoid Jenkins truncating ou…

ed37dbe

…tput

kennytm force-pushed the kennytm/inject-tidb-rowid branch from ca1eab3 to ed37dbe Compare November 14, 2018 12:15

kennytm removed the status/DNM Do not merge, test is failing or blocked by another PR label Nov 14, 2018

kennytm mentioned this pull request Nov 15, 2018

mydump: added character-set config to control schema decoding behavior #83

Merged

csuzhangxc reviewed Nov 19, 2018

View reviewed changes

lightning/mydump/parser.go Outdated Show resolved Hide resolved

lightning/mydump/parser_test.go Outdated Show resolved Hide resolved

csuzhangxc reviewed Nov 19, 2018

View reviewed changes

lightning/mydump/region.go Outdated Show resolved Hide resolved

csuzhangxc reviewed Nov 19, 2018

View reviewed changes

lightning/mydump/region_test.go Show resolved Hide resolved

mydump: addressed comments

61a933c

csuzhangxc reviewed Nov 19, 2018

View reviewed changes

amyangfei reviewed Nov 20, 2018

View reviewed changes

lightning/restore/checkpoints.go Outdated Show resolved Hide resolved

checkpoints: addressed comment, turned table name into constant

f7da202

amyangfei reviewed Nov 20, 2018

View reviewed changes

csuzhangxc reviewed Nov 20, 2018

View reviewed changes

kennytm merged commit a7122c4 into master Nov 20, 2018

kennytm deleted the kennytm/inject-tidb-rowid branch November 20, 2018 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite data file parser; Insert _tidb_rowid when needed; Update checkpoint structure #82

Rewrite data file parser; Insert _tidb_rowid when needed; Update checkpoint structure #82

kennytm commented Nov 1, 2018 •

edited by amyangfei

Loading

sre-bot commented Nov 1, 2018

kennytm commented Nov 1, 2018

This comment has been minimized.

kennytm commented Nov 6, 2018

kennytm commented Nov 6, 2018 •

edited

Loading

IANTHEREAL Nov 7, 2018

kennytm Nov 7, 2018

kennytm commented Nov 7, 2018

IANTHEREAL Nov 8, 2018

IANTHEREAL commented Nov 9, 2018

kennytm commented Nov 9, 2018

kennytm commented Nov 9, 2018

kennytm commented Nov 9, 2018

IANTHEREAL commented Nov 13, 2018

IANTHEREAL commented Nov 14, 2018

kennytm commented Nov 14, 2018

csuzhangxc Nov 19, 2018

kennytm Nov 19, 2018

kennytm commented Nov 20, 2018

amyangfei left a comment

csuzhangxc left a comment

kennytm commented Nov 20, 2018

	kvs, affectedRows, err := kvEncoder.SQL2KV(stmt)
	metrics.MarkTiming(encodeMark, start)
	common.AppLogger.Debugf("len(kvs) %d, len(sql) %d", len(kvs), len(stmt))

Rewrite data file parser; Insert _tidb_rowid when needed; Update checkpoint structure #82

Rewrite data file parser; Insert _tidb_rowid when needed; Update checkpoint structure #82

Conversation

kennytm commented Nov 1, 2018 • edited by amyangfei Loading

What problem does this PR solve?

What is changed and how it works?

Check List

sre-bot commented Nov 1, 2018

kennytm commented Nov 1, 2018

This comment has been minimized.

kennytm commented Nov 6, 2018

kennytm commented Nov 6, 2018 • edited Loading

IANTHEREAL Nov 7, 2018

Choose a reason for hiding this comment

kennytm Nov 7, 2018

Choose a reason for hiding this comment

kennytm commented Nov 7, 2018

IANTHEREAL Nov 8, 2018

Choose a reason for hiding this comment

IANTHEREAL commented Nov 9, 2018

kennytm commented Nov 9, 2018

kennytm commented Nov 9, 2018

kennytm commented Nov 9, 2018

IANTHEREAL commented Nov 13, 2018

IANTHEREAL commented Nov 14, 2018

kennytm commented Nov 14, 2018

csuzhangxc Nov 19, 2018

Choose a reason for hiding this comment

kennytm Nov 19, 2018

Choose a reason for hiding this comment

kennytm commented Nov 20, 2018

amyangfei left a comment

Choose a reason for hiding this comment

csuzhangxc left a comment

Choose a reason for hiding this comment

kennytm commented Nov 20, 2018

kennytm commented Nov 1, 2018 •

edited by amyangfei

Loading

kennytm commented Nov 6, 2018 •

edited

Loading