Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support postponed conflict check #556

Merged
merged 18 commits into from
Aug 30, 2022

Conversation

ekexium
Copy link
Contributor

@ekexium ekexium commented Jul 26, 2022

ref: pingcap/tidb#36579

What's changed:
Add a flag NeedConstraintCheckInPrewrite. It's a temporary flag, which means any read or write to a key with this flag will result in unsetting the flag. It's implemented by copying a new value log in the latest stage, so that later InspectStage could find the key. While it brings side effects to getValue, so we need to figure out where the side effect is needed.
An alternative way could be recording the changed value log addresses in a separate map, and resetting them every time stage changes. I chose not to do so for better compatibility (e.g. with savepoints).

I suppose it is unnecessary to encode flags in memBufferMutations, and CommitterMutationFlags could be removed. We can just read from the mem buffer for flags.
But for simplicity, I followed the routine.

ekexium added 5 commits July 25, 2022 17:24
Signed-off-by: ekexium <eke@fastmail.com>
Signed-off-by: ekexium <eke@fastmail.com>
Signed-off-by: ekexium <eke@fastmail.com>
Signed-off-by: ekexium <eke@fastmail.com>
@ekexium ekexium force-pushed the postponed-conflict-check branch from df2b4aa to 136f817 Compare August 23, 2022 05:41
Signed-off-by: ekexium <eke@fastmail.com>
@ekexium ekexium force-pushed the postponed-conflict-check branch from 4caca16 to 03942a5 Compare August 23, 2022 05:58
@ekexium ekexium marked this pull request as ready for review August 23, 2022 06:36
Copy link
Collaborator

@sticnarf sticnarf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering the possible impact to getValue, do you think it is an option to create a new variant of memDB (which may be a wrapper of the current one)? And the type of memDB is decided by the session variable when initializing?

@ekexium
Copy link
Contributor Author

ekexium commented Aug 25, 2022

Considering the possible impact to getValue, do you think it is an option to create a new variant of memDB (which may be a wrapper of the current one)? And the type of memDB is decided by the session variable when initializing?

Great idea!

Signed-off-by: ekexium <eke@fastmail.com>
@cfzjywxk
Copy link
Contributor

/cc @TonsnakeLin

@cfzjywxk
Copy link
Contributor

cfzjywxk commented Aug 25, 2022

An alternative way could be recording the changed value log addresses in a separate map, and resetting them every time stage changes. I chose not to do so for better compatibility (e.g. with savepoints).

Perhaps it would make the original checkpoint-related implementation much more complex.
BTW if the key flags are stored separately would it be possible to avoid the copy?

@ekexium
Copy link
Contributor Author

ekexium commented Aug 26, 2022

BTW if the key flags are stored separately would it be possible to avoid the copy?

The key here is to let InspectStage be able to find the change. IIUC as long as the impl doesn't change and flags are individual from stages it won't help.

@cfzjywxk
Copy link
Contributor

BTW if the key flags are stored separately would it be possible to avoid the copy?

The key here is to let InspectStage be able to find the change. IIUC as long as the impl doesn't change and flags are individual from stages it won't help.

Maybe we could record some future enhancement tasks for the MemDB like separating the flags.

@ekexium
Copy link
Contributor Author

ekexium commented Aug 26, 2022

Maybe we could record some future enhancement tasks for the MemDB like separating the flags.

Yes if it shows obvious increment in memory footprint. Let's see it when benchmarking.

Signed-off-by: ekexium <eke@fastmail.com>
Signed-off-by: ekexium <eke@fastmail.com>
@disksing disksing requested review from cfzjywxk and sticnarf August 29, 2022 03:11
Signed-off-by: ekexium <eke@fastmail.com>
@ekexium
Copy link
Contributor Author

ekexium commented Aug 29, 2022

"Compensating locks on read" can't totally avoid the anomaly of reading results that break uniqueness constraint.
Consider there are multiple unique keys, a select for update statement uses uk1 to read, and it locks uk1. However there are uniqueness conflict on uk2, but we are not able to know it until commit.
A more concrete case

set @@ tidb_constraint_check_in_place_pessimistic=1
create table t2 (id int primary key, uk int, unique key i1(uk))
insert into t2 values (1, 1)
begin pessimistic
insert into t2 values (2, 1)
select * from t2 use index(primary) for update -- (1,1) and (2,1)
commit -- fail

I'm considering falling back to the option of only removing the temporary flag when a key is modified. Or we can stick to the current design, but do not guarantee the integrity of read results.

@sticnarf
Copy link
Collaborator

I'm considering falling back to the option of only removing the temporary flag when a key is modified. Or we can stick to the current design, but do not guarantee the integrity of read results.

In my opinion, it is not so meaningful to stick to the current design if it cannot totally avoid the problem.

Signed-off-by: ekexium <eke@fastmail.com>

update tidb dependency

Signed-off-by: ekexium <eke@fastmail.com>
Signed-off-by: ekexium <eke@fastmail.com>
@ekexium ekexium force-pushed the postponed-conflict-check branch from f52d70e to de3b8ed Compare August 29, 2022 08:29
Signed-off-by: ekexium <eke@fastmail.com>
Signed-off-by: ekexium <eke@fastmail.com>
// When the key gets locked (and the existence is checked), the flag should be removed.
// It should only be applied to keys with PresumeNotExist, so that an in-place constraint check becomes
// (a conflict check + a constraint check) in prewrite.
flagNeedConstraintCheckInPrewrite
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe flagConstraintCheckInPrewrite is more consistency with flagKeyLocked and flagNeedConstraintCheckInPrewrite

Copy link
Contributor Author

@ekexium ekexium Aug 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I prefer need constraint check in prewrite. It clearly describes our intention.

internal/unionstore/memdb.go Outdated Show resolved Hide resolved
Signed-off-by: ekexium <eke@fastmail.com>
@MyonKeminta
Copy link
Contributor

Since the dependency to TiDB is replaced to a fork, should we merge this PR when the corresponding PR in TiDB is ready?

@sticnarf
Copy link
Collaborator

Since the dependency to TiDB is replaced to a fork, should we merge this PR when the corresponding PR in TiDB is ready?

We can accept this. The TiDB dependency is only used for integration test.

@sticnarf sticnarf merged commit 0130f76 into tikv:master Aug 30, 2022
TonsnakeLin pushed a commit to TonsnakeLin/client-go that referenced this pull request Aug 31, 2022
* replace kvproto

Signed-off-by: ekexium <eke@fastmail.com>

* support NeedConflictCheck

Signed-off-by: ekexium <eke@fastmail.com>

* fix mutation encoding

Signed-off-by: ekexium <eke@fastmail.com>

* support temporary flag

Signed-off-by: ekexium <eke@fastmail.com>

* update kvproto

Signed-off-by: ekexium <eke@fastmail.com>

* fix style

Signed-off-by: ekexium <eke@fastmail.com>

* add an option to enable the behavior

Signed-off-by: ekexium <eke@fastmail.com>

* replace AfterCheckPoint with existing canModity

Signed-off-by: ekexium <eke@fastmail.com>

* UpdateFlag do not unset temporary flag

Signed-off-by: ekexium <eke@fastmail.com>

* remove unused function

Signed-off-by: ekexium <eke@fastmail.com>

* update tidb dependency

Signed-off-by: ekexium <eke@fastmail.com>

update tidb dependency

Signed-off-by: ekexium <eke@fastmail.com>

* fix test

Signed-off-by: ekexium <eke@fastmail.com>

* do no unset flag on read

Signed-off-by: ekexium <eke@fastmail.com>

* update tidb dependency

Signed-off-by: ekexium <eke@fastmail.com>

* update comment

Signed-off-by: ekexium <eke@fastmail.com>

Signed-off-by: ekexium <eke@fastmail.com>
cfzjywxk added a commit that referenced this pull request Sep 6, 2022
* optimize for lock if exists

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* fix bugs for lock if exists

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* optimize lock info

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* fix bugs

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* remove the contrl flag for lock stats

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* update kvproto

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* change to LockOnlyIfExists

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* update kvproto

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* change to LockOnlyIfExists

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* make test pass

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* Update txnkv/transaction/txn.go

Co-authored-by: MyonKeminta <9948422+MyonKeminta@users.noreply.github.com>
Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* *: add getPDClient to rawKV public api (#570)

Signed-off-by: dongxu <i@huangdx.net>

Signed-off-by: dongxu <i@huangdx.net>
Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* Update README.md (#571)

Signed-off-by: dongxu <i@huangdx.net>
Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* update dependency of integration test (#572)

Signed-off-by: cfzjywxk <lsswxrxr@163.com>

Signed-off-by: cfzjywxk <lsswxrxr@163.com>
Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* Support postponed conflict check (#556)

* replace kvproto

Signed-off-by: ekexium <eke@fastmail.com>

* support NeedConflictCheck

Signed-off-by: ekexium <eke@fastmail.com>

* fix mutation encoding

Signed-off-by: ekexium <eke@fastmail.com>

* support temporary flag

Signed-off-by: ekexium <eke@fastmail.com>

* update kvproto

Signed-off-by: ekexium <eke@fastmail.com>

* fix style

Signed-off-by: ekexium <eke@fastmail.com>

* add an option to enable the behavior

Signed-off-by: ekexium <eke@fastmail.com>

* replace AfterCheckPoint with existing canModity

Signed-off-by: ekexium <eke@fastmail.com>

* UpdateFlag do not unset temporary flag

Signed-off-by: ekexium <eke@fastmail.com>

* remove unused function

Signed-off-by: ekexium <eke@fastmail.com>

* update tidb dependency

Signed-off-by: ekexium <eke@fastmail.com>

update tidb dependency

Signed-off-by: ekexium <eke@fastmail.com>

* fix test

Signed-off-by: ekexium <eke@fastmail.com>

* do no unset flag on read

Signed-off-by: ekexium <eke@fastmail.com>

* update tidb dependency

Signed-off-by: ekexium <eke@fastmail.com>

* update comment

Signed-off-by: ekexium <eke@fastmail.com>

Signed-off-by: ekexium <eke@fastmail.com>

* add testcase

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* add test case

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* unset pk if lockifexits failed

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* fix format

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* LockOnlyIfExists only when pk selected

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* remove test function to txn_probe

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* add more info to error

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* add more info to error

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

* add protection for input key

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

Signed-off-by: TonsnakeLin <lpbgytong@163.com>
Signed-off-by: dongxu <i@huangdx.net>
Signed-off-by: cfzjywxk <lsswxrxr@163.com>
Signed-off-by: ekexium <eke@fastmail.com>
Co-authored-by: MyonKeminta <9948422+MyonKeminta@users.noreply.github.com>
Co-authored-by: dongxu <i@huangdx.net>
Co-authored-by: cfzjywxk <lsswxrxr@163.com>
Co-authored-by: ekexium <eke@fastmail.com>
Co-authored-by: Yilin Chen <sticnarf@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants