WritePrepared Txn: Compaction/Flush #2926

yiwu-arbug · 2017-09-25T17:51:47Z

Summary:
Update Compaction/Flush to support WritePreparedTxnDB: Add SnapshotChecker which is a proxy to query WritePreparedTxnDB::IsInSnapshot. Pass SnapshotChecker to DBImpl on WritePreparedTxnDB open. CompactionIterator use it to check if a key has been committed and if it is visible to a snapshot. In CompactionIterator:

check if key has been committed. If not, output uncommitted keys AS-IS.
use SnapshotChecker to check if key is visible to a snapshot when in need.
do not output key with seq = 0 if the key is not committed.

Test Plan:
See the new tests.

yiwu-arbug · 2017-09-25T17:52:05Z

Work in progress: Pending test.

facebook-github-bot · 2017-09-25T17:52:27Z

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-09-25T19:46:29Z

@yiwu-arbug has updated the pull request. View: changes, changes since last import

facebook-github-bot · 2017-09-25T19:46:57Z

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-09-25T21:13:23Z

@yiwu-arbug has updated the pull request. View: changes, changes since last import

facebook-github-bot · 2017-09-25T21:13:45Z

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-09-27T17:39:55Z

@yiwu-arbug has updated the pull request.

facebook-github-bot · 2017-09-27T17:43:54Z

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-09-27T18:06:15Z

@yiwu-arbug has updated the pull request.

facebook-github-bot · 2017-09-27T18:06:26Z

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

maysamyabandeh · 2017-09-27T18:10:16Z

db/compaction_iterator.cc

@@ -292,6 +302,18 @@ void CompactionIterator::NextFromInput() {
      ikey_.user_key = current_key_.GetUserKey();
    }

+    if (UNLIKELY(!current_key_committed_)) {


Put a TODO to change this to LIKELY when our default txn policy changes.

Will refactor a bit to make this cleaner.

maysamyabandeh

1st pass

maysamyabandeh · 2017-09-27T18:17:11Z

db/compaction_iterator.cc

@@ -563,10 +588,13 @@ void CompactionIterator::PrepareOutput() {
  }
 }

