*: Check index number in txn #29453

ekexium · 2021-11-04T10:21:14Z

What problem does this PR solve?

Issue Number: close #xxx

Problem Summary:

It should be able to prevent some cases when index insertion is unexpectedly missing.

What is changed and how it works?

It asserts that the number of index insertions must be a multiple of row insertions.

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

ti-chi-bot · 2021-11-04T10:21:16Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

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.

ekexium · 2021-11-10T08:30:25Z

/run-integration-tests

ekexium · 2021-11-11T08:44:48Z

I'm not quite sure whether DDL or tools may use transactions that don't satisfy the constraint.

cfzjywxk · 2021-11-11T13:02:40Z

I'm not quite sure whether DDL or tools may use transactions that don't satisfy the constraint.

Backfill transactions may lock the row keys and insert needed index keys, the lock operation would put zero value kv pairs into the memdb. If so the backfill transaction does not satisfy the rule, we may need to verify the current code or run a test about it.

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

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

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

ekexium · 2021-12-06T10:07:11Z

I'm not quite sure whether DDL or tools may use transactions that don't satisfy the constraint.

Backfill transactions may lock the row keys and insert needed index keys, the lock operation would put zero value kv pairs into the memdb. If so the backfill transaction does not satisfy the rule, we may need to verify the current code or run a test about it.

In such situation, #(row insertion) = 0, and we don't check the index counts.

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

ekexium · 2021-12-08T02:15:55Z

Theoretically I cannot find a case doesn't satisfy the rule at present.
I'd like to merge it and run regression tests with it, since it's sort of risky. @cfzjywxk @MyonKeminta

cfzjywxk · 2021-12-08T08:10:00Z

Theoretically I cannot find a case doesn't satisfy the rule at present. I'd like to merge it and run regression tests with it, since it's sort of risky. @cfzjywxk @MyonKeminta

We could limit the usage of this check to user transaction and skip internal ones by now

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

ekexium · 2021-12-08T14:27:04Z

Good idea.
I didn't find a clear indicator of a "user sql" so I used the condition ~~!internal && (isInsert || isUpdate || isDelete)~~. Please point it out if there is a better choice.

Update: That's nonsense. The check is performed in a commit statement.

cfzjywxk · 2021-12-09T08:52:54Z

Good idea. I didn't find a clear indicator of a "user sql" so I used the condition !internal && (isInsert || isUpdate || isDelete). Please point it out if there is a better choice.

@ekexium
We could use the connection id to check if this transaction is user one, inner session always has a zero zonnection id

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

cfzjywxk · 2021-12-13T07:17:40Z

table/tables/mutation_checker.go

+		return errors.Trace(err)
+	}
+	for tableID, count := range indexInsertionCount {
+		if rowInsertionCount[tableID] > 0 && count%rowInsertionCount[tableID] != 0 {


We could add some comments to explain the proof or invariant here.

cfzjywxk · 2021-12-13T07:18:06Z

@MyonKeminta PTAL

MyonKeminta

I'm actually still not very sure if this invariant is always true...

session/session.go

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

ekexium · 2021-12-15T11:05:56Z

I'm actually still not very sure if this invariant is always true...

Me too, so I would like to see it if can pass regression tests. Can the comments somewhat convince you?

MyonKeminta · 2021-12-15T11:31:37Z

I'm actually still not very sure if this invariant is always true...

Me too, so I would like to see it if can pass regression tests. Can the comments somewhat convince you?

At least I didn't come up with any counterexample..

MyonKeminta · 2021-12-15T11:50:43Z

table/tables/mutation_checker.go

+//        v no duplicates (index does not exist): PR + 1, PI + N
+//        v duplicated (0 puts, M dels): PR + 1, DI - M, PI + N
+// 			note: duplicated index mutations can only come from deletions on unique indices
+//    h is del


Does h does not exist and h is del indicates whether it exists in the current transaction's mem buffer?

Yep. h is del means it exists in the membuf and is a del.

MyonKeminta · 2021-12-15T12:21:34Z

table/tables/mutation_checker.go

+		}
+		return nil
+	}
+	if err := kv.WalkMemBuffer(memBuffer, f); err != nil {


Does it process entries that's not modified but pessimistic-locked? Can it work with this change?

Yes it will process the locked but not changed keys, as each unique index key used to fetch rowkey will be turned into a PUT record, seems they are not compatibe, consider this:

create table t1(c1 int key, c2 int, c3 int, unique key uk(c2), key k1(c3)); insert into t1 values(1, 1, 1),(2, 2, 2),(3, 3, 3); update t1 set c3 = c3 + 1 where c1 in (2, 3); // Use the batch point get executor. here the to be committed keys are: PUT uk 1->1 uk 2->2 uk 3->3 PUT sk 3_2 -> 2 PUT sk 4_3 -> 3 DEL sk 3_3 -> 3 DEL sk 2_2 -> 2 PUT rowkey 3-> 3, 3, 4 PUT rowkey 2-> 2, 2, 3

The number of PUT on row is 2 and the number of PUT on index is 5？

@ekexium
Seems @MyonKeminta has found a key point.

Right.. it's an intentional special case. It seems to me we can't simply tell whether it comes from the optimization. A workaround might be adding a flag to indicate the special usage so they can be ignored in the check.

The change is actually not expected and it's a temoprary solution to workaround the performance issues with many LOCK records in the write cf. Maybe we could put back this check after solving the LOCK record issue so that these tricky optimization or transformation could be removed.

I'm thinking if there is a method to distinguish the usage and won't block removing it.

If we have to add some new flags to membuffer to handle this, I would suspect whether it worths...

ti-chi-bot · 2021-12-17T08:29:22Z

@ekexium: PR needs rebase.

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 kubernetes/test-infra repository.

ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 4, 2021

ekexium changed the title *: Check in txn *: Check index number in txn Nov 4, 2021

ekexium mentioned this pull request Nov 10, 2021

Defend against data inconsistency and improve its troubleshooting #26833

Open

8 tasks

ekexium force-pushed the check-in-txn branch from a83c747 to 6afdac9 Compare November 10, 2021 08:26

ekexium marked this pull request as ready for review November 11, 2021 08:26

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 11, 2021

ekexium requested review from MyonKeminta and cfzjywxk November 11, 2021 08:29

ekexium added 2 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]>

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2021

