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

Disallow commit-time-batch for write-prepared/write-unprepared txn conditionally #9794

Closed
wants to merge 5 commits into from

Conversation

riversand963
Copy link
Contributor

@riversand963 riversand963 commented Apr 1, 2022

For write-prepared/write-unprepared transactions,
GetCommitTimeWriteBatch() can be used only if the transaction is started
with TransactionOptions::use_only_the_last_commit_time_batch_for_recovery set
to true. Otherwise, it is possible that multiple uncommitted versions of the
same key exist in the database. During bottommost compaction, RocksDB may
set the sequence numbers of both to zero once they become committed, causing
output SST file to have two identical internal keys.

Test plan:
make check
pay special attention to the following

transaction_test --gtest_filter=MySQLStyleTransactionTest/MySQLStyleTransactionTest.TransactionStressTest/*

Apply the following test patch

index 36b2e9ec4..1f1d96d27 100644
--- a/test_util/transaction_test_util.cc
+++ b/test_util/transaction_test_util.cc
@@ -212,7 +212,7 @@ bool RandomTransactionInserter::DoInsert(DB* db, Transaction* txn,
         ROCKS_LOG_DEBUG(db->GetDBOptions().info_log,
                         "Prepare of %" PRIu64 " %s (%s)", txn->GetId(),
                         s.ToString().c_str(), txn->GetName().c_str());
-        if (rand_->OneIn(20)) {
+        if (true || rand_->OneIn(20)) {
           // This currently only tests the mechanics of writing commit time
           // write batch so the exact values would not matter.
           s = txn_->GetCommitTimeWriteBatch()->Put("cat", "dog");

and then run tests multiple times

TEST_TMPDIR=/dev/shm/ ~/gtest-parallel/gtest-parallel -r 1024 -w 64 ./transaction_test --gtest_filter=*MySQLStyleTransactionTest.TransactionStressTest/5

Before this fix, the above command has quite high probability to fail. After the fix, all tests pass consistently.

@facebook-github-bot
Copy link
Contributor

@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@riversand963 has updated the pull request. You must reimport the pull request before landing.

@riversand963 riversand963 changed the title Disallow usage of commit-time-batch for write-prepared txn conditionally Disallow commit-time-batch for write-prepared/write-unprepared txn conditionally Apr 1, 2022
@facebook-github-bot
Copy link
Contributor

@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@riversand963 riversand963 requested review from lth and siying April 4, 2022 15:58
Copy link
Contributor

@lth lth left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@facebook-github-bot
Copy link
Contributor

@riversand963 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@riversand963 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@riversand963 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@riversand963 riversand963 deleted the clarify-wp-ctwb branch April 5, 2022 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants