Clarify the ownership of root db after TransactionDB::Open

Summary:
The patch clarifies the ownership of the root db after TransactionDB::Open. If it is a success the ownership if with the TransactionDB, and the root db will be deleted when the destructor of the base class, StackableDB, is called. If it is failure, the temporarily created root db will also be deleted properly.
The patch also includes lots of useful formatting changes.

Closes https://github.com/facebook/rocksdb/pull/3714 upon which this patch is built.
Closes https://github.com/facebook/rocksdb/pull/3806

Differential Revision: D7878010

Pulled By: maysamyabandeh

fbshipit-source-id: f54f3942e29434143ae5a2423ceec9c7072cd4c2

include/rocksdb/utilities/transaction_db.h

@@ -193,6 +193,7 @@ class TransactionDB : public StackableDB {
   }
   // Open a TransactionDB similar to DB::Open().
   // Internally call PrepareWrap() and WrapDB()
+  // If the return status is not ok, then dbptr is set to nullptr.
   static Status Open(const Options& options,
                      const TransactionDBOptions& txn_db_options,
                      const std::string& dbname, TransactionDB** dbptr);
@@ -203,27 +204,29 @@ class TransactionDB : public StackableDB {
                      const std::vector<ColumnFamilyDescriptor>& column_families,
                      std::vector<ColumnFamilyHandle*>* handles,
                      TransactionDB** dbptr);
-  // The following functions are used to open a TransactionDB internally using
-  // an opened DB or StackableDB.
-  // 1. Call prepareWrap(), passing an empty std::vector<size_t> to
-  // compaction_enabled_cf_indices.
-  // 2. Open DB or Stackable DB with db_options and column_families passed to
-  // prepareWrap()
   // Note: PrepareWrap() may change parameters, make copies before the
   // invocation if needed.
-  // 3. Call Wrap*DB() with compaction_enabled_cf_indices in step 1 and handles
-  // of the opened DB/StackableDB in step 2
   static void PrepareWrap(DBOptions* db_options,
                           std::vector<ColumnFamilyDescriptor>* column_families,
                           std::vector<size_t>* compaction_enabled_cf_indices);
+  // If the return status is not ok, then dbptr will bet set to nullptr. The
+  // input db parameter might or might not be deleted as a result of the
+  // failure. If it is properly deleted it will be set to nullptr. If the return
+  // status is ok, the ownership of db is transferred to dbptr.
   static Status WrapDB(DB* db, const TransactionDBOptions& txn_db_options,
                        const std::vector<size_t>& compaction_enabled_cf_indices,
                        const std::vector<ColumnFamilyHandle*>& handles,
                        TransactionDB** dbptr);
+  // If the return status is not ok, then dbptr will bet set to nullptr. The
+  // input db parameter might or might not be deleted as a result of the
+  // failure. If it is properly deleted it will be set to nullptr. If the return
+  // status is ok, the ownership of db is transferred to dbptr.
   static Status WrapStackableDB(
       StackableDB* db, const TransactionDBOptions& txn_db_options,
       const std::vector<size_t>& compaction_enabled_cf_indices,
       const std::vector<ColumnFamilyHandle*>& handles, TransactionDB** dbptr);
+  // Since the destructor in StackableDB is virtual, this destructor is virtual
+  // too. The root db will be deleted by the base's destructor.
   ~TransactionDB() override {}
 
   // Starts a new Transaction.
@@ -252,6 +255,7 @@ class TransactionDB : public StackableDB {
 
  protected:
   // To Create an TransactionDB, call Open()
+  // The ownership of db is transferred to the base StackableDB
   explicit TransactionDB(DB* db) : StackableDB(db) {}
 
  private:

utilities/transactions/pessimistic_transaction_db.cc

@@ -207,7 +207,7 @@ Status TransactionDB::Open(
     const std::vector<ColumnFamilyDescriptor>& column_families,
     std::vector<ColumnFamilyHandle*>* handles, TransactionDB** dbptr) {
   Status s;
-  DB* db;
+  DB* db = nullptr;
 
   ROCKS_LOG_WARN(db_options.info_log, "Transaction write_policy is %" PRId32,
                  static_cast<int>(txn_db_options.write_policy));
@@ -223,6 +223,10 @@ Status TransactionDB::Open(
     s = WrapDB(db, txn_db_options, compaction_enabled_cf_indices, *handles,
                dbptr);
   }
+  if (!s.ok()) {
+    // just in case it was not deleted (and not set to nullptr).
+    delete db;
+  }
   return s;
 }
 
@@ -254,48 +258,62 @@ Status TransactionDB::WrapDB(
     DB* db, const TransactionDBOptions& txn_db_options,
     const std::vector<size_t>& compaction_enabled_cf_indices,
     const std::vector<ColumnFamilyHandle*>& handles, TransactionDB** dbptr) {
-  PessimisticTransactionDB* txn_db;
+  assert(db != nullptr);
+  assert(dbptr != nullptr);
+  *dbptr = nullptr;
+  std::unique_ptr<PessimisticTransactionDB> txn_db;
   switch (txn_db_options.write_policy) {
     case WRITE_UNPREPARED:
       return Status::NotSupported("WRITE_UNPREPARED is not implemented yet");
     case WRITE_PREPARED:
-      txn_db = new WritePreparedTxnDB(
-          db, PessimisticTransactionDB::ValidateTxnDBOptions(txn_db_options));
+      txn_db.reset(new WritePreparedTxnDB(
+          db, PessimisticTransactionDB::ValidateTxnDBOptions(txn_db_options)));
       break;
     case WRITE_COMMITTED:
     default:
-      txn_db = new WriteCommittedTxnDB(
-          db, PessimisticTransactionDB::ValidateTxnDBOptions(txn_db_options));
+      txn_db.reset(new WriteCommittedTxnDB(
+          db, PessimisticTransactionDB::ValidateTxnDBOptions(txn_db_options)));
   }
   txn_db->UpdateCFComparatorMap(handles);
-  *dbptr = txn_db;
   Status s = txn_db->Initialize(compaction_enabled_cf_indices, handles);
+  // In case of a failure at this point, db is deleted via the txn_db destructor
+  // and set to nullptr.
+  if (s.ok()) {
+    *dbptr = txn_db.release();
+  }
   return s;
 }
 
 Status TransactionDB::WrapStackableDB(
     // make sure this stackable_db is already opened with memtable history
-    // enabled,
-    // auto compaction distabled and 2 phase commit enabled
+    // enabled, auto compaction distabled and 2 phase commit enabled
     StackableDB* db, const TransactionDBOptions& txn_db_options,
     const std::vector<size_t>& compaction_enabled_cf_indices,
     const std::vector<ColumnFamilyHandle*>& handles, TransactionDB** dbptr) {
-  PessimisticTransactionDB* txn_db;
+  assert(db != nullptr);
+  assert(dbptr != nullptr);
+  *dbptr = nullptr;
+  std::unique_ptr<PessimisticTransactionDB> txn_db;
+
   switch (txn_db_options.write_policy) {
     case WRITE_UNPREPARED:
       return Status::NotSupported("WRITE_UNPREPARED is not implemented yet");
     case WRITE_PREPARED:
-      txn_db = new WritePreparedTxnDB(
-          db, PessimisticTransactionDB::ValidateTxnDBOptions(txn_db_options));
+      txn_db.reset(new WritePreparedTxnDB(
+          db, PessimisticTransactionDB::ValidateTxnDBOptions(txn_db_options)));
       break;
     case WRITE_COMMITTED:
     default:
-      txn_db = new WriteCommittedTxnDB(
-          db, PessimisticTransactionDB::ValidateTxnDBOptions(txn_db_options));
+      txn_db.reset(new WriteCommittedTxnDB(
+          db, PessimisticTransactionDB::ValidateTxnDBOptions(txn_db_options)));
   }
   txn_db->UpdateCFComparatorMap(handles);
-  *dbptr = txn_db;
   Status s = txn_db->Initialize(compaction_enabled_cf_indices, handles);
+  // In case of a failure at this point, db is deleted via the txn_db destructor
+  // and set to nullptr.
+  if (s.ok()) {
+    *dbptr = txn_db.release();
+  }
   return s;
 }
 

utilities/transactions/transaction_test.h

@@ -51,7 +51,7 @@ class TransactionTestBase : public ::testing::Test {
 
   TransactionTestBase(bool use_stackable_db, bool two_write_queue,
                       TxnDBWritePolicy write_policy)
-      : use_stackable_db_(use_stackable_db) {
+      : db(nullptr), env(nullptr), use_stackable_db_(use_stackable_db) {
     options.create_if_missing = true;
     options.max_write_buffer_number = 2;
     options.write_buffer_size = 4 * 1024;
@@ -78,6 +78,7 @@ class TransactionTestBase : public ::testing::Test {
 
   ~TransactionTestBase() {
     delete db;
+    db = nullptr;
     // This is to skip the assert statement in FaultInjectionTestEnv. There
     // seems to be a bug in btrfs that the makes readdir return recently
     // unlink-ed files. By using the default fs we simply ignore errors resulted
@@ -125,6 +126,7 @@ class TransactionTestBase : public ::testing::Test {
 
   Status ReOpen() {
     delete db;
+    db = nullptr;
     DestroyDB(dbname, options);
     Status s;
     if (use_stackable_db_ == false) {
@@ -139,16 +141,23 @@ class TransactionTestBase : public ::testing::Test {
                              std::vector<ColumnFamilyHandle*>* handles) {
     std::vector<size_t> compaction_enabled_cf_indices;
     TransactionDB::PrepareWrap(&options, &cfs, &compaction_enabled_cf_indices);
-    DB* root_db;
+    DB* root_db = nullptr;
     Options options_copy(options);
     const bool use_seq_per_batch =
         txn_db_options.write_policy == WRITE_PREPARED;
     Status s = DBImpl::Open(options_copy, dbname, cfs, handles, &root_db,
                             use_seq_per_batch);
+    StackableDB* stackable_db = new StackableDB(root_db);
     if (s.ok()) {
-      s = TransactionDB::WrapStackableDB(
-          new StackableDB(root_db), txn_db_options,
-          compaction_enabled_cf_indices, *handles, &db);
+      assert(root_db != nullptr);
+      s = TransactionDB::WrapStackableDB(stackable_db, txn_db_options,
+                                         compaction_enabled_cf_indices,
+                                         *handles, &db);
+    }
+    if (!s.ok()) {
+      delete stackable_db;
+      // just in case it was not deleted (and not set to nullptr).
+      delete root_db;
     }
     return s;
   }
@@ -161,19 +170,26 @@ class TransactionTestBase : public ::testing::Test {
     TransactionDB::PrepareWrap(&options, &column_families,
                                &compaction_enabled_cf_indices);
     std::vector<ColumnFamilyHandle*> handles;
-    DB* root_db;
+    DB* root_db = nullptr;
     Options options_copy(options);
     const bool use_seq_per_batch =
         txn_db_options.write_policy == WRITE_PREPARED;
     Status s = DBImpl::Open(options_copy, dbname, column_families, &handles,
                             &root_db, use_seq_per_batch);
+    StackableDB* stackable_db = new StackableDB(root_db);
     if (s.ok()) {
+      assert(root_db != nullptr);
       assert(handles.size() == 1);
-      s = TransactionDB::WrapStackableDB(
-          new StackableDB(root_db), txn_db_options,
-          compaction_enabled_cf_indices, handles, &db);
+      s = TransactionDB::WrapStackableDB(stackable_db, txn_db_options,
+                                         compaction_enabled_cf_indices, handles,
+                                         &db);
       delete handles[0];
     }
+    if (!s.ok()) {
+      delete stackable_db;
+      // just in case it was not deleted (and not set to nullptr).
+      delete root_db;
+    }
     return s;
   }
 
@@ -207,9 +223,9 @@ class TransactionTestBase : public ::testing::Test {
     // equivalent to commit without prepare.
     WriteBatch wb;
     auto istr = std::to_string(index);
-    wb.Put("k1" + istr, "v1");
-    wb.Put("k2" + istr, "v2");
-    wb.Put("k3" + istr, "v3");
+    ASSERT_OK(wb.Put("k1" + istr, "v1"));
+    ASSERT_OK(wb.Put("k2" + istr, "v2"));
+    ASSERT_OK(wb.Put("k3" + istr, "v3"));
     WriteOptions wopts;
     auto s = db->Write(wopts, &wb);
     if (txn_db_options.write_policy == TxnDBWritePolicy::WRITE_COMMITTED) {
@@ -231,15 +247,12 @@ class TransactionTestBase : public ::testing::Test {
     WriteOptions write_options;
     Transaction* txn = db->BeginTransaction(write_options, txn_options);
     auto istr = std::to_string(index);
-    auto s = txn->SetName("xid" + istr);
-    ASSERT_OK(s);
-    s = txn->Put(Slice("foo" + istr), Slice("bar"));
-    s = txn->Put(Slice("foo2" + istr), Slice("bar2"));
-    s = txn->Put(Slice("foo3" + istr), Slice("bar3"));
-    s = txn->Put(Slice("foo4" + istr), Slice("bar4"));
-    ASSERT_OK(s);
-    s = txn->Commit();
-    ASSERT_OK(s);
+    ASSERT_OK(txn->SetName("xid" + istr));
+    ASSERT_OK(txn->Put(Slice("foo" + istr), Slice("bar")));
+    ASSERT_OK(txn->Put(Slice("foo2" + istr), Slice("bar2")));
+    ASSERT_OK(txn->Put(Slice("foo3" + istr), Slice("bar3")));
+    ASSERT_OK(txn->Put(Slice("foo4" + istr), Slice("bar4")));
+    ASSERT_OK(txn->Commit());
     if (txn_db_options.write_policy == TxnDBWritePolicy::WRITE_COMMITTED) {
       // Consume one seq per key
       exp_seq += 4;
@@ -261,20 +274,16 @@ class TransactionTestBase : public ::testing::Test {
     WriteOptions write_options;
     Transaction* txn = db->BeginTransaction(write_options, txn_options);
     auto istr = std::to_string(index);
-    auto s = txn->SetName("xid" + istr);
-    ASSERT_OK(s);
-    s = txn->Put(Slice("foo" + istr), Slice("bar"));
-    s = txn->Put(Slice("foo2" + istr), Slice("bar2"));
-    s = txn->Put(Slice("foo3" + istr), Slice("bar3"));
-    s = txn->Put(Slice("foo4" + istr), Slice("bar4"));
-    s = txn->Put(Slice("foo5" + istr), Slice("bar5"));
-    ASSERT_OK(s);
+    ASSERT_OK(txn->SetName("xid" + istr));
+    ASSERT_OK(txn->Put(Slice("foo" + istr), Slice("bar")));
+    ASSERT_OK(txn->Put(Slice("foo2" + istr), Slice("bar2")));
+    ASSERT_OK(txn->Put(Slice("foo3" + istr), Slice("bar3")));
+    ASSERT_OK(txn->Put(Slice("foo4" + istr), Slice("bar4")));
+    ASSERT_OK(txn->Put(Slice("foo5" + istr), Slice("bar5")));
     expected_commits++;
-    s = txn->Prepare();
-    ASSERT_OK(s);
+    ASSERT_OK(txn->Prepare());
     commit_writes++;
-    s = txn->Commit();
-    ASSERT_OK(s);
+    ASSERT_OK(txn->Commit());
     if (txn_db_options.write_policy == TxnDBWritePolicy::WRITE_COMMITTED) {
       // Consume one seq per key
       exp_seq += 5;
@@ -292,20 +301,16 @@ class TransactionTestBase : public ::testing::Test {
     WriteOptions write_options;
     Transaction* txn = db->BeginTransaction(write_options, txn_options);
     auto istr = std::to_string(index);
-    auto s = txn->SetName("xid" + istr);
-    ASSERT_OK(s);
-    s = txn->Put(Slice("foo" + istr), Slice("bar"));
-    s = txn->Put(Slice("foo2" + istr), Slice("bar2"));
-    s = txn->Put(Slice("foo3" + istr), Slice("bar3"));
-    s = txn->Put(Slice("foo4" + istr), Slice("bar4"));
-    s = txn->Put(Slice("foo5" + istr), Slice("bar5"));
-    ASSERT_OK(s);
+    ASSERT_OK(txn->SetName("xid" + istr));
+    ASSERT_OK(txn->Put(Slice("foo" + istr), Slice("bar")));
+    ASSERT_OK(txn->Put(Slice("foo2" + istr), Slice("bar2")));
+    ASSERT_OK(txn->Put(Slice("foo3" + istr), Slice("bar3")));
+    ASSERT_OK(txn->Put(Slice("foo4" + istr), Slice("bar4")));
+    ASSERT_OK(txn->Put(Slice("foo5" + istr), Slice("bar5")));
     expected_commits++;
-    s = txn->Prepare();
-    ASSERT_OK(s);
+    ASSERT_OK(txn->Prepare());
     commit_writes++;
-    s = txn->Rollback();
-    ASSERT_OK(s);
+    ASSERT_OK(txn->Rollback());
     if (txn_db_options.write_policy == TxnDBWritePolicy::WRITE_COMMITTED) {
       // No seq is consumed for deleting the txn buffer
       exp_seq += 0;
@@ -345,15 +350,12 @@ class TransactionTestBase : public ::testing::Test {
       // For test the duplicate keys
       auto v2 = Slice("bar2-" + istr).ToString();
       auto type = rnd.Uniform(4);
-      Status s;
       switch (type) {
         case 0:
           committed_kvs[k] = v;
-          s = db->Put(write_options, k, v);
-          ASSERT_OK(s);
+          ASSERT_OK(db->Put(write_options, k, v));
           committed_kvs[k] = v2;
-          s = db->Put(write_options, k, v2);
-          ASSERT_OK(s);
+          ASSERT_OK(db->Put(write_options, k, v2));
           break;
         case 1: {
           WriteBatch wb;
@@ -361,26 +363,22 @@ class TransactionTestBase : public ::testing::Test {
           wb.Put(k, v);
           committed_kvs[k] = v2;
           wb.Put(k, v2);
-          s = db->Write(write_options, &wb);
-          ASSERT_OK(s);
+          ASSERT_OK(db->Write(write_options, &wb));
+
         } break;
         case 2:
         case 3:
           txn = db->BeginTransaction(write_options, txn_options);
-          s = txn->SetName("xid" + istr);
-          ASSERT_OK(s);
+          ASSERT_OK(txn->SetName("xid" + istr));
           committed_kvs[k] = v;
-          s = txn->Put(k, v);
-          ASSERT_OK(s);
+          ASSERT_OK(txn->Put(k, v));
           committed_kvs[k] = v2;
-          s = txn->Put(k, v2);
-          ASSERT_OK(s);
+          ASSERT_OK(txn->Put(k, v2));
+
           if (type == 3) {
-            s = txn->Prepare();
-            ASSERT_OK(s);
+            ASSERT_OK(txn->Prepare());
           }
-          s = txn->Commit();
-          ASSERT_OK(s);
+          ASSERT_OK(txn->Commit());
           if (type == 2) {
             auto pdb = reinterpret_cast<PessimisticTransactionDB*>(db);
             // TODO(myabandeh): this is counter-intuitive. The destructor should