ekexium added 6 commits December 2, 2021 11:31

satisfy gosec

92c91f0

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

assert that the number of index insertions must be a multiple of row …

98b9985

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

assert that the number of index insertions must be a multiple of row …

a448ac4

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

count for each table

e5545a0

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

comment CheckTxnConsistency

571a488

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

only check when InTxn is true

b985104

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

ekexium force-pushed the check-in-txn branch from 6afdac9 to b985104 Compare December 6, 2021 10:07

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 6, 2021

ekexium force-pushed the check-in-txn branch from b45fafc to d519b03 Compare December 6, 2021 10:36

add ErrIncorrectIndexCount

5cb41f9

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

ekexium force-pushed the check-in-txn branch from d519b03 to 5cb41f9 Compare December 7, 2021 04:36

Merge branch 'ft-data-inconsistency' into check-in-txn

d829187

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 7, 2021

only apply for certain SQLs

995a8e2

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

update enablement rule: use

909a7aa

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

ekexium force-pushed the check-in-txn branch from 4a0df52 to 909a7aa Compare December 9, 2021 09:19

fix a error message and a test

9e58e0b

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

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

ekexium mentioned this pull request Dec 13, 2021

[DNM] *: add some injections to simulate mutation errors #30666

Closed

12 tasks

MyonKeminta reviewed Dec 15, 2021

View reviewed changes

session/session.go Outdated Show resolved Hide resolved

address comments

bc408fd

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

MyonKeminta reviewed Dec 15, 2021

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2021

ti-chi-bot deleted the branch pingcap:ft-data-inconsistency February 11, 2022 05:51

ti-chi-bot closed this Feb 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: Check index number in txn #29453

*: Check index number in txn #29453

ekexium commented Nov 4, 2021 •

edited

Loading

ti-chi-bot commented Nov 4, 2021 •

edited

Loading

ekexium commented Nov 10, 2021

ekexium commented Nov 11, 2021

cfzjywxk commented Nov 11, 2021

ekexium commented Dec 6, 2021

ekexium commented Dec 8, 2021

cfzjywxk commented Dec 8, 2021

ekexium commented Dec 8, 2021 •

edited

Loading

cfzjywxk commented Dec 9, 2021 •

edited

Loading

cfzjywxk Dec 13, 2021

cfzjywxk commented Dec 13, 2021

MyonKeminta left a comment

ekexium commented Dec 15, 2021

MyonKeminta commented Dec 15, 2021

MyonKeminta Dec 15, 2021

ekexium Dec 15, 2021

MyonKeminta Dec 15, 2021

cfzjywxk Dec 15, 2021 •

edited

Loading

cfzjywxk Dec 15, 2021

ekexium Dec 15, 2021 •

edited

Loading

cfzjywxk Dec 15, 2021

ekexium Dec 15, 2021

MyonKeminta Dec 16, 2021

ti-chi-bot commented Dec 17, 2021

*: Check index number in txn #29453

*: Check index number in txn #29453

Conversation

ekexium commented Nov 4, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Nov 4, 2021 • edited Loading

ekexium commented Nov 10, 2021

ekexium commented Nov 11, 2021

cfzjywxk commented Nov 11, 2021

ekexium commented Dec 6, 2021

ekexium commented Dec 8, 2021

cfzjywxk commented Dec 8, 2021

ekexium commented Dec 8, 2021 • edited Loading

cfzjywxk commented Dec 9, 2021 • edited Loading

Choose a reason for hiding this comment

cfzjywxk commented Dec 13, 2021

MyonKeminta left a comment

Choose a reason for hiding this comment

ekexium commented Dec 15, 2021

MyonKeminta commented Dec 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfzjywxk Dec 15, 2021 • edited Loading

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

ti-chi-bot commented Dec 17, 2021

ekexium commented Nov 4, 2021 •

edited

Loading

ti-chi-bot commented Nov 4, 2021 •

edited

Loading

ekexium commented Dec 8, 2021 •

edited

Loading

cfzjywxk commented Dec 9, 2021 •

edited

Loading

cfzjywxk Dec 15, 2021 •

edited

Loading

ekexium Dec 15, 2021 •

edited

Loading