Support setting assertions and receiving assertion errors for Prewrite requests #311

MyonKeminta · 2021-09-24T09:30:49Z

This PR is part of pingcap/tidb#26833 and is a continuation of #273 .

sticnarf · 2021-09-26T03:12:26Z

txnkv/transaction/2pc.go

 	isPessimisticLock []bool
+	isAssertExist     []bool
+	isAssertNotExist  []bool


It looks a bit wasteful to me... Maybe also define "flags" here as the lowest 3 bits of UserData?

sticnarf · 2021-09-26T03:16:07Z

txnkv/transaction/2pc.go

 func (m *memBufferMutations) Slice(from, to int) CommitterMutations {
 	return &memBufferMutations{
 		handles: m.handles[from:to],
 		storage: m.storage,
 	}
 }

-func (m *memBufferMutations) Push(op kvrpcpb.Op, isPessimisticLock bool, handle unionstore.MemKeyHandle) {
-	aux := uint16(op) << 1
+func (m *memBufferMutations) Push(op kvrpcpb.Op, isPessimisticLock bool, assertExist, assertNotExist bool, handle unionstore.MemKeyHandle) {


I think it is better to add comments about the format of aux and add unit tests for it.

Ah.. sorry, this PR is not ready yet and it was supposed to be a draft PR, and currently this PR only did some minor changes based on #273 .

sticnarf · 2021-09-26T03:21:35Z

txnkv/transaction/2pc.go

+			ts, err1 := c.store.GetTimestampWithRetry(retry.NewBackofferWithVars(ctx, TsoMaxBackoff, c.txn.vars), c.txn.GetScope())
+			if err1 == nil {
+				_, _, err2 := c.checkSchemaValid(ctx, ts, c.txn.schemaVer, false)
+				if err2 != nil {
+					err = err2
+				}
+			}


I guess it is to avoid some false positives with online DDL? Could you add some comments?

disksing · 2021-10-20T04:08:37Z

Closing for no activity. Please reopen after ready.

MyonKeminta · 2021-10-20T04:11:07Z

Closing for no activity. Please reopen after ready.

Even it's using Github's draft PR functionality?

disksing · 2021-10-20T04:24:47Z

sorry, I misunderstood the time of last commit

Signed-off-by: lysu <sulifx@gmail.com> Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta · 2021-12-15T04:49:53Z

txnkv/transaction/txn.go

+				// TODO: Check if it's safe to use `val.Exists` instead of assuming empty value.
+				if !val.Exists {
+					valExists = tikv.SetKeyLockedValueNotExists
+				}


@cfzjywxk Is this safe? Previously when a key is locked without ReturnValue option, we assume it must exist since TiDB will only lock the key after it's found in scanning. However this changed the behavior. I'm thinking if I'd better add two new flags for storing whether it exists or not, that the new flags will only be used in assertion, to avoid changing the normal behavior.

I decide to add new flags for it. Even it's safe, it will introduce difficulties when we meet problem about the LockedValueExists flag (which affects the behavior about "duplicated entry") and try to diagnose it.
Nevermind.

@MyonKeminta
The meanings seems to be the same, use pessimistic lock response to record existing key value, I think it'ok if the judging condition is handled correctly.

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

ekexium · 2021-12-17T11:39:33Z

I think the keys and values in error messages can be encoded as hex for readability.

cfzjywxk · 2021-12-17T12:16:53Z

I think the keys and values in error messages can be encoded as hex for readability.

I usually use the hex encoded format printing keys and values.

MyonKeminta · 2021-12-20T04:49:09Z

I think the keys and values in error messages can be encoded as hex for readability.

Did you mean ErrAssertionFailed defined in error/errors.go? The Error method is implemented in the same way as other kinds of errors defined in this file.

However, by the way, I'm afraid things about redacting is questionable here, for example, the not-redacted keys might be printed when printing these kinds of errors in logs.

ekexium · 2021-12-20T06:02:48Z

Did you mean ErrAssertionFailed defined in error/errors.go? The Error method is implemented in the same way as other kinds of errors defined in this file.

Yes. It comes from the protobuf marshaller. I agree on leaving it for now.

However, by the way, I'm afraid things about redacting is questionable here, for example, the not-redacted keys might be printed when printing these kinds of errors in logs.

Then the problem also exists for other error types? client-go doesn't care about log redaction at all.

MyonKeminta · 2021-12-20T06:25:48Z

However, by the way, I'm afraid things about redacting is questionable here, for example, the not-redacted keys might be printed when printing these kinds of errors in logs.

Then the problem also exists for other error types? client-go doesn't care about log redaction at all.

Yes. I mean, it's possible that TiDB prints an error from client-go, and the error might be of these kinds, and as a result it will introduce the risk of leaking sensitive information to the log. And I think even if there's no such case in TiDB, it can still be easily introduced.
It's not related to this PR though. I'm just thinking there might be a problem.

sticnarf · 2021-12-20T08:52:20Z

txnkv/transaction/txn.go

+			// If ReturnValue is disabled and CheckExistence is requested, it's still possible that the TiKV's version
+			// is too old and CheckExistence is not supported.


I'm not clear about what this comment mean... Do we really need to pay attention to it?
TiDB should not be used with an old version of TiKV, so I don't think we should care about it. And I don't think a client-go user will set assertions (hopefully...)

It's not necessary if we assume TiDB is never used with an old version of TiKV. I'm feeling afraid since we are supporting downgrading, and I'm not sure if there's any special cases about that, so I added this...🤔

Emm, downgrading... I'm thinking when downgrading, the order should also reverse? First TiDB, then TiKV and PD?

I'm ok with the comment. It shows a requirement for minimal TiKV version.

Emm, downgrading... I'm thinking when downgrading, the order should also reverse? First TiDB, then TiKV and PD?

I think so, but I'm not sure how it really is.

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

ekexium · 2022-01-20T12:14:43Z

internal/mockstore/mocktikv/mvcc_leveldb.go

@@ -725,6 +734,9 @@ func checkConflictValue(iter *Iterator, m *kvrpcpb.Mutation, forUpdateTS uint64,
 		return nil, err
 	}
 	if !ok {
+		if m.Assertion == kvrpcpb.Assertion_Exist && assertionLevel != kvrpcpb.AssertionLevel_Off {
+			logutil.BgLogger().Error("ASSERTION FAIL!!! non-exist for must exist key", zap.Stringer("mutation", m))
+		}
 		return nil, nil


Why doesn't it return an error

Ah, this is old code before my modification and assertion is not supported in mocktikv (so there are only tests based on unistore). Since there's now too much difference between mocktikv and tikv, it might be complicated to implement assertion in mocktikv. I think there changes can be removed. How do you think?

Wait a minute, I need to double check it

Sorry I forgot it. To write tests for pessimistic lock returning existence, the related assertion logic is actually already half-implemented. Let's fix this return value for completeness.

ekexium · 2022-01-20T12:19:15Z

internal/mockstore/mocktikv/mvcc_leveldb.go

@@ -739,7 +751,8 @@ func checkConflictValue(iter *Iterator, m *kvrpcpb.Mutation, forUpdateTS uint64,
 	}

 	needGetVal := getVal
-	needCheckAssertion := m.Assertion == kvrpcpb.Assertion_NotExist
+	needCheckShouldNotExistForPessimisticLock := m.Assertion == kvrpcpb.Assertion_NotExist && m.Op == kvrpcpb.Op_PessimisticLock
+	needCheckAssertionForPrewerite := m.Assertion != kvrpcpb.Assertion_None && m.Op != kvrpcpb.Op_PessimisticLock && assertionLevel != kvrpcpb.AssertionLevel_Off


Suggested change

needCheckAssertionForPrewerite := m.Assertion != kvrpcpb.Assertion_None && m.Op != kvrpcpb.Op_PessimisticLock && assertionLevel != kvrpcpb.AssertionLevel_Off

needCheckAssertionForPrewrite := m.Assertion != kvrpcpb.Assertion_None && m.Op != kvrpcpb.Op_PessimisticLock && assertionLevel != kvrpcpb.AssertionLevel_Off

Do we need to consider Assertion_Unknown here? Though in the current implementation it doesn't make a difference.

No. Unknown will not be passed via TiKV RPC, so Assertion_Unknown even doesn't exist in kvproto.

ekexium · 2022-01-20T12:27:03Z

txnkv/transaction/2pc.go

+func (m *memBufferMutations) Push(op kvrpcpb.Op, isPessimisticLock, assertExist, assertNotExist bool, handle unionstore.MemKeyHandle) {
+	// The format to put to the handle.UserData:
+	// MSB                                                                            LSB
+	// [13 bits: Op][1 bit: assertNotExist][1 bit: assertExist][1 bit: isPessimisticLock]


I think it'd be better to place the comments next to its definition.

The format is defined by the user of memdb instead of memdb itself. I'll move the comment to near the definition of memBufferMutations.handles field.

Makes sense. It's just that when I read the usage of the type, I would jump to its definition and hope to find some explanations there.

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

ekexium

The rest LGTM

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

cfzjywxk · 2022-02-08T03:37:53Z

txnkv/transaction/pessimistic.go

+			// Handle the case that the TiKV's version is too old and doesn't support `CheckExistence`.
+			// If `CheckExistence` is set, `ReturnValues` is not set and `CheckExistence` is not supported, skip
+			// retrieving value totally (indicated by `skipRetrievingValue`) to avoid panicking.
+			skipRetrievingValue := !action.ReturnValues && action.CheckExistence && len(lockResp.NotFounds) == 0


Could the len(lockResp.NotFounds) == 0 represent such situation accurately?

The previous version of TiKV already supports setting the NotFounds field when ReturnValues is set. So when ReturnValues is set, both new and old versions of TiKV should work. But if CheckExistence is set and ReturnValues is unset, since old TiKV cannot recognize the new field CheckExistence, the NotFounds returned from TiKV will be empty. We cannot retrieve existence in this case, and it should be the only case.

cfzjywxk · 2022-02-08T03:45:49Z

kv/kv.go

+
+// InitCheckExistence creates the map to store whether each key exists or not.
+func (ctx *LockCtx) InitCheckExistence(capacity int) {
+	ctx.CheckExistence = true


This field will be set to true only in tests?

It's invoked by TiDB (https://github.com/pingcap/tidb/blob/e7bd1faf5526921badda00ed96d98fd4c2ac2bf3/executor/executor.go#L1011)

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta mentioned this pull request Sep 24, 2021

*: Support assertion for DMLs pingcap/tidb#28313

Merged

9 tasks

sticnarf reviewed Sep 26, 2021

View reviewed changes

ekexium mentioned this pull request Sep 29, 2021

Defend against data inconsistency and improve its troubleshooting pingcap/tidb#26833

Open

8 tasks

MyonKeminta marked this pull request as draft October 8, 2021 04:15

disksing closed this Oct 20, 2021

disksing reopened this Oct 20, 2021

lysu and others added 10 commits October 20, 2021 19:59

support set assertion in 2pc mutation

a5312ae

Signed-off-by: lysu <sulifx@gmail.com> Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Receive assertion fail errors from TiKV

c0db52f

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Add test log

ae15cad

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Remove verbose log

02e41c3

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

update kvproto

a711803

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Add metrics counter for assertions

8e258f0

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Address some comments

192b4a8

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Try to optimize assertion for pessimistic transactions

e21476f

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Fix panic on optimistic transactions

166b5f3

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Add InitCheckExistence method for LockCtx

e22a4eb

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta force-pushed the m/assertion branch from ccd06e8 to e22a4eb Compare October 20, 2021 12:00

Support assertion level

6369c44

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta force-pushed the m/assertion branch from 3c89534 to 6369c44 Compare October 27, 2021 09:59

MyonKeminta added 7 commits October 27, 2021 19:29

Check assertion level before doing assertion on client side

4ba0695

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Test bitoperations of menBUfferMutations.Push

59a2f94

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Add test for assertion in tikv

d5ac938

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Support run assertion test with unistore

ae6d576

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Fix test

e3ff51f

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Fix license

1acdbb0

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Fix test

4b4b2de

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta added 2 commits December 14, 2021 19:39

Add test for pessimistic lock check existence

ba52f73

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Test assertion takes no effect if amending is enabled

3c72eff

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta commented Dec 15, 2021

View reviewed changes

MyonKeminta added 2 commits December 16, 2021 13:13

Add HasAssertUnknown function

0ed9729

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Add comments

084762b

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta force-pushed the m/assertion branch from b11e6c2 to 084762b Compare December 16, 2021 06:08

Cleanup locks after assertion fail

e31bbdf

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

sticnarf reviewed Dec 20, 2021

View reviewed changes

Merge commit '62d6b4a2e8f7' into m/assertion

e78d46c

MyonKeminta force-pushed the m/assertion branch from 83e686c to e78d46c Compare January 11, 2022 07:18

MyonKeminta added 2 commits January 11, 2022 16:15

update tidb dependency

cc6aa72

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Fix panic in TestIllegalTSO

a942440

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta marked this pull request as ready for review January 20, 2022 09:48

MyonKeminta requested a review from ekexium January 20, 2022 10:21

ekexium reviewed Jan 20, 2022

View reviewed changes

Address comments

37ba4a8

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

ekexium approved these changes Jan 25, 2022

View reviewed changes

Add comments

30e265b

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

ekexium requested a review from sticnarf February 7, 2022 04:20

cfzjywxk reviewed Feb 8, 2022

View reviewed changes

cfzjywxk approved these changes Feb 8, 2022

View reviewed changes

MyonKeminta added 3 commits February 9, 2022 14:41

Update dependency of tidb

a9291e5

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Update dependency to tidb

4792e04

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Fix test

4d00224

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta merged commit b5eb031 into tikv:master Feb 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support setting assertions and receiving assertion errors for Prewrite requests #311

Support setting assertions and receiving assertion errors for Prewrite requests #311

MyonKeminta commented Sep 24, 2021

sticnarf Sep 26, 2021

sticnarf Sep 26, 2021

MyonKeminta Oct 8, 2021

sticnarf Sep 26, 2021

disksing commented Oct 20, 2021

MyonKeminta commented Oct 20, 2021

disksing commented Oct 20, 2021

MyonKeminta Dec 15, 2021

MyonKeminta Dec 15, 2021 •

edited

Loading

cfzjywxk Dec 16, 2021

ekexium commented Dec 17, 2021 •

edited

Loading

cfzjywxk commented Dec 17, 2021

MyonKeminta commented Dec 20, 2021

ekexium commented Dec 20, 2021

MyonKeminta commented Dec 20, 2021

sticnarf Dec 20, 2021 •

edited

Loading

MyonKeminta Dec 20, 2021

sticnarf Dec 20, 2021

sticnarf Dec 20, 2021

MyonKeminta Dec 20, 2021 •

edited

Loading

ekexium Jan 20, 2022

MyonKeminta Jan 21, 2022 •

edited

Loading

MyonKeminta Jan 21, 2022

MyonKeminta Jan 21, 2022

ekexium Jan 20, 2022

MyonKeminta Jan 21, 2022

ekexium Jan 20, 2022

MyonKeminta Jan 21, 2022

ekexium Jan 25, 2022

ekexium left a comment

cfzjywxk Feb 8, 2022

MyonKeminta Feb 8, 2022

cfzjywxk Feb 8, 2022

MyonKeminta Feb 8, 2022 •

edited

Loading

		// If ReturnValue is disabled and CheckExistence is requested, it's still possible that the TiKV's version
		// is too old and CheckExistence is not supported.

	needCheckAssertionForPrewerite := m.Assertion != kvrpcpb.Assertion_None && m.Op != kvrpcpb.Op_PessimisticLock && assertionLevel != kvrpcpb.AssertionLevel_Off
	needCheckAssertionForPrewrite := m.Assertion != kvrpcpb.Assertion_None && m.Op != kvrpcpb.Op_PessimisticLock && assertionLevel != kvrpcpb.AssertionLevel_Off

Support setting assertions and receiving assertion errors for Prewrite requests #311

Support setting assertions and receiving assertion errors for Prewrite requests #311

Conversation

MyonKeminta commented Sep 24, 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

disksing commented Oct 20, 2021

MyonKeminta commented Oct 20, 2021

disksing commented Oct 20, 2021

Choose a reason for hiding this comment

MyonKeminta Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekexium commented Dec 17, 2021 • edited Loading

cfzjywxk commented Dec 17, 2021

MyonKeminta commented Dec 20, 2021

ekexium commented Dec 20, 2021

MyonKeminta commented Dec 20, 2021

sticnarf Dec 20, 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

MyonKeminta Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MyonKeminta Jan 21, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekexium left a 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

MyonKeminta Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

MyonKeminta Dec 15, 2021 •

edited

Loading

ekexium commented Dec 17, 2021 •

edited

Loading

sticnarf Dec 20, 2021 •

edited

Loading

MyonKeminta Dec 20, 2021 •

edited

Loading

MyonKeminta Jan 21, 2022 •

edited

Loading

MyonKeminta Feb 8, 2022 •

edited

Loading