dump: always split TiDB v4.* tables through tidb rowid to save TiDB's memory #273
Conversation
|
|
||
| dataTypeNumArr := []string{ | ||
| "INTEGER", "BIGINT", "TINYINT", "SMALLINT", "MEDIUMINT", | ||
| "INT", "INT1", "INT2", "INT3", "INT8", |
There was a problem hiding this comment.
here we forget an int symbol INT4
So I doubt if we have list them totally 🤔 we may ask parser guys for help
There was a problem hiding this comment.
Update comments in 69c1e7c
| colTypeRowReceiverMap[s] = SQLTypeNumberMaker | ||
| } | ||
| for _, s := range dataTypeBin { | ||
| for _, s := range dataTypeBinArr { | ||
| dataTypeBin[s] = struct{}{} | ||
| colTypeRowReceiverMap[s] = SQLTypeBytesMaker |
There was a problem hiding this comment.
not sure if WriteToBuffer of SQLTypeBytes need escaping quote
There was a problem hiding this comment.
SQLTypeBytes will transfer binary data to base 16 code data, so I think we won't have this problem.
https://github.com/pingcap/dumpling/blob/bff60f8/v4/export/sql_type.go#L284
| } | ||
|
|
||
| func extractTiDBRowIDFromDecodedKey(indexField, key string) (string, error) { | ||
| if p := strings.Index(key, indexField); p != -1 { |
There was a problem hiding this comment.
do we have a system test, to get a report when TiDB changes the behaviour 😵
There was a problem hiding this comment.
I have tested TiDB v4.0.0 and v4.0.12. In TiDB v4.x versions, they work well. I can add a test to make sure TiDB won't change this.
v4/export/sql_type.go
Outdated
| "TIMESTAMP", "DATETIME", "DATE", "TIME", "YEAR", "SQL_TSI_YEAR", | ||
| "TEXT", "TINYTEXT", "MEDIUMTEXT", "LONGTEXT", | ||
| "ENUM", "SET", "JSON", "NULL", "VAR_STRING", | ||
| "GEOMETRY", // TODO: support GEOMETRY later |
There was a problem hiding this comment.
move this to dataTypeBinArr.
v4/export/sql.go
Outdated
| @@ -458,7 +470,8 @@ func GetSpecifiedColumnValue(rows *sql.Rows, columnName string) ([]string, error | |||
| strs = append(strs, oneRow[fieldIndex].String) | |||
| } | |||
| } | |||
| return strs, nil | |||
| rows.Close() | |||
There was a problem hiding this comment.
with that defer you're closing rows twice 🤔
| if len(partitions) == 0 { | ||
| handleColNames, handleVals, err = selectTiDBTableRegion(tctx, conn, db, tbl) | ||
| } else { | ||
| return d.concurrentDumpTiDBPartitionTables(tctx, conn, meta, taskChan, partitions) |
There was a problem hiding this comment.
weak review: could we not return concurrentDumpTiDBPartitionTables, instead, return enough data and reuse below sendConcurrentDumpTiDBTasks?
There was a problem hiding this comment.
Maybe we can do that later
|
/lgtm |
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by writing |
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: 55f9278
|
|
/cherrypick release-5.0 |
|
@lichunzhu: new pull request created: #280. In response to this:
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 ti-community-infra/tichi repository. |
What problem does this PR solve?
fix #104 and close #278
What is changed and how it works?
Split chunks by
PKIsHandlefor TiDB v4.0.*. Make sure TiDB won't OOM through splitting tables by rowids.Check List
Tests
Test dump with v4.0.0. The result is looks expected.
Related changes
Release note