*: Improve errors for mutation checker #30434

ekexium · 2021-12-06T07:09:52Z

The PR is based on #30310

What problem does this PR solve?

Issue Number: ref #26833

Problem Summary:

add error codes
redact log

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

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

Signed-off-by: ekexium <[email protected]>

ti-chi-bot · 2021-12-06T07:09:53Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

MyonKeminta
cfzjywxk

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: ekexium <[email protected]>

ekexium · 2021-12-06T08:55:58Z

/run-check_dev

executor/admin_test.go

table/tables/mutation_checker.go

Signed-off-by: ekexium <[email protected]>

ekexium · 2021-12-07T08:38:51Z

/run-check_dev

cfzjywxk · 2021-12-07T09:12:53Z

table/tables/mutation_checker.go

@@ -295,6 +302,7 @@ func collectTableMutationsFromBufferStage(t *TableCommon, memBuffer kv.MemBuffer
 // Returns error if the index data is not a subset of the input data.
 func compareIndexData(
 	sc *stmtctx.StatementContext, cols []*table.Column, indexData, input []types.Datum, indexInfo *model.IndexInfo,
+	tableName string,


We could pass table info directly as something related to the table could be referenced in the future for example if we want to print out if it is partitioned table etc.

I think the name is the only info we need at present, and it's easy to change the signature in the future if needed.
Anyway I've made this change. There's no big difference

Signed-off-by: ekexium <[email protected]>

cfzjywxk · 2021-12-13T07:16:38Z

@MyonKeminta PTAL

MyonKeminta · 2021-12-15T09:56:51Z

table/tables/mutation_checker.go

-				"inconsistent row mutation, row datum = {%v}, input datum = {%v}", decodedDatum.String(),
-				inputDatum.String(),
+				"inconsistent row mutation", zap.String("table", tableName),
+				logutil2.Redact(zap.String("decoded datum", decodedDatum.String())),


This Redact function looks like comes from BR and is not supposed to be used in code of TiDB's core. I thinks there should be another way to redact it..

And the table name should be redacted too

This Redact function looks like comes from BR and is not supposed to be used in code of TiDB's core. I thinks there should be another way to redact it..

Totally agree. I think the problem is that BR and TiDB maintain their own logging utilities. Those in BR should be moved/merged to TiDB's, which should be another PR.
I tried but didn't find something convenient in TiDB for log redaction. If we want clean dependencies, I'm afraid we have to copy these utilities?

Did you confirm if this function's behavior can be controlled by TiDB's configuration? Or can you try to print the error object you created instead, which seem to have built-in redacting?

Did you confirm if this function's behavior can be controlled by TiDB's configuration?

I think so. It's controlled by errors.RedactLogEnabled. I had a manual test.

Or can you try to print the error object you created instead, which seem to have built-in redacting?

Sounds like a good idea.

MyonKeminta · 2021-12-15T10:34:49Z

errno/errname.go

@@ -1009,6 +1009,9 @@ var MySQLErrName = map[uint16]*mysql.ErrMessage{
 	ErrInvalidIncrementAndOffset:     mysql.Message("Invalid auto_increment settings: auto_increment_increment: %d, auto_increment_offset: %d, both of them must be in range [1..65535]", nil),
 	ErrDataInconsistentMismatchCount: mysql.Message("data inconsistency in table: %s, index: %s, index-count:%d != record-count:%d", nil),
 	ErrDataInconsistentMismatchIndex: mysql.Message("data inconsistency in table: %s, index: %s, col: %s, handle: %#v, index-values:%#v != record-values:%#v, compare err:%#v", []int{3, 4, 5, 6}),
+	ErrInconsistentRowValue:          mysql.Message("writing inconsistent data in table: %s, expected-values:{%s} != record-values:{%s}", []int{1, 2}),


I think it would be hard to say which is the expected one

There's a mistake here, the arguments are passed in the wrong order. But I think the input of the AddRecords should be the expected data?

Signed-off-by: ekexium <[email protected]>

ekexium · 2021-12-16T06:05:50Z

/run-check_dev

ekexium · 2021-12-16T07:51:16Z

/run-check_dev

ekexium · 2021-12-16T08:03:32Z

/run-check_dev

MyonKeminta · 2021-12-16T10:06:01Z

table/tables/mutation_checker.go

-				"row handle = %v, index = %+v, row = %+v",
-				indexHandle, insertionHandle, m, rowInsertion)
+			err = ErrInconsistentHandle.GenWithStackByArgs(tableName, indexInfo.Name.O, indexHandle, insertionHandle, m, rowInsertion)
+			logutil.BgLogger().Error(err.Error())


Consider:

Suggested change

logutil.BgLogger().Error(err.Error())

logutil.BgLogger().Error("inconsistent handles in row and index insertions detected", zap.Error(err))

It would be better to use zap.Error to put the error to the log

What's the benefit?

It's the pattern that we always use. Usually the log message is a constant string and the other various information are provided in additional fields, wrapped with zap which makes it lazy-evaluated.

Signed-off-by: ekexium <[email protected]>

ekexium added 4 commits December 1, 2021 16:40

fix admin test

1f41bd3

Signed-off-by: ekexium <[email protected]>

fix gosec

c99a35f

Signed-off-by: ekexium <[email protected]>

satisfy gosec

92c91f0

Signed-off-by: ekexium <[email protected]>

add error codes for mutation checker; redact log if needed

3d1f75f

Signed-off-by: ekexium <[email protected]>

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 6, 2021

ekexium added 2 commits December 6, 2021 15:21

add comments

5b21b99

Signed-off-by: ekexium <[email protected]>

fix errors.toml

e9025ae

Signed-off-by: ekexium <[email protected]>

ekexium requested review from MyonKeminta, cfzjywxk and sticnarf December 6, 2021 09:21

cfzjywxk reviewed Dec 7, 2021

View reviewed changes

executor/admin_test.go Outdated Show resolved Hide resolved

cfzjywxk reviewed Dec 7, 2021

View reviewed changes

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

better redact log

3dd63d1

Signed-off-by: ekexium <[email protected]>

ekexium force-pushed the improve-errors branch from d8c9a99 to 3dd63d1 Compare December 7, 2021 04:31

Merge branch 'ft-data-inconsistency' into improve-errors

3b69007

ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 7, 2021

Merge branch 'ft-data-inconsistency' into improve-errors

3e117b8

cfzjywxk reviewed Dec 7, 2021

View reviewed changes

address comments

7a4948f

Signed-off-by: ekexium <[email protected]>

ekexium mentioned this pull request Dec 9, 2021

Defend against data inconsistency and improve its troubleshooting #26833

Open

8 tasks

cfzjywxk approved these changes Dec 13, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 13, 2021

MyonKeminta reviewed Dec 15, 2021

View reviewed changes

fix argument order

b6ccf0c

Signed-off-by: ekexium <[email protected]>

log error using redacted error object

8c188b6

Signed-off-by: ekexium <[email protected]>

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 16, 2021

ekexium force-pushed the improve-errors branch from 8f169db to 8c188b6 Compare December 16, 2021 08:44

ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 16, 2021

MyonKeminta reviewed Dec 16, 2021

View reviewed changes

modify log message

e894041

Signed-off-by: ekexium <[email protected]>

MyonKeminta approved these changes Dec 16, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 16, 2021

ekexium mentioned this pull request Dec 17, 2021

check mutation detects inconsistent index values when use expression-index #30809

Closed

ekexium merged commit 7f1b00c into pingcap:ft-data-inconsistency Dec 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: Improve errors for mutation checker #30434

*: Improve errors for mutation checker #30434

ekexium commented Dec 6, 2021 •

edited

Loading

ti-chi-bot commented Dec 6, 2021 •

edited

Loading

ekexium commented Dec 6, 2021

ekexium commented Dec 7, 2021

cfzjywxk Dec 7, 2021

ekexium Dec 7, 2021

cfzjywxk commented Dec 13, 2021

MyonKeminta Dec 15, 2021

MyonKeminta Dec 15, 2021

ekexium Dec 15, 2021 •

edited

Loading

MyonKeminta Dec 16, 2021

ekexium Dec 16, 2021

MyonKeminta Dec 15, 2021

ekexium Dec 15, 2021

ekexium commented Dec 16, 2021

ekexium commented Dec 16, 2021

ekexium commented Dec 16, 2021

MyonKeminta Dec 16, 2021

MyonKeminta Dec 16, 2021

ekexium Dec 16, 2021

MyonKeminta Dec 16, 2021

	logutil.BgLogger().Error(err.Error())
	logutil.BgLogger().Error("inconsistent handles in row and index insertions detected", zap.Error(err))

*: Improve errors for mutation checker #30434

*: Improve errors for mutation checker #30434

Conversation

ekexium commented Dec 6, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Dec 6, 2021 • edited Loading

ekexium commented Dec 6, 2021

ekexium commented Dec 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfzjywxk commented Dec 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekexium Dec 15, 2021 • 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

ekexium commented Dec 16, 2021

ekexium commented Dec 16, 2021

ekexium commented Dec 16, 2021

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

ekexium commented Dec 6, 2021 •

edited

Loading

ti-chi-bot commented Dec 6, 2021 •

edited

Loading

ekexium Dec 15, 2021 •

edited

Loading