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

PopSavePoint discards lock information #5618

Closed
lth opened this issue Jul 24, 2019 · 0 comments
Closed

PopSavePoint discards lock information #5618

lth opened this issue Jul 24, 2019 · 0 comments

Comments

@lth
Copy link
Contributor

lth commented Jul 24, 2019

Expected behavior

If you have:

txn->SetSavePoint();
txn->Put("A")
txn->SetSavePoint();
txn->Put("B")
txn->PopSavePoint()
txn->RollbackToSavePoint()

Then the write batch is rolled back to the first savepoint, meaning that both keys A and B are rolled back. I also expect that keys A and B will both be unlocked as well.

Actual behavior

Only A is unlocked, and B remains locked. The problem is that every savepoint tracks keys locked since its last savepoint, and that information is used for unlocking if RollbackToSavePoint is called. When PopSavePoint is called, instead of simply popping off the stack and discarding that information, the key set should be merged with the key set of the previous savepoint instead, so that all keys get unlocked.

Steps to reproduce the behavior

Add this to utilities/transactions/transaction_test.cc

TEST_P(TransactionTest, SavepointTest4) {
  WriteOptions write_options;
  ReadOptions read_options;
  TransactionOptions txn_options;
  Status s;

  txn_options.lock_timeout = 1;  // 1 ms
  Transaction* txn1 = db->BeginTransaction(write_options, txn_options);
  ASSERT_TRUE(txn1);

  txn1->SetSavePoint(); // 1
  s = txn1->Put("A", "");
  ASSERT_OK(s);

  txn1->SetSavePoint(); // 2
  s = txn1->Put("B", "");
  ASSERT_OK(s);

  s = txn1->PopSavePoint(); // Remove 2
  ASSERT_OK(s);

  ASSERT_OK(txn1->RollbackToSavePoint());  // Rollback to 1

  // Verify that everything was rolled back.
  std::string value;
  s = txn1->Get(read_options, "A", &value);
  ASSERT_TRUE(s.IsNotFound());

  s = txn1->Get(read_options, "B", &value);
  ASSERT_TRUE(s.IsNotFound());

  // Nothing should be locked
  Transaction* txn2 = db->BeginTransaction(write_options, txn_options);
  ASSERT_TRUE(txn2);

  s = txn2->Put("A", "");
  ASSERT_OK(s);

  s = txn2->Put("B", "");
  ASSERT_OK(s); // This fails

  delete txn2;
  delete txn1;
}
merryChris pushed a commit to merryChris/rocksdb that referenced this issue Nov 18, 2019
…5628)

Summary:
Transaction::RollbackToSavePoint undos the modification made since the SavePoint beginning, and also unlocks the corresponding keys, which are tracked in the last SavePoint. Currently ::PopSavePoint simply discard these tracked keys, leaving them locked in the lock manager. This breaks a subsequent ::RollbackToSavePoint behavior as it loses track of such keys, and thus cannot unlock them. The patch fixes ::PopSavePoint by passing on the track key information to the previous SavePoint.
Fixes facebook#5618
Pull Request resolved: facebook#5628

Differential Revision: D16505325

Pulled By: lth

fbshipit-source-id: 2bc3b30963ab4d36d996d1f66543c93abf358980
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 a pull request may close this issue.

1 participant