restore: check row value count to avoid unexpected encode result #528

glorv · 2020-12-18T09:47:46Z

What problem does this PR solve?

~~Check row field count before encodes, if row value count is bigger than table field count, directly return an error.~~

check column count in tidb encoder and return an error if column count doesn't match table column count
Be compatible with special field _tidb_rowid in getColumnNames and tidb encoder

What is changed and how it works?

Check List

Tests

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

Side effects

Related changes

Release Note

Fix the bug that tidb backend will panics if source file columns are more than target table columns

glorv · 2020-12-18T11:50:34Z

/run-all-tests

glorv · 2020-12-18T13:00:42Z

/run-all-tests

lance6716 · 2020-12-21T09:25:56Z

LGTM

kennytm

what bug does this PR fix?

lightning/restore/restore.go

glorv · 2020-12-22T02:34:28Z

what bug does this PR fix?

We met a case last week that the source file columns are more than the table schema, and this caused tidb backend panic at

tidb-lightning/lightning/backend/tidb.go

Line 251 in c2b6dde

if err := enc.appendSQL(&encoded, &field, cols[enc.columnIdx[i]]); err != nil {

.

And for local/importer backend, whether it can result in correct result depends on the implementation of tidb table encoder, thus it is undefined behavior.

…tning into check-read-row

glorv · 2020-12-23T09:42:21Z

To close this issue: #518

Not really. #531 should fix one cause of the tikv timeout in checksum/analyze phase. There is no corresponding issue for this one. We found it in an oncall issue though the root cause of that issue is user misconfiged the table schema.

kennytm · 2020-12-23T09:45:35Z

We met a case last week that the source file columns are more than the table schema, and this caused tidb backend panic at

tidb-lightning/lightning/backend/tidb.go

Line 251 in c2b6dde

if err := enc.appendSQL(&encoded, &field, cols[enc.columnIdx[i]]); err != nil {

.

maybe easier to check len(enc.columnIdx) >= len(row) in this file and also enc.columnIdx[i] >= 0.

or maybe do it inside initializeColumns and throw the unknownCols error.

And for local/importer backend, whether it can result in correct result depends on the implementation of tidb table encoder

pretty sure they will just be ignored.

we also need to support "ignore_extra_columns" in Fast Bulk Import i.e. it is legal to have a CSV in the form

name,info,ignored,address
Foo,Foo2,X,1234567
Bar,Bar2,Y,987654321

with the column "ignored" skipped.

glorv · 2020-12-23T11:37:35Z

ok, we could delay resolve this issue to the implementation of "ignore_extra_columns" since this is an ergent thing.

glorv · 2020-12-23T11:39:41Z

With a CSV without header, or it's an invalid csv with the fields in some line more than in header, maybe we still should check it in the tidb encoder

kennytm · 2020-12-23T19:38:06Z

i mean we should still check it, but we should do it initializeColumns or the TiDB Encoder, not in the encodeLoop.

glorv · 2020-12-24T02:49:15Z

i mean we should still check it, but we should do it initializeColumns or the TiDB Encoder, not in the encodeLoop.

got. I'll move it into the tidb encoder since tableKvEncoder can properly handle it.

…to check-read-row

glorv · 2020-12-24T07:25:12Z

@kennytm I have moved the check into tidb encoder, and fix an extra bug related to _tidb_rowid, PTAL again

…to check-read-row

kennytm

rest LGTM (and fix the test)

kennytm · 2020-12-24T16:07:57Z

lightning/backend/tidb.go

+	} else {
+		return cols[index]
+	}


Suggested change

} else {

return cols[index]

}

return cols[index]

strange that it is not linted

kennytm · 2020-12-24T16:09:27Z

lightning/backend/tidb.go

+	// See: tests/generated_columns/data/gencol.various_types.0.sql this sql has no columns, so encodeLoop will fill the
+	// column permutation with default, thus enc.columnCnt > len(row).
+	if len(row) > enc.columnCnt {
+		log.L().Error("column count mismatch", zap.Ints("column_permutation", columnPermutation),


Suggested change

log.L().Error("column count mismatch", zap.Ints("column_permutation", columnPermutation),

logger.Error("column count mismatch", zap.Ints("column_permutation", columnPermutation),

tests/tidb_rowid/run.sh

glorv · 2020-12-25T02:49:33Z

@lance6716 PTAL again, since it has changed a log.

lance6716

rest LGTM

lightning/backend/tidb.go

kennytm · 2020-12-25T06:38:19Z

/run-all-tests

TiDB crashed?!

[2020-12-25T03:56:45.752Z] [mysql] 2020/12/25 11:56:45 packets.go:36: unexpected EOF

[2020-12-25T03:56:45.752Z] [mysql] 2020/12/25 11:56:45 connection.go:135: write tcp 127.0.0.1:27016->127.0.0.1:4000: write: broken pipe

[2020-12-25T03:56:48.274Z] [mysql] 2020/12/25 11:56:48 packets.go:122: closing bad idle connection: EOF

[2020-12-25T03:56:48.275Z] [mysql] 2020/12/25 11:56:48 connection.go:135: write tcp 127.0.0.1:27018->127.0.0.1:4000: write: broken pipe

[2020-12-25T03:56:48.275Z] [mysql] 2020/12/25 11:56:48 packets.go:122: closing bad idle connection: EOF

[2020-12-25T03:56:48.275Z] [mysql] 2020/12/25 11:56:48 connection.go:135: write tcp 127.0.0.1:27036->127.0.0.1:4000: write: broken pipe

[2020-12-25T03:56:48.275Z] Error: read checkpoint failed: begin transaction failed: dial tcp 127.0.0.1:4000: connect: connection refused

glorv · 2020-12-25T06:56:09Z

Still need to resolve the integration test tidb_rowid first.

lance6716 · 2020-12-29T03:48:06Z

(LGTM, bot has already count my lgtm)

…tning into check-read-row

glorv · 2021-01-06T02:39:51Z

@kennytm PTAL again

kennytm · 2021-01-06T08:08:57Z

/lgtm

* check row value count to avoid unexpected encode result * check the '_tidb_row_id' field * resolve comments * fix issue related to '_tidb_rowid' and move column count to tidb encoder * add tidb_opt_write_row_id session var * fix test * resolve comments * update tidb to apply tidb#22062 * fix test

glorv added the type/bug-fix Bug fix label Dec 18, 2020

glorv requested review from kennytm and lance6716 December 18, 2020 09:47

check row value count to avoid unexpected encode result

9b63146

glorv force-pushed the check-read-row branch from c95b6d3 to 9b63146 Compare December 18, 2020 09:50

check the '_tidb_row_id' field

324954a

ti-srebot added the status/LGT1 One reviewer already commented LGTM (LGTM1) label Dec 21, 2020

kennytm reviewed Dec 21, 2020

View reviewed changes

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

glorv added 3 commits December 22, 2020 10:34

Merge branch 'master' into check-read-row

8c78d98

resolve comments

df51e9d

Merge branch 'check-read-row' of https://github.com/pingcap/tidb-ligh…

47240af

…tning into check-read-row

Merge branch 'master' into check-read-row

27d7448

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

72de282

…to check-read-row

fix issue related to '_tidb_rowid' and move column count to tidb encoder

e104101

glorv force-pushed the check-read-row branch from b6e33cc to e104101 Compare December 24, 2020 08:21

glorv added 3 commits December 24, 2020 16:50

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

cbb6242

…to check-read-row

add tidb_opt_write_row_id session var

f24b6fa

fix test

76b2023

kennytm reviewed Dec 24, 2020

View reviewed changes

lance6716 reviewed Dec 25, 2020

View reviewed changes

lightning/backend/tidb.go Show resolved Hide resolved

resolve comments

551ce93

glorv force-pushed the check-read-row branch from 588e5b6 to 551ce93 Compare December 25, 2020 03:53

glorv added 4 commits January 5, 2021 18:32

update tidb to apply tidb#22062

2e6f2c7

Merge branch 'master' into check-read-row

34ea98c

fix test

ddad685

Merge branch 'check-read-row' of https://github.com/pingcap/tidb-ligh…

5c4f9f7

…tning into check-read-row

ti-srebot added status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) and removed status/LGT1 One reviewer already commented LGTM (LGTM1) labels Jan 6, 2021

ti-srebot approved these changes Jan 6, 2021

View reviewed changes

kennytm merged commit 1709cc7 into master Jan 6, 2021

kennytm deleted the check-read-row branch January 6, 2021 08:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

restore: check row value count to avoid unexpected encode result #528

restore: check row value count to avoid unexpected encode result #528

glorv commented Dec 18, 2020 •

edited

Loading

glorv commented Dec 18, 2020

glorv commented Dec 18, 2020

lance6716 commented Dec 21, 2020

kennytm left a comment

glorv commented Dec 22, 2020

glorv commented Dec 23, 2020

kennytm commented Dec 23, 2020 •

edited

Loading

glorv commented Dec 23, 2020

glorv commented Dec 23, 2020 •

edited

Loading

kennytm commented Dec 23, 2020

glorv commented Dec 24, 2020 •

edited

Loading

glorv commented Dec 24, 2020

kennytm left a comment

kennytm Dec 24, 2020

kennytm Dec 24, 2020

glorv commented Dec 25, 2020

lance6716 left a comment

kennytm commented Dec 25, 2020

glorv commented Dec 25, 2020

lance6716 commented Dec 29, 2020

glorv commented Jan 6, 2021

kennytm commented Jan 6, 2021

	log.L().Error("column count mismatch", zap.Ints("column_permutation", columnPermutation),
	logger.Error("column count mismatch", zap.Ints("column_permutation", columnPermutation),

restore: check row value count to avoid unexpected encode result #528

restore: check row value count to avoid unexpected encode result #528

Conversation

glorv commented Dec 18, 2020 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

glorv commented Dec 18, 2020

glorv commented Dec 18, 2020

lance6716 commented Dec 21, 2020

kennytm left a comment

Choose a reason for hiding this comment

glorv commented Dec 22, 2020

glorv commented Dec 23, 2020

kennytm commented Dec 23, 2020 • edited Loading

glorv commented Dec 23, 2020

glorv commented Dec 23, 2020 • edited Loading

kennytm commented Dec 23, 2020

glorv commented Dec 24, 2020 • edited Loading

glorv commented Dec 24, 2020

kennytm left a comment

Choose a reason for hiding this comment

kennytm Dec 24, 2020

Choose a reason for hiding this comment

kennytm Dec 24, 2020

Choose a reason for hiding this comment

glorv commented Dec 25, 2020

lance6716 left a comment

Choose a reason for hiding this comment

kennytm commented Dec 25, 2020

glorv commented Dec 25, 2020

lance6716 commented Dec 29, 2020

glorv commented Jan 6, 2021

kennytm commented Jan 6, 2021

glorv commented Dec 18, 2020 •

edited

Loading

kennytm commented Dec 23, 2020 •

edited

Loading

glorv commented Dec 23, 2020 •

edited

Loading

glorv commented Dec 24, 2020 •

edited

Loading