-inline SequenceNumber CompactionIterator::findEarliestVisibleSnapshot(


why inline is removed?

maysamyabandeh · 2017-09-27T18:22:18Z

db/compaction_job_test.cc

+                                 versions_.get(), &shutting_down_, &log_buffer,
+                                 nullptr, nullptr, nullptr, &mutex_, &bg_error_,
+                                 snapshots, earliest_write_conflict_snapshot,
+                                 nullptr, table_cache_, &event_logger, false,


Should we extend this test to include snapshot_checker?

maysamyabandeh · 2017-09-27T18:23:21Z

db/db_impl.h

@@ -1213,6 +1219,10 @@ class DBImpl : public DB {
  std::unordered_map<uint64_t, uint64_t> prepared_section_completed_;
  std::mutex prep_heap_mutex_;

+  // Callback for compaction to check if a key is visible to a snapshot.
+  // REQUIRES: mutex held


smart pointers are not thread-safe, and I'm not making it constant since I set it during WritePrepareTxnDB::init() which is after DBImpl open.

Does it any overhead? Does compaction already holds the mutex_?

We only set the field once. Compaction hold mutex on-and-off. Just make sure the pointer is fetched while compaction is holding mutex.

maysamyabandeh · 2017-09-27T18:27:47Z

db/db_impl_open.cc

@@ -892,7 +892,7 @@ Status DBImpl::WriteLevel0TableForRecovery(int job_id, ColumnFamilyData* cfd,
          std::unique_ptr<InternalIterator>(mem->NewRangeTombstoneIterator(ro)),
          &meta, cfd->internal_comparator(),
          cfd->int_tbl_prop_collector_factories(), cfd->GetID(), cfd->GetName(),
-          snapshot_seqs, earliest_write_conflict_snapshot,
+          snapshot_seqs, earliest_write_conflict_snapshot, nullptr,


why no snapshot_checker is passed here?

I'm wanting to avoid adding yet another option to db_options, and we don't really need snapshot_checker before WritePreparedTxnDB init, which is after DBImpl open.

Then if we use SetSnapshotChecker to set it, does anybody uses this changed API of BuildTable to set the snapshot checker? If not can we remove it?

maysamyabandeh · 2017-09-27T18:29:40Z

db/flush_job_test.cc

+  FlushJob flush_job(
+      dbname_, versions_->GetColumnFamilySet()->GetDefault(), db_options_,
+      *cfd->GetLatestMutableCFOptions(), env_options_, versions_.get(), &mutex_,
+      &shutting_down_, {}, kMaxSequenceNumber, nullptr, &job_context, nullptr,


should we extend the test to work with snapshot checker?

maysamyabandeh · 2017-09-27T18:33:58Z

db/repair.cc

-          nullptr /* internal_stats */, TableFileCreationReason::kRecovery,
-          nullptr /* event_logger */, 0 /* job_id */, Env::IO_HIGH,
-          nullptr /* table_properties */, -1 /* level */, current_time);
+          {}, kMaxSequenceNumber, nullptr /*snapshot_checker*/, kNoCompression,


why does not repair need snapshot checker? does not it flush memtables when they are full?

Transaction is not involved in repair (let me know if I'm wrong) and snapshot_checker is not needed.

Could repair result into memtable flush? Does it involve any memtable insert?

I was assuming these will be no preparing transaction on db open, but I was wrong since there can be preparing transactions recovering from logs. Let me think..

Looks like the repair code doesn't support transactions, since it simply replay logs and write them into sst file but don't even look at prepare/commit markers.

maysamyabandeh · 2017-09-27T18:37:20Z

db/snapshot_checker.h

+                                   SequenceNumber snapshot_sequence) const {
+  return sequence <= snapshot_sequence;
+}
+#endif  // ROCKSDB_LITE


! ROCKSDB_LITE

maysamyabandeh · 2017-09-27T18:37:30Z

db/snapshot_checker.h

+#endif  // !ROCKSDB_LITE
+};
+
+#ifdef ROCKSDB_LITE


why not moving it to cc file?

maysamyabandeh · 2017-09-27T18:40:08Z

db/snapshot_checker.h

+
+bool SnapshotChecker::IsInSnapshot(SequenceNumber sequence,
+                                   SequenceNumber snapshot_sequence) const {
+  return sequence <= snapshot_sequence;


This is confusing. If we do not support WritePrepared in ROCKSDB_LITE, then SnapshotChecker should be set to nullptr. This function should never be called in that case so it should throw an exception if it is called.

snapshot_checker is not being used in LITE mode. The implementation here is just a place holder.

I understand that but providing an implementation that is never used could be confusing. Return true all the time with a proper in-line comment would make it more clear.

maysamyabandeh · 2017-09-27T18:44:02Z

utilities/transactions/pessimistic_transaction_db.h

-  port::RWMutex old_commit_map_mutex_;
-  port::RWMutex commit_cache_mutex_;
-  port::RWMutex snapshots_mutex_;
+  mutable port::RWMutex prepared_mutex_;


why mutable?

I'm adding const keyword to IsInSnapshot call and mutable here means holding mutex doesn't violate const.

maysamyabandeh

2nd pass is done.
Instead of passing a blind nullptr to the functions it would be great if you add a const variable, assigned to nullptr with some comments that why it makes sense to pass nullptr in each case.

maysamyabandeh · 2017-09-27T21:00:08Z

db/compaction_iterator.cc

+  if (snapshot_checker_ != nullptr) {
+    return findEarliestVisibleSnapshotWithSnapshotChecker(in, prev_snapshot);
+  }
+  SequenceNumber prev = kMaxSequenceNumber;
  for (const auto cur : *snapshots_) {
    assert(prev == kMaxSequenceNumber || prev <= cur);
    if (cur >= in) {


The only difference between this func and findEarliestVisibleSnapshotWithSnapshotChecker is the check in this line. Can we instead of adding findEarliestVisibleSnapshotWithSnapshotChecker, simply put the if-then condition here: i.e., if (snapshot_checker_ != nullptr) call snapsh_checker else (cur >= in).

maysamyabandeh · 2017-09-27T21:06:57Z

utilities/transactions/write_prepared_transaction_test.cc

+      ASSERT_EQ(expected_versions[i].sequence, versions[i].sequence);
+      ASSERT_EQ(expected_versions[i].type, versions[i].type);
+      if (versions[i].type != kTypeDeletion &&
+          versions[i].type != kTypeSingleDeletion) {


How about Range Delete?

RangeDelete is not supported for transactions (seems even for StackableDB).

Can you assert that here? Just in case it changes in future.

maysamyabandeh · 2017-09-27T21:19:24Z

utilities/transactions/write_prepared_transaction_test.cc

+  ASSERT_OK(db->Put(WriteOptions(), "a", "dummy"));
+  ASSERT_OK(db->Put(WriteOptions(), "z", "dummy"));
+  ASSERT_OK(db->CompactRange(CompactRangeOptions(), nullptr, nullptr));
+  VerifyInternalKeys({


Is there a way to make it easier to verify by eyeball checking? Currently it is not clear whether this values actually test the implementation or they are set to make the test pass. For example we can add an item to the verify list right after the final insertion for the key. This would not still help figuring the sequence number.

any idea here?

Fixed by adding the expected_seq variable.

maysamyabandeh · 2017-09-27T21:24:37Z

utilities/transactions/write_prepared_transaction_test.cc

+  delete transaction;
+}
+
+TEST_P(WritePreparedTransactionTest, CompactionShouldKeepSnapshotVisibleKeys) {


should we disable auto compaction to make sure the test is deterministic?

good catch!

maysamyabandeh · 2017-09-27T21:25:40Z

utilities/transactions/write_prepared_transaction_test.cc

+  ASSERT_OK(db->Flush(FlushOptions()));
+  db->ReleaseSnapshot(snapshot1);
+  db->ReleaseSnapshot(snapshot3);
+  // Dummy keys to avoid trivial move.


What does "move" mean here?

Compaction can trivially move file from lower level to upper level, which is called trivial move. When it happen CompactionIterator is not called and we are not testing actual compaction.

Would be great if you put this response as an in-line comment.

maysamyabandeh · 2017-09-27T21:31:02Z

utilities/transactions/write_prepared_transaction_test.cc

+  VerifyInternalKeys({
+      {"key1", "value1_2", 5, kTypeValue},
+      // "value1_1" is visible to snapshot2. Also keys at bottom level visible
+      // to earliest snapshot will output with seq = 0.


Does this mean that IsInSnapshot should always return true for seq == 0? Can you add a unit test for that and make the code changes in IsInSnapshot to let the test pass?

Will do in a separate PR.

maysamyabandeh · 2017-09-27T21:33:11Z

utilities/transactions/write_prepared_transaction_test.cc

+  ASSERT_OK(db->Put(WriteOptions(), "z", "dummy"));
+  ASSERT_OK(db->CompactRange(CompactRangeOptions(), nullptr, nullptr));
+  VerifyInternalKeys({
+      {"key1", "value1_2", 5, kTypeValue},


again it is hard to see if the seq 5 is verifying the code or it is a number we came up with to make the test pass. How about adding a variable exp_seq and ++ it after each operation. This way there would be less likely that we have got the math wrong.

any thought?

Added the expected_seq variable.

maysamyabandeh · 2017-09-27T21:34:25Z

utilities/transactions/write_prepared_transaction_test.cc

+}
+
+TEST_P(WritePreparedTransactionTest,
+       CompactionShouldKeepSequenceForUncommittedKeys) {


The name sounds similar to CompactionShouldKeepUncommittedKeys. Perhaps add some comments to clarify what the test does and how these two are different.

Any thoughts here?

Comment added.

facebook-github-bot · 2017-09-29T05:50:45Z

@yiwu-arbug has updated the pull request.

facebook-github-bot · 2017-09-29T05:51:17Z

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-09-29T06:03:32Z

@yiwu-arbug has updated the pull request. View: changes, changes since last import

facebook-github-bot · 2017-09-29T06:14:05Z

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-10-03T06:12:23Z

@yiwu-arbug has updated the pull request.

facebook-github-bot

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-10-03T16:42:44Z

@yiwu-arbug has updated the pull request. View: changes, changes since last import

facebook-github-bot

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

yiwu-arbug · 2017-10-05T23:13:40Z

utilities/transactions/write_prepared_transaction_test.cc

+// number equal to 0 if the key is not visible to earliest snapshot, based on
+// commit sequence number.
+TEST_P(WritePreparedTransactionTest,
+       CompactionShouldKeepSequenceForUncommittedKeys) {


will disable this test before landing, since without #2972 it will fail.

facebook-github-bot · 2017-10-05T23:16:02Z

@yiwu-arbug has updated the pull request.

facebook-github-bot · 2017-10-05T23:17:52Z

@yiwu-arbug has updated the pull request. View: changes, changes since last import

facebook-github-bot

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

maysamyabandeh · 2017-10-05T23:39:04Z

db/repair.cc

@@ -400,17 +400,21 @@ class Repairer {
      int64_t _current_time = 0;
      status = env_->GetCurrentTime(&_current_time);  // ignore error
      const uint64_t current_time = static_cast<uint64_t>(_current_time);
+      // Only TransactionDB make use of snapshot_checker and repair doesn't
+      // currently support TransactionDB with uncommitted prepared keys in WAL.


Does Repair do any compaction? Does it need to be aware of uncommitted values? If yes, should we add a TODO to upgrade repair later?

facebook-github-bot · 2017-10-06T00:04:13Z

@yiwu-arbug has updated the pull request.

facebook-github-bot

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@yiwu-arbug is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

yiwu-arbug requested a review from maysamyabandeh September 25, 2017 17:51

facebook-github-bot added the CLA Signed label Sep 25, 2017

yiwu-arbug mentioned this pull request Sep 27, 2017

[Transaction] Add TransactionDB::SingleDelete() #2937

Closed

yiwu-arbug force-pushed the txn_compaction branch from 5a64352 to 363ddc2 Compare September 27, 2017 17:39

yiwu-arbug changed the title ~~[WIP][Transaction] Compaction/Flush support for WritePreparedTxnDB~~ [Transaction] Compaction/Flush support for WritePreparedTxnDB Sep 27, 2017

yiwu-arbug changed the title ~~[Transaction] Compaction/Flush support for WritePreparedTxnDB~~ [Transaction] WritePrepared Txn: Compaction/Flush Sep 27, 2017

yiwu-arbug changed the title ~~[Transaction] WritePrepared Txn: Compaction/Flush~~ WritePrepared Txn: Compaction/Flush Sep 27, 2017

yiwu-arbug force-pushed the txn_compaction branch from 363ddc2 to aa79ae5 Compare September 27, 2017 18:06

maysamyabandeh reviewed Sep 27, 2017

View reviewed changes

yiwu-arbug force-pushed the txn_compaction branch from aa79ae5 to ac4d8fa Compare September 29, 2017 05:50

yiwu-arbug force-pushed the txn_compaction branch from e4fd2b2 to 19a84ed Compare October 3, 2017 06:12

facebook-github-bot reviewed Oct 3, 2017

View reviewed changes

facebook-github-bot reviewed Oct 5, 2017

View reviewed changes

yiwu-arbug commented Oct 5, 2017

View reviewed changes

yiwu-arbug force-pushed the txn_compaction branch from 10452b2 to 254fab1 Compare October 5, 2017 23:15

facebook-github-bot reviewed Oct 5, 2017

View reviewed changes

maysamyabandeh reviewed Oct 5, 2017

View reviewed changes

Yi Wu added 16 commits October 5, 2017 16:57

[WIP][Transaction] Flush/Compaction support for WritePreparedTxnDB

f9d0a54

Fix lint and minor refactor

9faa2a0

Avoid sequence turn into zero if key not visible to earliest snapshot

3fbd148

Fix lite build and minor refactor

cce815e

update TARGETS

e6db6a7

Add tests and fixes

d4c703f

address most of the comments

edd0719

clang-format

1756fb0

fix lite

52c9793

disable SeqAdvanceTest auto compactions

941962d

add comment for the nullptrs

3a90c6e

address comments other than multiple snapshot test

f0272df

Improve tests

c665b42

fix test. minor

9092eeb

disable the new test

9e2baa6

Adding TODO: repairing TransactionDB

939fc2d

yiwu-arbug force-pushed the txn_compaction branch from a9fcbc1 to 939fc2d Compare October 6, 2017 00:04

facebook-github-bot reviewed Oct 6, 2017

View reviewed changes

maysamyabandeh approved these changes Oct 6, 2017

View reviewed changes

facebook-github-bot reviewed Oct 6, 2017

View reviewed changes

facebook-github-bot closed this in d1b74b0 Oct 6, 2017

yiwu-arbug deleted the txn_compaction branch October 6, 2017 17:45

WritePrepared Txn: Compaction/Flush #2926

WritePrepared Txn: Compaction/Flush #2926

Conversation

yiwu-arbug commented Sep 25, 2017 • edited

yiwu-arbug commented Sep 25, 2017

facebook-github-bot commented Sep 25, 2017

facebook-github-bot commented Sep 25, 2017

facebook-github-bot commented Sep 25, 2017

facebook-github-bot commented Sep 25, 2017

facebook-github-bot commented Sep 25, 2017

facebook-github-bot commented Sep 27, 2017

facebook-github-bot commented Sep 27, 2017

facebook-github-bot commented Sep 27, 2017

facebook-github-bot commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysamyabandeh 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysamyabandeh 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

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

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

Choose a reason for hiding this comment

facebook-github-bot commented Sep 29, 2017

facebook-github-bot commented Sep 29, 2017

facebook-github-bot commented Sep 29, 2017

facebook-github-bot commented Sep 29, 2017

facebook-github-bot commented Oct 3, 2017

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 3, 2017

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 5, 2017

facebook-github-bot commented Oct 5, 2017

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 6, 2017

facebook-github-bot left a comment

yiwu-arbug commented Sep 25, 2017 •

edited