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

Fix two bugs with insert and delete #253

Merged
merged 1 commit into from Apr 14, 2021
Merged

Fix two bugs with insert and delete #253

merged 1 commit into from Apr 14, 2021

Conversation

nrc
Copy link
Collaborator

@nrc nrc commented Apr 9, 2021

Fixes #234

PTAL @sticnarf

Previously there were two bugs:

  • If the user deletes a key, then inserts a value to the same key, then there will be false positive failure of the insert invariant. This is fixed by converting the insert into a put.
  • If the user inserts then deletes a key, the delete is converted into a check that the value doesn't exist. However, if the transaction is pessimistic, TiKV cannot handle that check and the key is left locked (Committing a delete must unlock #234). This is fixed by checking for previous values when we lock the key for insertion, then removing the insert rather than converting it if it is deleted.

@nrc nrc added the bug Something isn't working label Apr 9, 2021
@nrc nrc added this to the 0.1.0 release milestone Apr 9, 2021
@nrc nrc requested a review from sticnarf April 9, 2021 00:25
@nrc nrc force-pushed the unlock-delete branch 2 times, most recently from e4097d4 to c2987a8 Compare April 9, 2021 02:22
@ekexium
Copy link
Collaborator

ekexium commented Apr 9, 2021

If we consider the buffer as a state machine, there might be several more issues that are worth consideration:

  1. Locked + Lock: we don't need to lock again, unless (1) auto_heartbeat disabled (2) value needed but there is no cache
  2. CheckNotExist + Delete: should be CheckNotExist

And I think CheckNotExist is a little special one. I suggest we treat it as a normal operation in the buffer, though we don't provide a direct corresponding operation in Transaction. So that for example in

if !matches!(entry, BufferEntry::Cached(_)) {
self.primary_key.get_or_insert_with(|| key.clone());
}
, we should also exclude CheckNotExist.

@sticnarf
Copy link
Collaborator

sticnarf commented Apr 9, 2021

If we consider the buffer as a state machine, there might be several more issues that are worth consideration:

1. Locked + Lock: we don't need to lock again, unless auto_heartbeat disabled

2. CheckNotExist + Delete: should be CheckNotExist

+1 for "Locked + Lock".

CheckNotExist was introduced for TiDB to simulate MySQL like behavior in optimistic transactions. I am truly not sure what users expect under "insert and delete". Is there a case people expect commit not to fail under this case?

There may also be an alternative that we always check existence inplace at insert instead postponing it until commit.

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.

This PR itself looks good. The remaining problems may not be the subject of this PR :)

Copy link
Collaborator

@ekexium ekexium left a comment

Choose a reason for hiding this comment

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

LGTM

src/transaction/lowering.rs Outdated Show resolved Hide resolved
tests/integration_tests.rs Outdated Show resolved Hide resolved
@nrc nrc force-pushed the unlock-delete branch 4 times, most recently from 7493015 to 13fabea Compare April 13, 2021 22:14
@nrc
Copy link
Collaborator Author

nrc commented Apr 13, 2021

rust-lang/rust#82608 caused a compiler regression which causes the build to fail (due to spurious errors in the clang-sys crate). I've added a rust-toolchain file to address this temporarily.

@nrc nrc mentioned this pull request Apr 13, 2021
Fixes tikv#234

Signed-off-by: Nick Cameron <nrc@ncameron.org>
@nrc nrc merged commit 0314d63 into tikv:master Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working status/LGT2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Committing a delete must unlock
4 participants